968727 - Clean up the markup-view tests

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

I'd like to propose a change to the way the inspector tests are written/organized:

- naming convention for the test files: is not consistent at all and file names are often very very long. We should just agree on one way to name them

- test files length: varies a great deal. Some test files only test one single usecase, others several. It would help a lot to split the bigger files into single testcases that can be maintained more easily

- common code: is duplicated. Because we have folders for each of the inspector's tools (markup, rule, computed, font, layout), we are not sharing their head.js common functions, and within one folder, a lot of tests are duplicating common things. I propose to regroup all tests into browser/devtools/inspector/test, or find a way to share head.js but I don't think that's possible.

- start test pattern: is duplicated all over the place. We almost always open a new tab, load a URL, open the inspector. That should be put into a single function that returns a promise

- async test patterns: varies a great deal too. We have nested callbacks here and there, nested promises in other places, and some Task.spawn in yet other places. I'd like to propose that we always use something like:

  Task.spawn(function() {
    let inspector = yield openInspector(url);
    yield doSomething();
    yield doSomethingElse();
  }).then(null, ok.bind(null, false)).then(endTests);

Comment 1

[Patrick Brosset <:pbro>]

we have, as of today, 45 style-inspector tests, 1 layout-view test, 1 font-inspector test, 14 markup-view tests and 38 inspector tests = 99 tests.
The first step could be to quickly go through all and decide what is common and needs to be extracted to head.js
Then refactor head.js to have these nice, common, utilities (returning promises).

For info, Victor did something similar with the debugger tests (they have more than 200 files) and it's a pleasure to create new debugger tests now.

Comment 2

[Joe Walker [:jwalker]]

+1 and I think we should consider creating a top level head.js. Many of the functions in debugger/head.js are good for all modules.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: bug968727-1-clean-up-all-markupview-tests.patch

This patch is a refactor of all the markupview tests.
It's a relatively big patch because all tests have been rewrote *but* there's no functional changes made to the tests.
What has been done is:
- all tests now are consistently written
- they all use Task.spawn to describe the steps in a nice, flat, manner
- head.js has been augmented with more functions to further reduce each test size
- many js exceptions which were thrown during tests have been removed

2 changes worth noting too:
- a fix in rule-view.js to avoid throwing errors if the current node changes before a promise resolves
- removed a skip-if condition in browser.ini. I'm hoping that rewriting the test will fix the intermittent. I will confirm with the try build.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug968727-2-rename-all-markupview-tests.patch

This is part 2 of the markupview tests rewrite:
- this part renames all files to something more consistent and shorter
- also, the support files now all start with "doc_" to visually separate them

Comment 5

[Patrick Brosset <:pbro>]

Ongoing try build for the 2 previous patches: https://tbpl.mozilla.org/?tree=Try&rev=a1245e59418f

Comment 6

[Patrick Brosset <:pbro>]

The try build is almost entirely green except for unrelated intermittent failures and orange factors, AND the skip-if that I removed seems to be still failing on OSX10.6 Opt.
So, the test refactoring didn't break anything, but didn't fix the intermittent either, so I'll need to skip-if it again and leave this another bug.

Comment 7

[Patrick Brosset <:pbro>]

Also, the new tests use implicit generator functions, which should be changed to using function*().

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug968727-1-clean-up-all-markupview-tests v2.patch

Added * in all the generator functions

Comment 9

[Patrick Brosset <:pbro>]

Attachment: bug968727-2-rename-all-markupview-tests v2.patch

Re adding the skip-if condition that I'd removed before since the try build shows that this test still fails on 10.6.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8379385 [details] [diff] [review]
bug968727-1-clean-up-all-markupview-tests v2.patch

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

I've just looked through the changes without running it.  The patch needs to be rebased for browser/devtools/markupview/test/head.js and browser/devtools/styleinspector/rule-view.js.  I'm sure it will need more changes once Bug 663778 lands, so no big deal.

::: browser/devtools/markupview/test/browser.ini
@@ -9,5 @@
>    browser_inspector_markup_950732.html
>    head.js
>  
>  [browser_bug896181_css_mixed_completion_new_attribute.js]
> -# Bug 916763 - too many intermittent failures

Does this refactor fix the intermittents?  Just making sure we won't introduce an issue with this.

::: browser/devtools/markupview/test/browser_bug896181_css_mixed_completion_new_attribute.js
@@ +64,4 @@
>  function test() {
> +  Task.spawn(function*() {
> +    info("Opening the inspector on the test URL");
> +    let args = yield addTab(TEST_URL).then(openInspector);

Just for consistency, you could use:
let {inspector} = yield addTab(TEST_URL).then(openInspector);

I've noted other places where this pattern is used too.

::: browser/devtools/markupview/test/browser_inspector_markup_950732.js
@@ +13,5 @@
>  
>  function test() {
> +  Task.spawn(function*() {
> +    yield addTab(TEST_URL);
> +    let {inspector} = yield openInspector();

let {inspector} = yield addTab(TEST_URL).then(openInspector);

::: browser/devtools/markupview/test/browser_inspector_markup_add_attributes.js
@@ +137,2 @@
>      info("Opening the inspector on the test URL");
>      let args = yield addTab(TEST_URL).then(openInspector);

let {inspector} = yield addTab(TEST_URL).then(openInspector);

::: browser/devtools/markupview/test/browser_inspector_markup_edit.js
@@ +18,5 @@
>   * underlying DOM, not that the UI updates - UI updates are based on
>   * underlying DOM changes, and the mutation tests should cover those cases.
>   */
>  
>  waitForExplicitFinish();

Can remove this waitForExplicitFinish

@@ +387,5 @@
> +    info("Opening a tab with the test URL");
> +    yield addTab(TEST_URL);
> +
> +    info("Opening the inspector panel");
> +    let args = yield openInspector();

let {inspector} = yield addTab(TEST_URL).then(openInspector);

::: browser/devtools/markupview/test/browser_inspector_markup_edit_2.js
@@ +5,5 @@
>  "use strict";
>  
>  // Tests that an existing attribute can be modified
>  
>  waitForExplicitFinish();

Can remove this waitForExplicitFinish()

::: browser/devtools/markupview/test/browser_inspector_markup_edit_4.js
@@ +5,5 @@
>  "use strict";
>  
>  // Tests that a node can be deleted from the markup-view with the delete key
>  
>  waitForExplicitFinish();

Can remove this waitForExplicitFinish()

::: browser/devtools/markupview/test/browser_inspector_markup_edit_outerhtml.js
@@ +141,4 @@
>  
>  function test() {
> +  Task.spawn(function*() {
> +    yield addTab(TEST_URL);

let {inspector} = yield addTab(TEST_URL).then(openInspector);

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8379390 [details] [diff] [review]
bug968727-2-rename-all-markupview-tests v2.patch

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

::: browser/devtools/markupview/test/browser.ini
@@ +10,3 @@
>    head.js
>  
> +# Bug 916763 - too many intermittent failures

Is this test intended to be disabled in this patch?

Comment 12

[Brian Grinstead [:bgrins]]

There is a frequent orange in Bug 970240 for browser_inspector_markup_edit_outerhtml.js.  Since this patch splits it up (and does some other cleanup) I've marked it as blocking.

Comment 13

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

We are working on moving our tests off of standalone hardware and onto ec2 slaves (which requires us to split into chunks for timing issues), this is causing some tests to flare up more often.

I am interested in this bug specifically because we see a lot of failures in:
browser_inspector_markup_edit_outerhtml.js 

Here is a sample log if needed:
https://tbpl.mozilla.org/php/getParsedLog.php?id=36382098&tree=Mozilla-Inbound

I am happy to see patches on this bug, are these close to landing?  If not, I can temporarily disable browser_inspector_markup_edit_outerhtml.js.

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Joel Maher (:jmaher) from comment #13)
> We are working on moving our tests off of standalone hardware and onto ec2
> slaves (which requires us to split into chunks for timing issues), this is
> causing some tests to flare up more often.
> 
> I am interested in this bug specifically because we see a lot of failures in:
> browser_inspector_markup_edit_outerhtml.js 
> 
> Here is a sample log if needed:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=36382098&tree=Mozilla-
> Inbound
> 
> I am happy to see patches on this bug, are these close to landing?  If not,
> I can temporarily disable browser_inspector_markup_edit_outerhtml.js.

Ah, I see the sample log is running on 'tst-linux32-spot-302', which I'm assuming is an EC2 slave.  Will a push to try run in this same environment?  This way we can make sure these patches are green even on the new systems.

Comment 15

[Patrick Brosset <:pbro>]

(In reply to Joel Maher (:jmaher) from comment #13)
> I am happy to see patches on this bug, are these close to landing?  If not,
> I can temporarily disable browser_inspector_markup_edit_outerhtml.js.
Unfortunately not at this stage. I think what I'll do is land multiple patches one by one keeping this bug open. That makes more sense with this bug.

Comment 16

[Patrick Brosset <:pbro>]

Attachment: bug968727-1-clean-up-all-markupview-tests.patch

I'm resuming work on this bug, now that the highlighter patch has landed, I have rebased my patches and cleaned up a few things thanks to Brian's review.

All markupview tests seem to be passing now and have a lot less code than before. Also, the code is a lot nicer with the use of tasks.

I haven't yet renamed test and support files to be more consistent. I will do that in a separate patch.

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=f212934b62f6

In my patch, I have re-activated test browser_bug896181_css_mixed_completion_new_attribute.js intentionally, because rewriting the test made it pass. I guess there must have been race conditions before because the test wasn't waiting for the right events.

I'm expecting this patch to intermittently fail on:
- devtools/markupview/test/browser_inspector_markup_edit_outerhtml.js
- devtools/markupview/test/browser_inspector_markup_edit.js
as these 2 are failing a lot these days (timeouts) since about a week I'd say. 
I will need to split these up in several smaller tests in a separate patch.

Comment 17

[Patrick Brosset <:pbro>]

Brian, I realize it's a bit of a big patch to review, but these are only test changes and I have made an effort not to change what the tests were actually testing. So I guess the review should be primarily based on what the tests look like now. And as long as the try build is green, we should be ok.

Comment 18

[Patrick Brosset <:pbro>]

Apart from the many test timeouts that are due to us moving to slower EC2 slaves (and that are not happening in markupview tests anyway), there are 2 failures of the mutation_flashing test in this try build https://tbpl.mozilla.org/?tree=Try&rev=89af3320099b
This test is relying on the inspector-updated event to assert that a node is flashing in the markup-view panel, which isn't great, there are many ways this can fail in a slower than usual environment.
I'm going to change that to listen to the markupmutation event instead, that should take care of this.

Comment 19

[Patrick Brosset <:pbro>]

Attachment: bug968727-1-clean-up-all-markupview-tests v2.patch

Brian, sorry for this last minute change, I hope you haven't started the review on this patch.
Anyway, this new version only has a minor change in the mutation_flashing test (as explained in my last comment).

I'm going to attach 4 more patches: 3 about splitting tests up to avoid timeouts, and 1 that renames all files for consistency reasons.
These patches should be quite quick to review therefore.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: bug968727-2-split-edit-outerhtml-tests.patch

Splits the outer-html edition test in several tests as it's been intermittently failing recently.

Comment 21

[Patrick Brosset <:pbro>]

Attachment: bug968727-3-rename-all-markupview-tests.patch

Renaming all files in markupview/test to be shorter, more consistent and self-explanatory (rather than things like browser_inspector_markupview_bug_123456.js)

Comment 22

[Patrick Brosset <:pbro>]

Attachment: bug968727-4-split-add-attributes-tests.patch

Split some more of the attributes add/edit tests.

Comment 23

[Patrick Brosset <:pbro>]

Attachment: bug968727-5-split-edit-attributes-tests.patch

And again, some more splitting of the add/edit attributes tests.

Comment 24

[Patrick Brosset <:pbro>]

And here is a try build for all 5 parts: https://tbpl.mozilla.org/?tree=Try&rev=ffc2fded82ab

Comment 25

[Patrick Brosset <:pbro>]

Interesting approach to load helper files into head.js and help with categorizing things better in extra smaller files: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/test/head.js#10

Comment 26

[Patrick Brosset <:pbro>]

Try build https://tbpl.mozilla.org/?tree=Try&rev=ffc2fded82ab bc results don't show failures in the new markupview tests so far.

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8395634 [details] [diff] [review]
bug968727-1-clean-up-all-markupview-tests v2.patch

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

Looks good to me

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8395636 [details] [diff] [review]
bug968727-2-split-edit-outerhtml-tests.patch

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

::: browser/devtools/markupview/test/browser.ini
@@ +16,5 @@
>  [browser_inspector_markup_edit_3.js]
>  [browser_inspector_markup_edit_4.js]
>  [browser_inspector_markup_add_attributes.js]
>  [browser_inspector_markup_edit_outerhtml.js]
> +[browser_inspector_markup_edit_outerhtml3.js]

Nit: keep these in alphabetical order

::: browser/devtools/markupview/test/head.js
@@ +315,5 @@
>    EventUtils.sendKey("return", inspector.panelWin);
>  }
> +
> +/**
> + * Run a series of edit-outer-html tests.

As discussed, I'm not a huge fan of moving this test specific logic into head.js.  I think if we could load via a helper file as you mention in Comment 25 this could be a better solution.  I'm OK with doing this now, or as a follow up

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8395638 [details] [diff] [review]
bug968727-4-split-add-attributes-tests.patch

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

Same as Comment 28

Comment 30

[Brian Grinstead [:bgrins]]

Comment on attachment 8395639 [details] [diff] [review]
bug968727-5-split-edit-attributes-tests.patch

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

Same as Comment 28

Comment 31

[Patrick Brosset <:pbro>]

Thanks Brian for the reviews.
Try build doesn't show any markupview test failures, so I guess we're good to land this first set of patches.

I'm going to work on extracting the test runner parts into helper files in a subsequent patch and leave this bug open (in any case it needs to stay open for working on the styleinspector, fontinspector, ... tests)

Comment 32

[Patrick Brosset <:pbro>]

Attachment: bug968727-3-rename-all-markupview-tests v2.patch

Just re-ordered tests in browser.ini by alphabetical order

Comment 33

[Patrick Brosset <:pbro>]

Attachment: bug968727-4-split-add-attributes-tests v2.patch

Just re-ordered tests in browser.ini by alphabetical order

Comment 34

[Patrick Brosset <:pbro>]

Attachment: bug968727-5-split-edit-attributes-tests v2.patch

Just re-ordered tests in browser.ini by alphabetical order

Comment 35

[Patrick Brosset <:pbro>]

Fixed in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/b2a7a334bd0a
https://hg.mozilla.org/integration/fx-team/rev/f5b9c7d28c53
https://hg.mozilla.org/integration/fx-team/rev/67965b9fa074
https://hg.mozilla.org/integration/fx-team/rev/80fb6397af45
https://hg.mozilla.org/integration/fx-team/rev/5308c7eee7a0

Comment 36

[Patrick Brosset <:pbro>]

Attachment: bug968727-6-extract-test-runner-helpers.patch

One more review Brian, then I promise to stop (at least for a while).

This patch contains (apart from a few minor license and use strict clean-ups) 2 new helper files that contain the add/edit attributes and edit outerhtml test runners.
They're loaded only by the tests that actually need them.
I thought of loading them in head.js, but they actually depend on functions defined in head.js so, if we want to do that, the load call would have to remain always at the bottom of the file, which is a little bit strange.

Comment 37

[Brian Grinstead [:bgrins]]

Comment on attachment 8396245 [details] [diff] [review]
bug968727-6-extract-test-runner-helpers.patch

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

I like these changes.  Made a minor note about making the helper loading easier from the test script, but up to you if you want to change it.

::: browser/devtools/markupview/test/browser_markupview_html_edit_01.js
@@ +3,5 @@
>   http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +"use strict";
> +
> +// Import the markupview outerhtml test runner

Could we have a loadHelperScript function in head.js that takes in "helper_outerhtml_test_runner.js" and runs these two lines?

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b2a7a334bd0a
https://hg.mozilla.org/mozilla-central/rev/f5b9c7d28c53
https://hg.mozilla.org/mozilla-central/rev/67965b9fa074
https://hg.mozilla.org/mozilla-central/rev/80fb6397af45
https://hg.mozilla.org/mozilla-central/rev/5308c7eee7a0

Comment 39

[Patrick Brosset <:pbro>]

Attachment: bug968727-6-extract-test-runner-helpers v2.patch

(In reply to Brian Grinstead [:bgrins] from comment #37)
> Comment on attachment 8396245 [details] [diff] [review]
> bug968727-6-extract-test-runner-helpers.patch
> 
> Review of attachment 8396245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like these changes.  Made a minor note about making the helper loading
> easier from the test script, but up to you if you want to change it.
> 
> ::: browser/devtools/markupview/test/browser_markupview_html_edit_01.js
> @@ +3,5 @@
> >   http://creativecommons.org/publicdomain/zero/1.0/ */
> >  
> > +"use strict";
> > +
> > +// Import the markupview outerhtml test runner
> 
> Could we have a loadHelperScript function in head.js that takes in
> "helper_outerhtml_test_runner.js" and runs these two lines?
Done in this new patch.
Thanks for the review.

Btw, I forgot to paste in the URL to the try build I launched: https://tbpl.mozilla.org/?tree=Try&rev=8f269d0ee415

Comment 40

[Patrick Brosset <:pbro>]

Part 6 patch is fixed in fx-team now: https://hg.mozilla.org/integration/fx-team/rev/8b8e48c3b387
Still keeping this bug open to work on the style-inspector tests.
These are the candidates next in line, and they need a lot of work especially right now with all the timeout intermittents we're having.
I have a big patch ready already, it will need rebasing and I'll try and align it as much as possible with what we now have for the markupview.

Comment 41

[Patrick Brosset <:pbro>]

Going to open other bugs for the style-inspector, font-inspector, layout-view and inspector.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b8e48c3b387

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bbdc0136061e
https://hg.mozilla.org/releases/mozilla-aurora/rev/329ad4441fd9
https://hg.mozilla.org/releases/mozilla-aurora/rev/d4c886f9516a
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca1a77077c8b
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c267ebca47d
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1a02bbf28e9