Closed
Bug 1337887
Opened 8 years ago
Closed 7 years ago
Get mochitest a11y tests running reliably with e10s turned on (or convert them to bc tests)
Categories
(Core :: Disability Access APIs, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jimm, Assigned: eeejay)
References
Details
(Whiteboard: aes-)
Attachments
(1 file)
(deleted),
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
For a11y coverage, we want to get these running. These tests cover core a11y functionality. We currently have them running under non-e10s which eventually we'll want to turn off.
* Windows 7 opt: non-e10s
* Windows 7 pgo: no coverage
* Windows 7 VM debug: no coverage
* Windows 8 opt: no coverage
* Windows 8 x64 opt: non-e10s
* Windows 8 x64 pgo: non-e10s
* Windows 8 x64 debug: non-e10s
* Windows 10 x64 VM opt: no coverage
* Windows 10 x64 VM debug: no coverage
* Windows 2012 opt: no coverage
* Windows 2012 pgo: no coverage
* Windows 2012 debug: no coverage
* Windows 2012 x64 opt: no coverage
* Windows 2012 x64 pgo: no coverage
* Windows 2012 x64 debug: no coverage
Also need to take a look at linux and osx to see where we are there.
Comment 1•8 years ago
|
||
So we're on the same page, mochitest-other consists these days of only mochitest-a11y and mochitest-chrome.
For mochitest-chrome, we did a thorough audit back during the initial e10s rollout and ported many tests over to other suites (with remaining tests being deemed not-applicable to e10s). So I don't think there's anything to do there still unless we want to take another look at the remaining tests to ensure that nothing else needs porting?
For mochitest-a11y, it's my understanding that Yura did a similar audit and porting effort. But NI him to confirm.
Assuming the above is all accurate, I'm not sure there's anything that needs to be done here?
Flags: needinfo?(yzenevich)
Reporter | ||
Comment 2•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> So we're on the same page, mochitest-other consists these days of only
> mochitest-a11y and mochitest-chrome.
>
> For mochitest-chrome, we did a thorough audit back during the initial e10s
> rollout and ported many tests over to other suites (with remaining tests
> being deemed not-applicable to e10s). So I don't think there's anything to
> do there still unless we want to take another look at the remaining tests to
> ensure that nothing else needs porting?
>
> For mochitest-a11y, it's my understanding that Yura did a similar audit and
> porting effort. But NI him to confirm.
>
> Assuming the above is all accurate, I'm not sure there's anything that needs
> to be done here?
Within a year or so, we'll stop supporting the non-e10s run mode, so I don' think we want to continue to run any test suite under it. Once we get these switched over to e10s, we can turn non-e10s runs off.
Comment 3•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> So we're on the same page, mochitest-other consists these days of only
> mochitest-a11y and mochitest-chrome.
>
> For mochitest-chrome, we did a thorough audit back during the initial e10s
> rollout and ported many tests over to other suites (with remaining tests
> being deemed not-applicable to e10s). So I don't think there's anything to
> do there still unless we want to take another look at the remaining tests to
> ensure that nothing else needs porting?
>
> For mochitest-a11y, it's my understanding that Yura did a similar audit and
> porting effort. But NI him to confirm.
>
> Assuming the above is all accurate, I'm not sure there's anything that needs
> to be done here?
So I ported a number of tests that are relevant to e10s, but not all. I'm not sure if these tests fail with e10s enabled, since they run in the same process. If they pass we should just run them, if there are issues, ideally, they should all be ported to bc type, and definitely new ones should all be bc.
Flags: needinfo?(yzenevich)
Comment 4•8 years ago
|
||
Also wondering what David's and Alex's thoughts are about this..
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(dbolter)
Comment 5•8 years ago
|
||
Here's the current situation as I understand it:
Yura ported some mochitests to ensure we are testing IPC as well, these are:
https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser
This is good and gives us decent confidence for correctness including IPC.
There are a lot more remaining correctness mochitests:
https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest
These run in single process mode in 'oth'. We want to know if the can be run in multi-process even though they wouldn't test IPC (presumably hoping that the test suite can run in the content process and still show up as green/passing on tbpl)? I don't recall this working but would be good to confirm.
Flags: needinfo?(dbolter)
Reporter | ||
Updated•8 years ago
|
Assignee: jmathies → nobody
Updated•8 years ago
|
Assignee: nobody → yzenevich
Comment 6•8 years ago
|
||
Got a Try run of mochitest-a11y with e10s forced on:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a82d9f6dd2333778959595b6b61843149db0a0f6&group_state=expanded
It's a start anyway! :-)
Also, here's the patch to use for subsequent pushes to Try:
https://hg.mozilla.org/try/raw-rev/3f267f8f28dbf36c6a3289f044edac83298f4e87
Comment 7•8 years ago
|
||
I think the confusion with e10s/disable-e10s logging comes from here:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/TestRunner.js#465
With the patch we default to e10s however the TestRunner will report e10s only if current process type is not default which, I guess, is the same as parent.
Comment 8•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> Got a Try run of mochitest-a11y with e10s forced on:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a82d9f6dd2333778959595b6b61843149db0a0f6&group_state=e
> xpanded
>
> It's a start anyway! :-)
>
> Also, here's the patch to use for subsequent pushes to Try:
> https://hg.mozilla.org/try/raw-rev/3f267f8f28dbf36c6a3289f044edac83298f4e87
I think for local testing, e.g. changes in testing/mochitest/runtests.py we need to still keep chrome check and only remove a11y?
Comment 9•8 years ago
|
||
I don't think it'd really matter one way or the other.
Comment 11•8 years ago
|
||
Ideally I would want to see all our tests written as b-c tests: some of the tests that deal with content across different frames will are failing and writing the mochitest that will pass will take as long as writing a b-c test. Other tests should work in theory with e10s enabled but some fail for reasons that need further investigation.
Flags: needinfo?(yzenevich)
Updated•8 years ago
|
Flags: needinfo?(surkov.alexander)
Reporter | ||
Comment 12•8 years ago
|
||
Not blocking rollout. We can continue to run the existing a11y mochitest suite under non-e10s for now. The tests cover what they need to test under the current configuration.
Summary: Get OTH tests running under e10s on Windows → Get mochitest a11y tests running reliably with e10s turned on (or convert them to bc tests)
Whiteboard: aes+ → aes-
Reporter | ||
Updated•8 years ago
|
Component: General → Disability Access APIs
Assignee | ||
Updated•8 years ago
|
Assignee: yzenevich → eitan
Assignee | ||
Comment 13•8 years ago
|
||
Logs are gone from previous run. Here is another one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=088b43d116595480154fd423fd2a79862886fbe5
Assignee | ||
Comment 14•7 years ago
|
||
Pulled out all the problematic tests (and tried to fix one). This try will be maybe green-ish.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d992ed5e111f082cf77db15c3d7b32387ec73b
The rest of the tests will either need to be fixed or ported to browser tests.
Assignee | ||
Comment 15•7 years ago
|
||
I went back to see if there is a simpler way of fixing our current mochitests, it seems like most of the issues are opening a remote window and failing to get a reference to its DOM. If we force it to be non-remote, most of these tests should work in "e10s" mode as well.
The orginal force-e10s patch RyanVM put here stopped working, so I am not 100% convinced e10s is force on in all cases. But here is a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6756b84c66bfd9a62b3acf75bc8c229fcc1ad863
The two tests that fail in e10s are the plugin and <embed> test in Windows.
RyanVM, does this look promising?
Flags: needinfo?(ryanvm)
Comment 16•7 years ago
|
||
Per our IRC discussion, this looks good I think. But we should verify that this approach won't break if/when support for non-remote windows breaks.
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 17•7 years ago
|
||
Sorry for dropping this. This try push is the best ever.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cd94495033ab15d9f0e3eeb230032884d7cd328
Blake, can we rely on opening windows in non-remote mode for testing purposes?
Flags: needinfo?(mrbkap)
Comment 18•7 years ago
|
||
I don't know how much I'd rely on that. I get the impression that folks are pretty eager to stop supporting e10s once a11y supports it and I'd imagine that would include cutting support for loading foreign content in the parent process.
Gabor or Felipe might have more thoughts about this, though.
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 20•7 years ago
|
||
I think opening the windows non-remote is a no-brainer for now. So we should just push that. After that, we will have a couple of tests to tackle.
If non-remote won't be an option in the future, we'll figure it out.
Flags: needinfo?(eitan)
Assignee | ||
Comment 21•7 years ago
|
||
Attachment #8906144 -
Flags: review?(yzenevich)
Comment 22•7 years ago
|
||
Comment on attachment 8906144 [details] [diff] [review]
Open new browser windows as non-remote in a11y mochitests. r?yzen
Review of attachment 8906144 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good,thanks
Attachment #8906144 -
Flags: review?(yzenevich) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 23•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfdf38e0d9be
Open new browser windows as non-remote in a11y mochitests. r=yzen
Keywords: checkin-needed
Comment 24•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•