Closed Bug 1385417 Opened 7 years ago Closed 7 years ago

Intermittent toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js | test_sendOnTimeout - [test_sendOnTimeout : 20] Should have recorded a health ping. - "ping-on-timeout" == "health"

Categories

(Toolkit :: Telemetry, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: katejimmelon)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:timing])

Attachments

(1 file, 5 obsolete files)

Assignee: nobody → kustiuzhanina
Attached patch fixTimeout.patch (obsolete) — Splinter Review
Attachment #8891948 - Flags: review?(gfritzsche)
Comment on attachment 8891948 [details] [diff] [review]
fixTimeout.patch

Review of attachment 8891948 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
@@ +136,3 @@
>    await TelemetryController.submitExternalPing(PING_TYPE, {});
> +
> +  let ac = new TelemetryArchiveTesting.Checker();

"// Wait for the health ping to be triggered."

@@ +140,5 @@
> +  await waitForConditionWithPromise(() => {
> +    ac.promiseFindPing("health", []);
> +  },"Failed to find health ping", 10);
> +
> +  TelemetrySend.testResetTimeOutToDefault();

"// Reset and trigger ping send activity.
Attachment #8891948 - Flags: review?(gfritzsche) → review+
Attached patch fixTimeout.patch (obsolete) — Splinter Review
Attachment #8891948 - Attachment is obsolete: true
Attachment #8892015 - Flags: review?(gfritzsche)
Comment on attachment 8892015 [details] [diff] [review]
fixTimeout.patch

Review of attachment 8892015 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
@@ +31,4 @@
>    telemetryHealthPing.Policy.clearSchedulerTickTimeout = clear;
>  }
>  
> +async function waitForConditionWithPromise(promiseFn, timeoutMsg, tryCount) {

Give this a default value, `tryCount = 30`?
Attachment #8892015 - Flags: review?(gfritzsche) → review+
Attached patch fixTimeout.patch (obsolete) — Splinter Review
Attachment #8892015 - Attachment is obsolete: true
Attachment #8892021 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4446ecfee3d7
Fix sendOnTimeout test. r=gfritzsche
Keywords: checkin-needed
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7e4865a477

That caused near-permaorange (well over 90%) on Win8 in "test_sendOnlyTopTenDiscardedPings - [test_sendOnlyTopTenDiscardedPings : 170] Should have recorded a health ping. - "ping-on-timeout" == "health"" like https://treeherder.mozilla.org/logviewer.html#?job_id=119861691&repo=mozilla-inbound while continuing to intermittently fail on Win7 either the same way, or "test_sendOnTimeout - [test_sendOnTimeout : 21] Should have recorded a health ping. - "ping-on-timeout" == "health"" like https://treeherder.mozilla.org/logviewer.html#?job_id=119822323&repo=mozilla-inbound

(I don't actually need any info from the needinfo, it's just an incredibly stupid thing we do because some people ignore all of their bugmail, including comments on bugs where they are the assignee, and we pretend that's somehow okay.)
Flags: needinfo?(kustiuzhanina)
Whiteboard: [stockwell needswork]
Attached patch fixTimeout.patch (obsolete) — Splinter Review
Attachment #8892021 - Attachment is obsolete: true
Flags: needinfo?(kustiuzhanina)
Attachment #8892836 - Flags: review?(gfritzsche)
Comment on attachment 8892836 [details] [diff] [review]
fixTimeout.patch

Review of attachment 8892836 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
@@ +127,4 @@
>    PingServer.clearRequests();
>    let PING_TYPE = "ping-on-timeout";
>  
> +  fakePingSendTimer(() => {}, () => {});

Add: // Disable send retry to make this test more deterministic.

@@ +139,2 @@
>      PingServer.resetPingHandler();
> +    res.processAsync();

Add: // We don't finish the response yet to make sure to trigger a timeout.

@@ +156,5 @@
> +  PingServer.resetPingHandler();
> +  TelemetrySend.notifyCanUpload();
> +
> +  let pings = await PingServer.promiseNextPings(2);
> +  let healthPing = pings[0].type === "health" ? pings[0] : pings[1];

... = pings.find(p => p.type == ...);

Then assert that we found a health ping.
Attachment #8892836 - Flags: review?(gfritzsche) → review+
Attached patch fixTimeout.patch (obsolete) — Splinter Review
Attachment #8892836 - Attachment is obsolete: true
Attachment #8892895 - Flags: review+
Attachment #8893729 - Flags: review+ → review?(gfritzsche)
Attachment #8893729 - Flags: review?(gfritzsche) → review+
You can add the keyword checkin-needed without removing the keyword intermittent-failure, just stick a comma after the existing one and type another.
https://hg.mozilla.org/mozilla-central/rev/7164463da2f8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
https://hg.mozilla.org/releases/mozilla-beta/rev/4923833da3f9
Flags: in-testsuite+
Whiteboard: [stockwell needswork] → [stockwell fixed]
Whiteboard: [stockwell fixed] → [stockwell fixed:timing]
You need to log in before you can comment on or make changes to this bug.