Closed
Bug 1465491
Opened 6 years ago
Closed 6 years ago
Web Replay: Tests
Categories
(Core Graveyard :: Web Replay, defect)
Core Graveyard
Web Replay
Tracking
(firefox62 affected)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox62 | --- | affected |
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
(Whiteboard: leave-open)
Attachments
(2 files, 2 obsolete files)
24.85 KB,
patch
|
Details | Diff | Splinter Review | |
5.56 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
This bug has new tests added for Web Replay.
Assignee | ||
Comment 1•6 years ago
|
||
Mochitests for basic recording/replaying, and debugger playing, rewinding, stepping/reverse-stepping, and evaluating.
Assignee: nobody → bhackett1024
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8981929 -
Flags: review?(jryans)
Comment on attachment 8981929 [details] [diff] [review] Part 2 - Helper functions and whitelists for web replay mochitests. Review of attachment 8981929 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks quite interesting! I have a few questions about the helpers here, but overall they look like a good starting point. ::: devtools/client/debugger/new/test/mochitest/head.js @@ +66,5 @@ > } > + > +// Specify a callback to be invoked the next time a particular message is sent > +// by a test file. > +function addMessageListener(message, callback) { Hmm, I am a bit worried about defining a global `addMessageListener` test helper here, since the name `addMessageListener` references the message manager API, but this helper works differently. What if we extend the DevTools `once` helper from shared-head.js[1] (already imported here) to work with message listeners (by adding `addMessageListener` and `removeMessageListener` to the end of the function list[2])? To use it, you would do something like: ``` await once(Services.ppmm, "RecordingFinished"); ``` If that can't be done for some reason, then let's at least give this helper a different name here. [1]: https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/devtools/client/shared/test/shared-head.js#334 [2]: https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/devtools/client/shared/test/shared-head.js#269 @@ +77,5 @@ > +} > + > +// Attach a debugger to a tab, returning a promise that resolves with the > +// debugger's thread client. > +function attachDebugger(tab) { For new code, we try to use async / await style instead of `then` callback chains for better readability. I believe most APIs used here support promises and so can converted to that style. That should even be true for things like `sourceClient.setBreakpoint` that call `this._client.request(packet)` which in recent times also supports promises (in addition to the older callback style). Let me know if there's some reason we can't use async / await here. @@ +177,5 @@ > +} > + > +// Return a pathname that can be used for a new recording file. > +function newRecordingFile() { > + return "/tmp/MochitestRecording" + Math.round(Math.random() * 1000000000); This path seems like it would fail on Windows...? Maybe you can import "resource://gre/modules/osfile.jsm"[1], and then use something like: ``` return OS.Path.join(OS.Constants.Path.tmpDir, "MochitestRecording" + Math.round(Math.random() * 1000000000)); ``` [1]: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.Path
Attachment #8981929 -
Flags: review?(jryans) → review-
Jason, should these files be modified upstream at debugger.html, or is it okay to land these patches in m-c?
Flags: needinfo?(jlaster)
Comment 5•6 years ago
|
||
Happy to land in mc first and then backport them to debugger.html
Flags: needinfo?(jlaster)
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8981916 -
Attachment is obsolete: true
Assignee | ||
Comment 7•6 years ago
|
||
Thanks for the feedback, I also updated the tests themselves to use async/await more.
Attachment #8981929 -
Attachment is obsolete: true
Attachment #8982353 -
Flags: review?(jryans)
Comment on attachment 8982353 [details] [diff] [review] Part 2 - Helper functions and whitelists for web replay mochitests. Review of attachment 8982353 [details] [diff] [review]: ----------------------------------------------------------------- Great, this looks reasonable to me! :)
Attachment #8982353 -
Flags: review?(jryans) → review+
Updated•6 years ago
|
Component: General → Web Replay
Comment 9•6 years ago
|
||
Any reason why the patch didn't land yet? Thanks
Flags: needinfo?(jryans)
Flags: needinfo?(bhackett1024)
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] [PTO until 2018/08/12] from comment #9) > Any reason why the patch didn't land yet? > Thanks I've been iterating this on the tryserver (last push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09fecbeb6e3c58e15054aa5e173e19131e75b480&selectedJob=190709548) but right now the tests are permanently orange with a shutdown crash. I can reproduce this locally and should have a fix in the next day or two.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jryans)
Flags: needinfo?(bhackett1024)
Comment 11•6 years ago
|
||
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2073834a70d6 Part 1 - Web Replay tests. https://hg.mozilla.org/integration/mozilla-inbound/rev/32c5b19c77c9 Part 2 - Helper functions and whitelists for web replay mochitests, r=jryans.
Comment 12•6 years ago
|
||
Backed out for turning bug 1270731 into permafail. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=32c5b19c77c93c9941214f533ae41c556b6ec8e4&tochange=6436144d9173d6a72128bd90b5e4efbe748b8204&filter-searchStr=dt&selectedJob=190957103 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=190957103&repo=mozilla-inbound&lineNumber=1811 Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/6436144d9173d6a72128bd90b5e4efbe748b8204 [task 2018-07-30T20:16:32.930Z] 20:16:32 INFO - Longer timeout required, waiting longer... Remaining timeouts: 1 [task 2018-07-30T20:16:32.931Z] 20:16:32 INFO - Buffered messages finished [task 2018-07-30T20:16:32.941Z] 20:16:32 INFO - TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_browser_toolbox_debugger.js | Test timed out - [task 2018-07-30T20:16:32.941Z] 20:16:32 INFO - GECKO(1622) | MEMORY STAT | vsize 611MB | residentFast 232MB | heapAllocated 60MB [task 2018-07-30T20:16:32.941Z] 20:16:32 INFO - TEST-OK | devtools/client/framework/test/browser_browser_toolbox_debugger.js | took 180072ms [task 2018-07-30T20:16:32.941Z] 20:16:32 INFO - checking window state [task 2018-07-30T20:16:32.941Z] 20:16:32 INFO - GECKO(1622) | must wait for focus Please also take a look at: https://treeherder.mozilla.org/logviewer.html#?job_id=190945585&repo=mozilla-inbound&lineNumber=285840
Flags: needinfo?(bhackett1024)
Assignee | ||
Comment 13•6 years ago
|
||
Sorry, I'll look for a workaround for the browser_browser_toolbox_debugger permafail. I'm less sure about what to do about the test-verify failure. These are the only tests we have for a complicated new feature, and until the feature has more time to stabilize (my #1 priority at this point) its tests will intermittently fail. AFAICT from that test-verify run and the try runs I've done each test has less than a 1% chance of failing, but there are a dozen or so tests in each run which could fail. What is the best way to proceed here? (There is also some irony in asking about this, since this feature has a lot of promise for helping to debug intermittent failures and thereby *reducing* the amount of orange on treeherder.)
Flags: needinfo?(bhackett1024) → needinfo?(csabou)
Comment 14•6 years ago
|
||
Hi, as far I've seen with TV failures is that they eventually point to a future tier 1 failure, so if you can reproduce this locally and have the time to care of it I would advise to do so. It eliminates the possibility to be backed out another time for that kind of failure.
Flags: needinfo?(csabou)
Comment 15•6 years ago
|
||
Hmm, browser_browser_toolbox_debugger has been a problematic test for us. We might want to disable webreplay for this test... This looks like the exception: [task 2018-07-30T20:17:05.056Z] 20:17:05 INFO - GECKO(2060) | Exception: [Exception... "Component is not available" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: chrome://devtools/content/framework/toolbox-process-window.js :: <TOP_LEVEL> :: line 384" data: no] at undefined:384 [task 2018-07-30T20:17:05.057Z] 20:17:05 INFO - GECKO(2060) | Stack: @chrome://devtools/content/framework/toolbox-process-window.js:384:1 [task 2018-07-30T20:17:05.059Z] 20:17:05 INFO - GECKO(2060) | evaluateTestScript@chrome://devtools/content/framework/toolbox-process-window.js:185:3 [task 2018-07-30T20:17:05.061Z] 20:17:05 INFO - GECKO(2060) | onNewToolbox@chrome://devtools/content/framework/toolbox-process-window.js:175:7 [task 2018-07-30T20:17:05.063Z] 20:17:05 INFO - GECKO(2060) | openToolbox@chrome://devtools/content/framework/toolbox-process-window.js:159:3 [task 2018-07-30T20:17:05.065Z] 20:17:05 INFO - GECKO(2060) | async*connect@chrome://devtools/content/framework/toolbox-process-window.js:90:11 [task 2018-07-30T20:17:05.066Z] 20:17:05 INFO - GECKO(2060) | async*@chrome://devtools/content/framework/toolbox-process-window.js:118:11 [task 2018-07-30T20:17:05.068Z] 20:17:05 INFO - GECKO(2060) | EventListener.handleEvent*@chrome://devtools/content/framework/toolbox-process-window.js:111:1
Comment 16•6 years ago
|
||
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b648ddfe64 Part 2 - Helper functions for web replay mochitests, r=jryans.
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to Jason Laster [:jlast] from comment #15) > Hmm, browser_browser_toolbox_debugger has been a problematic test for us. We > might want to disable webreplay for this test... The problem with browser_browser_toolbox_debugger seems to be due to the promise rejection whitelisting code in part 2, which is in support code that runs for a variety of tests. The above push removes this stuff; I'll add any necessary whitelisting to the individual tests themselves.
Whiteboard: leave-open
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f5b648ddfe64
Comment 19•6 years ago
|
||
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/458afcec5dbe Add one web replay test.
Assignee | ||
Comment 20•6 years ago
|
||
I'm adding one test for now since this looks good on try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a9b87e1c3dcce1807b33faaded8972889c6f2ff) and I'm not able to reproduce the TV failure locally --- ./mach --verify passes on all the new tests.
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/458afcec5dbe
Comment 22•6 years ago
|
||
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f893f5da2e63 Part 1 - Web Replay tests.
Assignee | ||
Comment 23•6 years ago
|
||
Try push for the above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da860dbe9a82845ea2d347103dfebaf3067d553c
Comment 24•6 years ago
|
||
Backed out changeset f893f5da2e63 (bug 1465491) for failing Devtools on devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js Log: https://treeherder.mozilla.org/logviewer.html#?job_id=192838326&repo=mozilla-inbound&lineNumber=8897 TEST-START | devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js 11:35:03 INFO - GECKO(799) | JavaScript error: chrome://browser/content/tabbrowser.js, line 2204: Error: Required argument triggeringPrincipal missing within addTab 11:35:48 INFO - TEST-INFO | started process screencapture 11:35:49 INFO - TEST-INFO | screencapture: exit 0 11:35:49 INFO - Buffered messages logged at 11:35:03 11:35:49 INFO - Console message: [JavaScript Error: "Error: Required argument triggeringPrincipal missing within addTab" {file: "chrome://browser/content/tabbrowser.js" line: 2204}] 11:35:49 INFO - Buffered messages finished 11:35:49 INFO - TEST-UNEXPECTED-FAIL | devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js | Test timed out - 11:35:49 INFO - Not taking screenshot here: see the one that was previously logged 11:35:49 INFO - TEST-UNEXPECTED-FAIL | devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js | A promise chain failed to handle a rejection: Required argument triggeringPrincipal missing within addTab - stack: addTab@chrome://browser/content/tabbrowser.js:2204:13 11:35:49 INFO - test@chrome://mochitests/content/browser/devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js:10:13 11:35:49 INFO - async*Tester_execTest@chrome://mochikit/content/browser-test.js:1137:9 11:35:49 INFO - nextTest/<@chrome://mochikit/content/browser-test.js:999:9 11:35:49 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59 11:35:49 INFO - Rejection date: Wed Aug 08 2018 11:35:03 GMT-0700 (PDT) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 257 11:35:49 INFO - Stack trace: 11:35:49 INFO - resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257 11:35:49 INFO - chrome://mochikit/content/browser-test.js:nextTest:748 11:35:49 INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1203 11:35:49 INFO - setTimeout handler*chrome://mochikit/content/browser-test.js:Tester_execTest:1165 11:35:49 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:999 11:35:49 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795 11:35:49 INFO - GECKO(799) | MEMORY STAT | vsize 4847MB | residentFast 791MB | heapAllocated 100MB 11:35:49 INFO - TEST-OK | devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js | took 45023ms Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f893f5da2e6335ac1bc52ffa15a848cc9cb06027 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b5e57cae88462e7b9f21d984fb449d55363b582
Flags: needinfo?(bhackett1024)
Assignee | ||
Comment 25•6 years ago
|
||
(In reply to Dorel Luca [:dluca] from comment #24) > Backed out changeset f893f5da2e63 (bug 1465491) for failing Devtools on > devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js > > Log: > https://treeherder.mozilla.org/logviewer.html#?job_id=192838326&repo=mozilla- > inbound&lineNumber=8897 > > TEST-START | > devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js > 11:35:03 INFO - GECKO(799) | JavaScript error: > chrome://browser/content/tabbrowser.js, line 2204: Error: Required argument > triggeringPrincipal missing within addTab > 11:35:48 INFO - TEST-INFO | started process screencapture > 11:35:49 INFO - TEST-INFO | screencapture: exit 0 > 11:35:49 INFO - Buffered messages logged at 11:35:03 > 11:35:49 INFO - Console message: [JavaScript Error: "Error: Required > argument triggeringPrincipal missing within addTab" {file: > "chrome://browser/content/tabbrowser.js" line: 2204}] > 11:35:49 INFO - Buffered messages finished > 11:35:49 INFO - TEST-UNEXPECTED-FAIL | > devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js > | Test timed out - > 11:35:49 INFO - Not taking screenshot here: see the one that was > previously logged > 11:35:49 INFO - TEST-UNEXPECTED-FAIL | > devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js > | A promise chain failed to handle a rejection: Required argument > triggeringPrincipal missing within addTab - stack: > addTab@chrome://browser/content/tabbrowser.js:2204:13 > 11:35:49 INFO - > test@chrome://mochitests/content/browser/devtools/client/debugger/new/test/ > mochitest/browser_dbg_rr_breakpoints-01.js:10:13 > 11:35:49 INFO - > async*Tester_execTest@chrome://mochikit/content/browser-test.js:1137:9 > 11:35:49 INFO - > nextTest/<@chrome://mochikit/content/browser-test.js:999:9 > 11:35:49 INFO - > SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome:// > mochikit/content/tests/SimpleTest/SimpleTest.js:795:59 > 11:35:49 INFO - Rejection date: Wed Aug 08 2018 11:35:03 GMT-0700 (PDT) > - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm > :: assertNoUncaughtRejections :: line 257 > 11:35:49 INFO - Stack trace: > 11:35:49 INFO - > resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257 > 11:35:49 INFO - chrome://mochikit/content/browser-test.js:nextTest:748 > 11:35:49 INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1203 > 11:35:49 INFO - setTimeout > handler*chrome://mochikit/content/browser-test.js:Tester_execTest:1165 > 11:35:49 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:999 > 11:35:49 INFO - > chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest. > waitForFocus/waitForFocusInner/focusedOrLoaded/<:795 > 11:35:49 INFO - GECKO(799) | MEMORY STAT | vsize 4847MB | residentFast > 791MB | heapAllocated 100MB > 11:35:49 INFO - TEST-OK | > devtools/client/debugger/new/test/mochitest/browser_dbg_rr_breakpoints-01.js > | took 45023ms > > Push with failures: > https://treeherder.mozilla.org/#/jobs?repo=mozilla- > inbound&revision=f893f5da2e6335ac1bc52ffa15a848cc9cb06027 > > Backout: > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > 1b5e57cae88462e7b9f21d984fb449d55363b582 This failure seems to be due to bug 1362034 (which has since been backed out) instead of f893f5da2e63 --- the 'Required argument triggeringPrincipal missing within addTab' exception was added by f68b1b76af36 (bug 1362034), which landed after f893f5da2e63, the failure above is from a push later than either of these, and f68b1b76af36 passed all devtools mochitests on 10.10 opt (the only platform where browser_dbg_rr_breakpoints-01.js runs on). Is it possible to reland f893f5da2e63?
Flags: needinfo?(bhackett1024) → needinfo?(dluca)
Comment 26•6 years ago
|
||
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac3e522f55f Part 1 - Web Replay tests.
Comment 27•6 years ago
|
||
Backed out changeset 1ac3e522f55f (bug 1465491)For failing on browser_dbg_rr_replay-02.js Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/3113161a3af10892ad42dd8bb856ad50987e6fba Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1ac3e522f55fe1d893089d31e0f650a26e7ce332&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=193067383&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=193067377&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=193096592&repo=mozilla-inbound&lineNumber=8750 It stared on the failing push on Test verify and turned in to mochitest devtools dt6 on later pushes.
Flags: needinfo?(bhackett1024)
Comment 28•6 years ago
|
||
I think this could be relanded safely the assertion is from something I was adding in Bug 1362034.
Assignee | ||
Comment 29•6 years ago
|
||
(In reply to Arthur Iakab [arthur_iakab] from comment #27) > Backed out changeset 1ac3e522f55f (bug 1465491)For failing on > browser_dbg_rr_replay-02.js > Backout revision > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > 3113161a3af10892ad42dd8bb856ad50987e6fba > Failed push > https://treeherder.mozilla.org/#/jobs?repo=mozilla- > inbound&revision=1ac3e522f55fe1d893089d31e0f650a26e7ce332&filter- > resultStatus=testfailed&filter-resultStatus=busted&filter- > resultStatus=exception&filter-classifiedState=unclassified > > Failure logs: > > https://treeherder.mozilla.org/logviewer.html#?job_id=193067383&repo=mozilla- > inbound > > https://treeherder.mozilla.org/logviewer.html#?job_id=193067377&repo=mozilla- > inbound > > https://treeherder.mozilla.org/logviewer.html#?job_id=193096592&repo=mozilla- > inbound&lineNumber=8750 > > It stared on the failing push on Test verify and turned in to mochitest > devtools dt6 on later pushes. AFAICT this is the exact same issue I needinfo'ed dluca for in comment 25 (except the TV failures, which are tier 2 per comment 14). I'm getting kind of tired of trying to land these tests.
Flags: needinfo?(bhackett1024)
Comment 30•6 years ago
|
||
Brian it looks like my patch will land which adds an assertion to the use of addTab without a triggering principal. We have a test version which can be used. Instead of using: gBrowser.addTab(url, {}) Use: BrowserTestUtils.addTab(gBrowser, url, {}); Alternatively add: gBrowser.addTab(url, {triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal()})
Flags: needinfo?(bhackett1024)
Comment 31•6 years ago
|
||
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7da8698ae52b Add some more web replay tests.
Comment 32•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7da8698ae52b
Comment 33•6 years ago
|
||
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/39ad7ffcd976 Add remaining web replay tests.
Updated•6 years ago
|
Flags: needinfo?(dluca)
Comment 34•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/39ad7ffcd976
Assignee | ||
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(bhackett1024)
Updated•4 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•