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

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.

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

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

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 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!

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

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

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.

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.

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

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 on attachment 8402560 [details] [diff] [review] bug988313-3-renaming-tests.patch Review of attachment 8402560 [details] [diff] [review]: ----------------------------------------------------------------- r+ assuming green try

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

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

See the top push on that link ;)

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

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

rebased too

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

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

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.

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.

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

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

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

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

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

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

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

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