Closed Bug 1473322 Opened 6 years ago Closed 6 years ago

Stop recording settle subtests in DAMP

Categories

(DevTools :: General, defect, P2)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(firefox62 ?, firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox62 --- ?
firefox63 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1473322 - Stop recording "settle" tests on DAMP.
59 bytes, text/x-review-board-request
yulia
: review+
Details 
"settle" subtests on DAMP have been more confusing than helpful so far.
It would be great to keep recording this data over time, but for now it is a burden for every developer facing a regression on it.
Examples are bug 1468150 and bug 1454948.
Severity: normal → enhancement
Priority: -- → P2
Assignee: nobody → poirot.alex
Comment on attachment 8990969 [details]
Bug 1473322 - Stop recording "settle" tests on DAMP.

https://reviewboard.mozilla.org/r/255966/#review262826

This looks good, I can't see any issues with this. One question I have is, since I found it useful, do we have a plan for informing people writing these damp tests that something is outside of what they are testing?
Attachment #8990969 - Flags: review?(ystartsev) → review+
(In reply to Yulia Startsev [:yulia] from comment #3)
> Comment on attachment 8990969 [details]
> Bug 1473322 - Stop recording "settle" tests on DAMP.
> 
> https://reviewboard.mozilla.org/r/255966/#review262826
> 
> This looks good, I can't see any issues with this. One question I have is,
> since I found it useful, do we have a plan for informing people writing
> these damp tests that something is outside of what they are testing?

Not in the current state of things.
I considering two options, but didn't had time to look into any of that:
* have DAMP scripts ran as mochitests and see if we can replace that with assertions.
  Something like, there should be no pending reflows, requests, ...
  But that may replace problematic DAMP regression report into problematic mochitest intermittent.
* have DAMP being run a second time, where all the data is collected and graphed,
  but no reports are being made if anything regress. We could collect various experimental data
  like settle, JS allocations, ...

What I can do in this patch is still print the settle timings in stdout, without recording them, still.
That sounds like a good compromise -- that way we could track when we are making changes to the damp tests.
Comment on attachment 8990969 [details]
Bug 1473322 - Stop recording "settle" tests on DAMP.

Reflaging for review as I made significant changes.

Here is the kind of logs this new patch prints on stdout:
08:48:38     INFO -  PID 83471 | Executing test 'debugger/custom.js'
08:48:41     INFO -  PID 83471 | Waiting for state change: count sources
08:48:41     INFO -  PID 83471 | Finished waiting for state change: count sources
08:48:41     INFO -  PID 83471 | Selecting source: App.js
08:48:42     INFO -  PID 83471 | Waiting for state change: selected source
08:48:42     INFO -  PID 83471 | Finished waiting for state change: selected source
08:48:42     INFO -  PID 83471 | Waiting until: text is visible
08:48:42     INFO -  PID 83471 | Finished Waiting until: text is visible
08:48:42     INFO -  PID 83471 | Waiting until: has file metadata
08:48:42     INFO -  PID 83471 | Finished Waiting until: has file metadata
08:48:42     INFO -  PID 83471 | 'custom.jsdebugger.open.settle.DAMP' took 43.398111000000426ms.
08:48:42     INFO -  PID 83471 | Garbage collect
08:48:43     INFO -  PID 83471 | Reload page on 'custom.jsdebugger'
08:48:43     INFO -  PID 83471 | Waiting for state change: count sources
08:48:44     INFO -  PID 83471 | Finished waiting for state change: count sources
08:48:44     INFO -  PID 83471 | Waiting until: text is visible
08:48:45     INFO -  PID 83471 | Finished Waiting until: text is visible
08:48:45     INFO -  PID 83471 | Waiting until: has file metadata
08:48:45     INFO -  PID 83471 | Finished Waiting until: has file metadata
08:48:45     INFO -  PID 83471 | Wait for pending paints on 'custom.jsdebugger'
08:48:45     INFO -  PID 83471 | 'custom.jsdebugger.reload.settle.DAMP' took 24.53803700000026ms.
08:48:45     INFO -  PID 83471 | Waiting for debugger panel
Attachment #8990969 - Flags: review+ → review?(ystartsev)
Comment on attachment 8990969 [details]
Bug 1473322 - Stop recording "settle" tests on DAMP.

https://reviewboard.mozilla.org/r/255966/#review263046

I like this solution a lot. We can maybe control this somehow from the command line in the future. I have two tiny nits, but they are not significant at all.

::: testing/talos/talos/tests/devtools/addon/content/tests/head.js:111
(Diff revision 2)
>    let test = runTest(name + ".open.DAMP");
>    let toolbox = await openToolbox(tool, onLoad);
>    test.done();
>  
> -  test = runTest(name + ".open.settle.DAMP");
> +  // Settle test isn't recorded, it only prints the pending duration
> +  test = runTest(name + ".open.settle.DAMP", false);

nit: we could use a template string here 

`${name}.open.settle.damp`

::: testing/talos/talos/tests/devtools/addon/content/tests/head.js:140
(Diff revision 2)
>    await damp.reloadPage(onReload);
>    test.done();
>  
> +  // Settle test isn't recorded, it only prints the pending duration
>    dump("Wait for pending paints on '" + name + "'\n");
> -  test = runTest(name + ".reload.settle.DAMP");
> +  test = runTest(name + ".reload.settle.DAMP", false);

same nit as above, could be a template string
Attachment #8990969 - Flags: review?(ystartsev) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d44bdd3fdd4
Stop recording "settle" tests on DAMP. r=yulia
Once this lands, do you want to request uplift to beta as well?
https://hg.mozilla.org/mozilla-central/rev/2d44bdd3fdd4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1475157
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: