1387330 - [meta] Fix devtools tests skipped in e10s

Status: NEW

(DevTools :: General, enhancement, P2)

Description

[Julian Descottes [:jdescottes]]

After Bug 1386689, all devtools mochitests jobs are only run on e10s.

However we are disabling some of our tests for e10s:
- ~100 tests simply disabled in e10s http://searchfox.org/mozilla-central/search?q=skip-if+%3D+e10s+%23&case=false&regexp=false&path=devtools%2F**%2Fbrowser
- ~300 tests disabled for a combination of e10s + something else http://searchfox.org/mozilla-central/search?q=skip-if%5B%5E%23%5Cn%5D*%5B%5E!%5De10s&case=false&regexp=true&path=devtools%2F**%2Fbrowser

This means we are losing test coverage, we should review those tests and decide what to do.

Comment 1

[Julian Descottes [:jdescottes]]

Try push that removes all skip-if = e10s (and replaces skip-if = e10s && debug by skip-if = debug)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73359e32c35bcfd8d1574ea9d4e0fb66dff9b2b9

Comment 2

[Julian Descottes [:jdescottes]]

Of course my first try push had a typo: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a111f5d8ac5d56f523136b1e3d69f7c9e28280fd

Comment 3

[Julian Descottes [:jdescottes]]

Failures on Linux 64 opt:

TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_stack-03.js |
TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_stack-04.js | 
TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_stack-05.js | 
TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_stack-06.js | 
TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_stack-07.js |
TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_canvasframe_helper_01.js |
TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_canvasframe_helper_03.js | 
TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_canvasframe_helper_04.js | 
TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_canvasframe_helper_05.js | 
TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_canvasframe_helper_06.js | 
TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_navigateEvents.js | 
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_console_dead_objects.js | 
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_console_private_browsing.js |
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_bug_595934_message_categories.js | 
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_bug_632347_iterators_generators.js |
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_property_provider.js | 
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_script_errordoc_urls.js | 
TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_bfcache.js | 
TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-event-01.js | 
TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-event-02.js | 
TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_cmd-dbg.js | 
TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_globalactor.js | 
TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_multiple-windows.js | 
TEST-UNEXPECTED-FAIL | devtools/client/shared/test/browser_layoutHelpers-getBoxQuads.js |
TEST-UNEXPECTED-FAIL | devtools/client/shared/test/browser_toolbar_webconsole_errors_count.js |
TEST-UNEXPECTED-FAIL | devtools/client/commandline/test/browser_cmd_appcache_valid.js | 
TEST-UNEXPECTED-FAIL | devtools/client/commandline/test/browser_cmd_cookie.js | 
TEST-UNEXPECTED-FAIL | devtools/client/commandline/test/browser_cmd_cookie_host.js | 
TEST-UNEXPECTED-FAIL | devtools/client/commandline/test/browser_cmd_media.js | 
TEST-UNEXPECTED-FAIL | devtools/client/commandline/test/browser_gcli_async.js |

Comment 4

[Patrick Brosset <:pbro>]

Do I understand correctly that among the ~400 disabled tests, only 30 fail anymore? That sounds like quite a lot, and worth us doing a push now to make the rest of them pass.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Patrick Brosset <:pbro> from comment #4)
> Do I understand correctly that among the ~400 disabled tests, only 30 fail
> anymore? That sounds like quite a lot, and worth us doing a push now to make
> the rest of them pass.

Only 100 tests were ran _only_ in non-e10s. The others were skipped on e10s + some other setting (such as debug), so they still run in some way. From the list of 100 non-e10s only tests, 30 tests seem to fail consistently. Most of them fail for issues with CPOWs if I remember correctly. 

Note that some of the tests skipped on e10s were skipped because of intermittent failures. There's no way to know if they won't start being intermittent again. For instance see https://hg.mozilla.org/mozilla-central/rev/1db2ae4b7124

Comment 6

[Alexandre Poirot [:ochameau]]

Looks like nothing changed since August.(In reply to Julian Descottes [:jdescottes][:julian] from comment #0)
> After Bug 1386689, all devtools mochitests jobs are only run on e10s.
> 
> However we are disabling some of our tests for e10s:
> - ~100 tests simply disabled in e10s
> http://searchfox.org/mozilla-central/search?q=skip-
> if+%3D+e10s+%23&case=false&regexp=false&path=devtools%2F**%2Fbrowser

Looks like it decreased a bit, we are now down to 88 tests.

A slightly more precise regexp report 93 tests disabled completely on e10s:
$ egrep skip-if -r devtools/ | grep e10s | grep -v \!e10s | grep -vE "e10s +&&"
All the 63 tests in gcli, devtools/client/commandline/test/browser.ini
33 files in shared client, devtools/client/shared/test/browser.ini
22 tests in the debugger, devtools/client/debugger/test/mochitest/browser2.ini and devtools/client/debugger/test/mochitest/browser.ini.
15 tests in the old console, devtools/client/webconsole/test/browser.ini
10 tests in the server, devtools/server/tests/browser/browser.ini
7 tests in the new console, devtools/client/webconsole/new-console-output/test/mochitest/browser.ini
3 tests in the inspector, devtools/client/inspector/shared/test/browser.ini
1 test in framework, devtools/client/framework/test/browser.ini
1 test in WebIDE, devtools/client/webide/test/browser.ini

Any idea why there is two browser.ini files for the debugger??

I imagine, we can triage based on the importance of each component.
webide and the old console sounds not so important to address. gcli may also be less important than the others.

I ran test against a recent m-c:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=62dd476b2c920c984ee05f362020d4038457425b&selectedJob=150174376
  $ sed -E "/(skip-if *= *)e10s *#/d" -i $(egrep skip-if -r devtools/ | grep e10s | grep -v \!e10s | grep -vE "e10s +&&" | cut -d: -f1 | uniq)
  Then manually edit the leftovers found via: $ egrep skip-if -r devtools/ | grep e10s | grep -v \!e10s | grep -vE "e10s +&&"

And get these permafails:
dt2
- devtools/client/webconsole/test/browser_console_dead_objects.js
- devtools/client/webconsole/test/browser_console_private_browsing.js
- devtools/client/webconsole/test/browser_webconsole_bug_595934_message_categories.js
- devtools/client/webconsole/test/browser_webconsole_bug_632347_iterators_generators.js
- devtools/client/webconsole/test/browser_webconsole_property_provider.js
- devtools/client/webconsole/test/browser_webconsole_script_errordoc_urls.js
dt4
- devtools/client/shared/test/browser_layoutHelpers-getBoxQuads.js 
- devtools/client/shared/test/browser_toolbar_webconsole_errors_count.js
dt7
- devtools/client/debugger/test/mochitest/browser_dbg_stack-03.js
- devtools/client/debugger/test/mochitest/browser_dbg_tabactor-01.js
- devtools/client/debugger/test/mochitest/browser_dbg_tabactor-02.js
- devtools/client/debugger/test/mochitest/browser_dbg_variables-view-edit-click.js
dt8
- devtools/client/commandline/test/browser_cmd_appcache_valid.js
- devtools/client/commandline/test/browser_cmd_cookie.js
- devtools/client/commandline/test/browser_cmd_cookie_host.js
- devtools/client/commandline/test/browser_cmd_media.js
- devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-event-01.js
- devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-event-02.js
- devtools/client/debugger/test/mochitest/browser_dbg_cmd-break.js
- devtools/client/debugger/test/mochitest/browser_dbg_cmd-dbg.js
- devtools/client/debugger/test/mochitest/browser_dbg_globalactor.js
- devtools/client/debugger/test/mochitest/browser_dbg_iframes.js
- devtools/client/debugger/test/mochitest/browser_dbg_multiple-windows.js
- devtools/server/tests/browser/browser_canvasframe_helper_01.js
- devtools/server/tests/browser/browser_canvasframe_helper_03.js
- devtools/server/tests/browser/browser_canvasframe_helper_04.js
- devtools/server/tests/browser/browser_canvasframe_helper_05.js
- devtools/server/tests/browser/browser_canvasframe_helper_06.js
- devtools/server/tests/browser/browser_navigateEvents.js

=> 29 tests (vs 30 from comment 3)

> - ~300 tests disabled for a combination of e10s + something else
> http://searchfox.org/mozilla-central/search?q=skip-if%5B%5E%23%5Cn%5D*%5B%5E!
> %5De10s&case=false&regexp=true&path=devtools%2F**%2Fbrowser

This is unchanged, we are still at 299.
This sounds like something completely independent that can be done at any time, and hopefully should be easy to do?

Comment 7

[Julian Descottes [:jdescottes]]

Thanks for getting more up-to-date info!

> All the 63 tests in gcli, devtools/client/commandline/test/browser.ini

So that's actually 92 tests outside of GCLI + 63 tests in GCLI, right?

> 7 tests in the new console, devtools/client/webconsole/new-console-output/test/mochitest/browser.ini

I think all the skips in the new-console are commented out at the moment (eg "# old console skip-if = e10s"). When migrating tests, we usually try to remove such skip ifs. 

> Any idea why there is two browser.ini files for the debugger??

According to the headers of the ini files, it's in order to facilitate chunking (bug 1294489)

> => 29 tests (vs 30 from comment 3)

If you compare our lists there are some significant differences despite the similar counts. Either because something changed in 4 months or because these tests are really intermittent. We should probably get a few more retriggers before narrowing down a list of tests to fix in e10s. Or we simply don't care about that and just migrate all the tests disabled in e10s. 

> I imagine, we can triage based on the importance of each component.

Agreed. Looking at your list, we could set:
- high priority to inspector, shared client, server, framework, debugger (69 tests)
- low priority to old console, webide, gcli (79 tests)

> This is unchanged, we are still at 299.
> This sounds like something completely independent that can be done at any time, 
> and hopefully should be easy to do?

I agree, no reason to address this here.

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #7)
> Thanks for getting more up-to-date info!
> 
> > All the 63 tests in gcli, devtools/client/commandline/test/browser.ini
> 
> So that's actually 92 tests outside of GCLI + 63 tests in GCLI, right?

Sorry, I don't get it, 63 is the number of test declared in devtools/client/commandline/test/browser.ini.
I assumed it is all the tests for gcli.
Overall there is 155 test being disabled on e10s.

> > 7 tests in the new console, devtools/client/webconsole/new-console-output/test/mochitest/browser.ini
> 
> I think all the skips in the new-console are commented out at the moment (eg
> "# old console skip-if = e10s"). When migrating tests, we usually try to
> remove such skip ifs. 

Oh right, yes, there is no test completely disabled on e10s for the new console, that's great!

> > Any idea why there is two browser.ini files for the debugger??
> 
> According to the headers of the ini files, it's in order to facilitate
> chunking (bug 1294489)

Ah ok, makes sense.

> 
> > => 29 tests (vs 30 from comment 3)
> 
> If you compare our lists there are some significant differences despite the
> similar counts. Either because something changed in 4 months or because
> these tests are really intermittent. We should probably get a few more
> retriggers before narrowing down a list of tests to fix in e10s. Or we
> simply don't care about that and just migrate all the tests disabled in
> e10s. 

Yes, obviously this try push only consider permafailure, but there might be intermittent in this list.
We will have to be careful about intermittents.
I would like to first focus on the perma failing so that it is easier to identify the intermittents.

> > I imagine, we can triage based on the importance of each component.
> 
> Agreed. Looking at your list, we could set:
> - high priority to inspector, shared client, server, framework, debugger (69
> tests)
> - low priority to old console, webide, gcli (79 tests)

Sounds great.

Comment 9

[Alexandre Poirot [:ochameau]]

New try run with latest tip:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82840a826876621496af1c3cbb41169da6142157&selectedJob=158006156
This is mostly multiple test coming from:
* devtools/client/webconsole/test/
  I imagine we can block on bug 1400847.
* devtools/client/commandline/test/
  let's consider gcli as already removed per bug 1429421.
* devtools/client/debugger/test/mochitest/
  I'm not sure what is the plan here.
And there is only bug 1427988 outside of these ones.

Comment 10

[Julian Descottes [:jdescottes]]

It appears that DevTools tests no longer run in non-e10s now, so bumping this to P1.

Comment 11

[Alexandre Poirot [:ochameau]]

Note that a set of these tests fail because they are registering test actors via DebuggerServer.registerModule that registers them only in the parent process.

I was able to make it work in bug 1477988 by using a content task. We may want to expose a test helper to easily register actors over all processes?

 await ContentTask.spawn(gBrowser.selectedBrowser, ACTOR_URL, url => {
    const { require } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});
    const { DebuggerServer } = require("devtools/server/main");
    DebuggerServer.registerModule(url, {
      prefix: "inContent",
      constructor: "InContentActor",
      type: { target: true },
    });
 });

For example bug 1478244 pass when modifying its registerModule call with that code (in addition to a workaround because chrome://mochitests/ URI isn't registered in the content process...).
I'll submit a patch in that bug to introduce such helper and follow up there.
But I think we should coordinate here fixing all tests that fail because of that.

Comment 12

[Alexandre Poirot [:ochameau]]

I tried to go over all tests related to devtools/server that are still in debugger test folder.
A good followup would be to move them to server as described per bug 1470037.

Otherwise, there is a couple of tests in debugger that I'm not sure are worth fixing?
They all seem to test features of the old debugger:
   1478229: Enable client/debugger/test/mochitest/browser_dbg_addon-modules.js
   1478230: Enable client/debugger/test/mochitest/browser_dbg_addon-modules-unpacked.js
   1478231: Enable client/debugger/test/mochitest/browser_dbg_break-on-dom-event-02.js
   1478232: Enable client/debugger/test/mochitest/browser_dbg_breakpoints-break-on-last-line-of-script-on-reload.js
   1478233: Enable client/debugger/test/mochitest/browser_dbg_breakpoints-disabled-reload.js
   1478235: Enable client/debugger/test/mochitest/browser_dbg_hide-toolbar-buttons.js
   1478236: Enable client/debugger/test/mochitest/browser_dbg_iframes.js
   1478238: Enable client/debugger/test/mochitest/browser_dbg_location-changes-03-new.js
   1478239: Enable client/debugger/test/mochitest/browser_dbg_location-changes-04-breakpoint.js
   1478241: Enable client/debugger/test/mochitest/browser_dbg_paused-keybindings.js
   1478242: Enable client/debugger/test/mochitest/browser_dbg_search-global-03.js
   1478243: Enable client/debugger/test/mochitest/browser_dbg_source-maps-04.js
We may only try to ensure the new debugger is tested against the same features?

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Moving to p2 because no activity for at least 24 weeks.
See How Do You Triage for more information