988313 - Clean up the styleinspector tests

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

See bug 968727's first comment for a description.
This bug should focus on the /browser/devtools/styleinspector/test folder only.

Comment 1

[Patrick Brosset <:pbro>]

Currently working on a massive patch for this.
It's proving pretty hard to keep up with changes done in fx-team, since everytime I have a really non-trivial merge to do (head.js and all test files have changed a lot).
I should hopefully be able to attach the first patch for review soon.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug988313-cleanup-styleinspector-tests.patch

Attaching the patch for safe-keeping.
This first patch is almost ready for review and try build. Just waiting for a couple of things:
- a final round of checks on my side for obvious mistakes (remaining TODOs and FIXMEs, ...)
- the eyedropper patch has been backed out again after I merged in one of its tests, so I'd rather wait for it to land again

This patch rewrites all tests and the head.js file. The new patterns used are the same as in the markupview tests.

After this patch, I'll be working on a couple more patches:
- at least one to split long tests (we have some tests that time out at the moment and generally, our tests are way too long),
- one patch to rename all tests to be more consistent

Comment 3

[Patrick Brosset <:pbro>]

Attachment: bug988313-1-cleanup-styleinspector-tests v2.patch

I think this is ready for review.
Sorry Mike it's a big one, but as for the markupview tests (which Brian reviewed), I don't think you should go over each and every file: what I've done is rewrite the files as per the new test running pattern, but I haven't rewritten the actual tests functionality, so the actual meat of each test should be the same, although moved around in the file.
We need to make sure the test pattern you see is fine (should be exactly the same as the one of the markupview tests) and that head.js looks correct.
And of course that the try build is green: https://tbpl.mozilla.org/?tree=Try&rev=bb667d187aa1

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug988313-2-splitting-tests.patch

This is part 2.
This part splits a few big tests in several small ones.

Don't worry about the test file names, I have a third patch coming up that will rename everything nicely and consistently.

Same try build as for the part 1 patch: https://tbpl.mozilla.org/?tree=Try&rev=bb667d187aa1

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8400644 [details] [diff] [review]
bug988313-1-cleanup-styleinspector-tests v2.patch

Review of attachment 8400644 [details] [diff] [review]:
-----------------------------------------------------------------

I am always a great fan of any change that improves the maintainability of our code, especially when it helps break down any barriers to contributing. Awesome work!

Comment 6

[Ryan VanderMeulen [:RyanVM]]

I'd love to run these through Cedar too where we have the browser-chrome tests running in chunks on all platforms and the devtools tests split out into mochitest-dt. Of particular note, styleinspector tests are the most frequent oranges we still see in the mochitest-dt runs. However, there's way too much rebasing needed for the current patches (mostly in head.js) for me to do it myself.

BTW, I don't suppose I could talk you into sorting the test names alphabetically in the manifest while you're cleaning things up in there? Makes it easier to work in when tests can be easily found :)

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> I'd love to run these through Cedar too where we have the browser-chrome
> tests running in chunks on all platforms and the devtools tests split out
> into mochitest-dt. Of particular note, styleinspector tests are the most
> frequent oranges we still see in the mochitest-dt runs. However, there's way
> too much rebasing needed for the current patches (mostly in head.js) for me
> to do it myself.
I'll do the rebasing. I've moved around so much code that it'd be very hard for someone else to do that.
> BTW, I don't suppose I could talk you into sorting the test names
> alphabetically in the manifest while you're cleaning things up in there?
> Makes it easier to work in when tests can be easily found :)
I've got a 3rd patch coming up that renames the tests and sort the browser.ini file alphabetically. Will be attaching it shortly.
For now I'm not ready to check this in since one of the test I rewrote is failing constantly on try on all platforms, but not locally, and I haven't yet found a fix for that.
I will be updating this bug this week.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug988313-1-cleanup-styleinspector-tests v3.patch

Just a few minor changes since last R+. Trying to make the keybinding test not fail on try. I haven't had luck so far, but I haven't spent too much time on this either.
I will attach 2 more patches.

Comment 9

[Patrick Brosset <:pbro>]

Attachment: bug988313-2-splitting-tests v2.patch

Comment 10

[Patrick Brosset <:pbro>]

Attachment: bug988313-3-renaming-tests.patch

Mike, this is the 3rd and last patch in this bug. This one renames all files so it's more consistent with what we now have in the markup-view tests.

Comment 11

[Patrick Brosset <:pbro>]

And the try build for this new patch series is: https://tbpl.mozilla.org/?tree=Try&rev=4b714b627745

Comment 12

[Patrick Brosset <:pbro>]

Attachment: bug988313-1-cleanup-styleinspector-tests v4.patch

Thanks to Paolo who pointed out to the screenshot in the try build log, I was able to fix the keybinding test.
Here's a new patch for it.
And a new try build: https://tbpl.mozilla.org/?tree=Try&rev=c82752f4e666

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8402560 [details] [diff] [review]
bug988313-3-renaming-tests.patch

Review of attachment 8402560 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming green try

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Running on cedar too. So far so good.
https://tbpl.mozilla.org/?tree=Cedar&jobname=mochitest-devtools&fromchange=c27138be046a&tochange=2b33b2e84bab

Comment 15

[Patrick Brosset <:pbro>]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> Running on cedar too. So far so good.
> https://tbpl.mozilla.org/?tree=Cedar&jobname=mochitest-
> devtools&fromchange=c27138be046a&tochange=2b33b2e84bab
One of the tests (brower_computedview_keybinding_01.js) will be failing in this try push, can you push again with the last patches? (Attachment #8402632 [details] [diff] replaced Attachment #8402558 [details] [diff] in my comment 12)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

See the top push on that link ;)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Cedar is liking these patches a lot. Please push to fx-team (or set checkin-needed) whenever you're happy with them :)

Comment 18

[Patrick Brosset <:pbro>]

Attachment: bug988313-1-cleanup-styleinspector-tests v5.patch

Another rebase was needed with an incoming change in the styleinspector tests that landed today.

Comment 19

[Patrick Brosset <:pbro>]

Attachment: bug988313-2-splitting-tests v3.patch

Comment 20

[Patrick Brosset <:pbro>]

Attachment: bug988313-3-renaming-tests v2.patch

rebased too

Comment 21

[Patrick Brosset <:pbro>]

Fixed in fx-team. Let's see if that sticks.
Looks pretty good on cedar: https://tbpl.mozilla.org/?tree=Cedar&jobname=mochitest.*chrome&rev=2b33b2e84bab

https://hg.mozilla.org/integration/fx-team/rev/070988f93458
https://hg.mozilla.org/integration/fx-team/rev/13c7239294aa
https://hg.mozilla.org/integration/fx-team/rev/ebcad2bf9017

Comment 22

[Phil Ringnalda (:philor)]

Well, if it was easy and pleasant, someone else would have already done it :(

Your orange: browser_tabview_bug654295.js leaking until shutdown in bc2, browser_ruleview_refresh-on-attribute-change_01.js timing out rather quite a lot in bc3

After RyanVM's https://hg.mozilla.org/integration/fx-team/rev/bb8e0a572620 disabling browser_tabview_bug654295.js: browser_tabview_bug654721.js leaking until shutdown in bc2, browser_ruleview_refresh-on-attribute-change_01.js timing out rather quite a lot in bc3 (and a bonus one-off exceeding the maxtime of 4800 seconds in bc3, so you might have shoved a bit of extra time out into 3)

Backed out all four in https://hg.mozilla.org/integration/fx-team/rev/ecfe03732faf

Comment 23

[Phil Ringnalda (:philor)]

Oh, and because Windows always has to get involved, there was one instance of browser_ruleview_refresh-on-attribute-change_01.js timing out in Win7 debug bc.

Comment 24

[:Paolo Amadini]

Maybe the code of browser_ruleview_refresh-on-attribute-change_01.js can be fixed easily, but if we're trading 19 ultra-high-frequency intermittent failures (see bug 992211) for 3 tests disabled on a few identified platforms, I'd definitely go for landing the patch as is and disabling the tests.

I'd prefer this solution rather than aim for perfection, and thus delay the important Task.jsm conversion to Promise.jsm (see bug 992223) that we want to complete in this release cycle and blocks other improvements in turn.

Comment 25

[Patrick Brosset <:pbro>]

(In reply to :Paolo Amadini from comment #24)
> Maybe the code of browser_ruleview_refresh-on-attribute-change_01.js can be
> fixed easily, but if we're trading 19 ultra-high-frequency intermittent
> failures (see bug 992211) for 3 tests disabled on a few identified
> platforms, I'd definitely go for landing the patch as is and disabling the
> tests.
> 
> I'd prefer this solution rather than aim for perfection, and thus delay the
> important Task.jsm conversion to Promise.jsm (see bug 992223) that we want
> to complete in this release cycle and blocks other improvements in turn.
I've made a fix that I hope will take care of the failure in browser_ruleview_refresh-on-attribute-change_01.js (which of course doesn't happen locally, and only happens on Linux on try, for which failure screenshots are useless).
I'm waiting for try results now: https://tbpl.mozilla.org/?tree=Try&rev=e347d71c777d
(seems like my last pull/merge caught a nasty bug that is making "oth" tests fail, but that doesn't seem related to my patches).

Comment 26

[Patrick Brosset <:pbro>]

Alright, new try build: https://tbpl.mozilla.org/?tree=Try&rev=3511af741d82

Comment 27

[Patrick Brosset <:pbro>]

The try push in comment 26 is shaping up to be pretty green. There's still the tabview test leak being reported.
I understand that it's indirectly caused by my patches because I've changed the number of tests being run and therefore, with the simple test chunking mechanism in place today, it basically changes the list of which tests run together, and that caused the leak to appear.
The leak is being worked on in bug 989083.
Here's a new try push with the patch from bug 989083 applied on top of my patches: https://tbpl.mozilla.org/?tree=Try&rev=fb5945e8884d

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

sounds like a case of 10 steps forward, 1 step back

Comment 29

[Patrick Brosset <:pbro>]

The patch for bug 989083 seems to fix the leak in bc2.
Just waiting for it to land and then landing the patches again.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/be0436eaf21d
https://hg.mozilla.org/integration/fx-team/rev/aec22b0da647
https://hg.mozilla.org/integration/fx-team/rev/0336c7f72b03

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be0436eaf21d
https://hg.mozilla.org/mozilla-central/rev/aec22b0da647
https://hg.mozilla.org/mozilla-central/rev/0336c7f72b03

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ba4be334346
https://hg.mozilla.org/releases/mozilla-aurora/rev/9e6e3bfe9e99
https://hg.mozilla.org/releases/mozilla-aurora/rev/508f43d65a23