Closed Bug 1465491 Opened 6 years ago Closed 6 years ago

Web Replay: Tests

Categories

(Core Graveyard :: Web Replay, defect)

Product:
Core Graveyard
Component:
Web Replay
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox62 affected)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox62 --- affected

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

(Whiteboard: leave-open)

Attachments

(2 files, 2 obsolete files)

Part 1 - Web Replay tests.
24.85 KB, patch
Details | Diff | Splinter Review
Part 2 - Helper functions and whitelists for web replay mochitests.
5.56 KB, patch
jryans
: review+
Details | Diff | Splinter Review 
This bug has new tests added for Web Replay.
Attached patch Part 1 - Web Replay tests. (obsolete) — — Splinter Review 
Mochitests for basic recording/replaying, and debugger playing, rewinding, stepping/reverse-stepping, and evaluating.
Assignee: nobody → bhackett1024
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)
Happy to land in mc first and then backport them to debugger.html
Flags: needinfo?(jlaster)

        Attachment #8981916 -
        Attachment is obsolete: true

    
    

    
  


  
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+
Component: General → Web Replay

    
    

    
  
Any reason why the patch didn't land yet?
Thanks
Flags: needinfo?(jryans)
Flags: needinfo?(bhackett1024)

    
    

    
  
(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.

    
  
Flags: needinfo?(jryans)
Flags: needinfo?(bhackett1024)

    
  
Depends on: 1479334

    
  
Depends on: 1479339

    
    

    
  
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.

    
    

    
  
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)

    
    

    
  
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)

    
    

    
  
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)

    
    

    
  
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

    
  
Depends on: 1480426

    
    

    
  
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b648ddfe64
Part 2 - Helper functions for web replay mochitests, r=jryans.

    
    

    
  
(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

    
    

    
  
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.
Depends on: 1480994

    
    

    
  
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)

    
    

    
  
(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)

    
    

    
  
I think this could be relanded safely the assertion is from something I was adding in Bug 1362034.

    
    

    
  
(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)

    
    

    
  
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)

    
  
Flags: needinfo?(dluca)
Depends on: 1482857

    
  
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED

    
  
Flags: needinfo?(bhackett1024)

    
  
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: