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

RESOLVED FIXED in Firefox 56

Status

()

defect
P5
normal
RESOLVED FIXED
2 years ago
2 years ago

People

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

Tracking

({intermittent-failure})

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [stockwell fixed:timing])

Attachments

(1 attachment, 5 obsolete attachments)

Assignee: nobody → kustiuzhanina
Posted 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+
Posted 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+
Posted 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
Duplicate of this bug: 1386043
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]
Posted 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+
Posted 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: 2 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.