Closed Bug 1385417 Opened 6 years ago Closed 6 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)

Product:
Toolkit
Component:
Telemetry
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
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)

fixTimeout.patch
4.15 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
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+

    
  
Keywords: intermittent-failure → checkin-needed

    
    

    
  
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+

    
  
Keywords: intermittent-failure → checkin-needed

    
    

    
  
You can add the keyword checkin-needed without removing the keyword intermittent-failure, just stick a comma after the existing one and type another.
Keywords: intermittent-failure

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/7164463da2f8
Status: NEW → RESOLVED
Closed: 6 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

    
    

    
  
https://hg.mozilla.org/releases/mozilla-beta/rev/4923833da3f9

          status-firefox56:
          --- → fixed
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.