Closed Bug 1244755 Opened 4 years ago Closed 4 years ago

Enable devtools/client/inspector/layout/test with e10s

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(e10s+, firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
e10s + ---
firefox47 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

For now, the whole test suite is disabled. And I think this might have been the case since the beginning. Some tests might just pass in fact. So let's try and enable the whole thing.

Luckily, this test folder has very few tests (~10), so it should be possible to enable them all in one bug.
Assignee: nobody → pbrosset
I'll use the opportunity to clean some things up in this test directory.
In particular, some of the tests use a custom addTest/runTests test runner thing that others don't, and certainly no other tests in devtools use either. So I'll remove this for more consistency.
I'll try to make eslint clean-ups too, fix unhandled rejected promises if there are any, and maybe split tests too (some of these are awfully long).
Blocks: e10s-tests
tracking-e10s: --- → +
Comment on attachment 8716003 [details]
MozReview Request: Bug 1244755 - 1 - Enable browser_layout.js with e10s by using the testActor

https://reviewboard.mozilla.org/r/33667/#review30565
Attachment #8716003 - Flags: review?(mratcliffe) → review+
Comment on attachment 8716004 [details]
MozReview Request: Bug 1244755 - 2 - Remove addTest logic from layout-view tests for better consistency

https://reviewboard.mozilla.org/r/33669/#review30567
Attachment #8716004 - Flags: review?(mratcliffe) → review+
Attachment #8716005 - Flags: review?(mratcliffe) → review+
Comment on attachment 8716005 [details]
MozReview Request: Bug 1244755 - 3 - Remove CPOW usages and eslint warnings from devtools/client/inspector/layout

https://reviewboard.mozilla.org/r/33671/#review30569
r+ but the reason those were disabled was due to intermittent oranges.

If the mouse cursor on the test boxes happened to be over a layout view region when the tests run then the tests are broken.

We should ensure that running the tests with a mouse cursor over a layout region causes these oranges.

I will try to find the original bug (I believe we were waiting on platform work for the fix).
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #8)
> r+ but the reason those were disabled was due to intermittent oranges.
> 
> If the mouse cursor on the test boxes happened to be over a layout view
> region when the tests run then the tests are broken.
> 
> We should ensure that running the tests with a mouse cursor over a layout
> region causes these oranges.
> 
> I will try to find the original bug (I believe we were waiting on platform
> work for the fix).
Thanks Mike, that'd be great. I'd love to learn more about this.
One thing might help though: in head.js, we mock the highlighter so that no requests are actually sent when the cursor is over one of the regions.
Seems to still be an issue so if we re-enable these tests we will have intermittent oranges (see comment 8) until bug 1028895 is fixed, which is blocked by bug 1029451, which ttromey was working on 9 months ago.
Maybe mocking the layout view would be a workaround but the best fix would be to add EventUtils.disableNonTestMouseEvents(true) to the start of our tests and make the call work.

In fact, I think this should be the default for *all* mochitests because we never want mouseover events on a test box to interfere with running tests.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #11)
> In fact, I think this should be the default for *all* mochitests because we
> never want mouseover events on a test box to interfere with running tests.
That makes a lot of sense. I'm sort of surprised we don't have more problems related to this.
I tried running the tests locally as well as move my mouse over the layout-view while they were running.
I managed to make this test fail: devtools\client\inspector\layout\test\browser_layout_guides.js
Looking at the code, it's obvious that it would fail if mouseover events, other than the ones simulated by the test, were to happen. But I don't see how this would happen more with e10s than non-e10s.
And this patch is only about enabling these tests with e10s.
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #14)
> I tried running the tests locally as well as move my mouse over the
> layout-view while they were running.
> I managed to make this test fail:
> devtools\client\inspector\layout\test\browser_layout_guides.js
> Looking at the code, it's obvious that it would fail if mouseover events,
> other than the ones simulated by the test, were to happen. But I don't see
> how this would happen more with e10s than non-e10s.
> And this patch is only about enabling these tests with e10s.

Of course, feel free to remove the dependencies... I didn't realize that these tests were still enabled in non-E10S mode.

At some point we should fix or, at least, workaround the problem to keep our orange count down.
I tried to uplift these to aurora to get e10s greened up, but hit conflicts. Could we get rebased patches if these would be useful on aurora?
Flags: needinfo?(pbrosset)
(In reply to Wes Kocher (:KWierso) from comment #18)
> I tried to uplift these to aurora to get e10s greened up, but hit conflicts.
> Could we get rebased patches if these would be useful on aurora?
There's indeed quite a lot of conflicts when applying. Getting patches for aurora pretty much means re-applying them manually: that is, going through the patch files and applying the changes one by one by hand. This is going to be really time consuming and error-prone. Not sure if it is worth it to be honest.
Flags: needinfo?(pbrosset)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.