1223527 - [markup-view] Migrate markup-view tests to use testActor

Status: RESOLVED DUPLICATE of bug 1252099

(DevTools :: Inspector, defect)

Description

[Grisha Pushkov]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151029004130

Steps to reproduce:

Bug 1093593 introduces usage of testActor for working with DOM.
In this bug we want to replace all content.document occurrences with testActor's approach.

Files in client/markupview/test directory will be modified.

As follow up we can also:
* Migrate client/markupview/test/head.js from promises to generators
* Add category to filenames (e.g. browser_markupview_tag_edit_03-tag.js, where "tag" is a category)

Comment 1

[Jon Droniak]

I would like to work on this, but don't really know where to start. Can you give an example of testActor and how it differs from the current method?

Comment 2

[Grisha Pushkov]

No-no, this is my bug :)

About testActor. You can find different methods in client/shared/test/test-actor.js:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/test/test-actor.js

If you grep devtools tests for document.content, you will see various ways of working with DOM. As I understand, test runs in different process and document.content is not available and wrapped by CPOWs (cross-process object wrappers). Difference between document.element and testActor usage could be inconspicuous, the main problem is that testActor doesn't have all methods which DOMNode has.

Patrick wrote about it in bug 994555 comment 31 and bug 1093593 comment 13.

Comment 3

[Patrick Brosset <:pbro>]

Before multi-process Firefox existed (e10s) the devtools mochitests would run in the same process as the content page loaded in the test tab. Now it isn't the case anymore as content pages run in a "content" process while the test (and the devtools toolbox) run in a parent "chrome" process.
As Grisha said, that isn't so much of a problem if you're willing to use CPOWs: a black magic mechanism that allows to access the content page from the test just like before.
But, we're in the process of trying to get rid of CPOWs usage *and* we're also in the process of trying to run our tests remotely, on remote devices. In the latter case, CPOWs won't help us, because the test and content page not only run in 2 separate processes but on 2 separate machines.

This is why we introduced the testActor for /devtools/client/inspector/test/ tests. The testActor is just a custom devtools actor (something that we can call async methods on and that lives in the process of the test page) that tests should use to access the content page, retrieve information about the DOM, modify it, etc.

The goal of this bug is to do the same for /devtools/client/markup-view/test/
Now, the hard part has already been done, the testActor exists and we have a mechanism to load it before the test starts (bug 1137285).
So, in this bug, we should find out all CPOW usages in the markup-view tests and replace those with calls to the testActor. 
This means, in a lot of cases, making a previously sync test async. But fortunately, most (all?) of our tests already run in tasks (within generator functions that can yield promises).

For info, bug 1218412 introduces a new ESLint rule that can find CPOW occurrences in tests, this could be helpful for this bug.

CC'ing Alex who has introduced the testActor in case he has things we should know about.

Comment 4

[Alexandre Poirot [:ochameau]]

Note that while doing this refactoring, you shouldn't introduce a new test-actor method for every possible API of DOMNode. Look carefuly at existing ones to see if you could reuse one and do not hesitate to use `eval` method for stuff that is done just once or twice.

Comment 5

[Grisha Pushkov]

Thanks for assigning this bug to me. Currently it's work in progress, but I already stuck with a problem.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/markupview/test/browser_markupview_keybindings_04.js#61-63:
> let contentAreaContextMenu = document.querySelector("#contentAreaContextMenu");
> let contextMenu = new nsContextMenu(contentAreaContextMenu);
> yield contextMenu.inspectNode();

If I change document to using walker:
> let contentAreaContextMenu =
>   yield walker.querySelector(walker.rootNode, "#contentAreaContextMenu");
> let contextMenu = new nsContextMenu(contentAreaContextMenu);
> yield contextMenu.inspectNode();

I get that contentAreaContextMenu is undefined and there is no aXulMenu property.
Already understand that testActor.eval is executed in every-time-newly-created-sandboxed-environment. There is something similar for DOM?

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to Grisha Pushkov from comment #5)
> Thanks for assigning this bug to me. Currently it's work in progress, but I
> already stuck with a problem.
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/markupview/
> test/browser_markupview_keybindings_04.js#61-63:
> > let contentAreaContextMenu = document.querySelector("#contentAreaContextMenu");
> > let contextMenu = new nsContextMenu(contentAreaContextMenu);
> > yield contextMenu.inspectNode();
> 
> If I change document to using walker:
> > let contentAreaContextMenu =
> >   yield walker.querySelector(walker.rootNode, "#contentAreaContextMenu");
> > let contextMenu = new nsContextMenu(contentAreaContextMenu);
> > yield contextMenu.inspectNode();
> 
> I get that contentAreaContextMenu is undefined and there is no aXulMenu
> property.
> Already understand that testActor.eval is executed in
> every-time-newly-created-sandboxed-environment. There is something similar
> for DOM?

That's because #contentAreaContextMenu isn't in the content document, but in the browser chrome document.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#379
This is the XUL context menu displayed when you right click in a web page.

The test actor, as the walker actor are targetting the content document, the web page.

I think there isn't much value to convert this particular test.
You can still convert the rest of the test, for this particular lines can't be converted:
> let contentAreaContextMenu = document.querySelector("#contentAreaContextMenu");
> let contextMenu = new nsContextMenu(contentAreaContextMenu);
> yield contextMenu.inspectNode();

You should look for `content` usages. This targets the content document. Direct usages of `document` and `window` are for the browser chrome document for which there is no need to do anything.

Comment 7

[Grisha Pushkov]

Attachment: 1223527-migrate-markup-view-to-testActor.patch

Most of the work is done. I'm going to comment on some parts of the code.
Test summary comparison for tests running with and without this patch:
> --- refactored ---
> Browser Chrome Test Summary
>	Passed: 1776
> 	Failed: 0
>	Todo: 0
> *** End BrowserChrome Test Results ***
> SUITE-END | took 116s
>
> --- original ---
> Browser Chrome Test Summary
>	Passed: 1776
>	Failed: 0
>	Todo: 0
> *** End BrowserChrome Test Results ***
> SUITE-END | took 115s

Important that numbers for "Passed" are equal.

Next I want to do migrate head.js to generators and bring all files to error-free ESLint state. Will upload new patch soon.

Comment 8

[Grisha Pushkov]

I commented on patch, but new comment with these comments isn't created. It is saved as draft, don't know if somebody is able to see it.

Comment 9

[Grisha Pushkov]

Looking at inspector/test/head.js and markupview/test/head.js, there are so much in common (e.g. addTab, openInspector, selectAndHighlightNode and so on). These helpers do the same thing, but with (quite) different implementation.

Thinking about doing "migrate head.js to generators" in new bug with name "unify markupview's and inspector's head.js", because this bug is already big enough.

Also found duplication in storage/test/head.js, layoutview/test/head.js. Or these helpers were separated intentionally?

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Grisha Pushkov from comment #9)
> Looking at inspector/test/head.js and markupview/test/head.js, there are so
> much in common (e.g. addTab, openInspector, selectAndHighlightNode and so
> on). These helpers do the same thing, but with (quite) different
> implementation.
> 
> Thinking about doing "migrate head.js to generators" in new bug with name
> "unify markupview's and inspector's head.js", because this bug is already
> big enough.
> 
> Also found duplication in storage/test/head.js, layoutview/test/head.js. Or
> these helpers were separated intentionally?
Yes, our head.js have a tone of code duplication. We've started to fix this slowly, bit by bit, in bug 1181833.
Yes please, do not work on this here, as you said, the bug is already big enough. Take a look at the blockers of bug 1181833.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8692633 [details] [diff] [review]
1223527-migrate-markup-view-to-testActor.patch

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

Great work. Thanks Grisha.
I have only gone quickly over the changes because this was just a feedback request. I'd spend more time for a review.
In particular, I have seen things in browser_markupview_keybindings_04.js and browser_markupview_mutation_01.js that we'll discussed during the review.
Other than this, I did make a few comments (see below), but wanted to thank you for all your hard work. Especially refactoring a large number of tests! That's always very very appreciated.

::: devtools/client/markupview/test/browser_markupview_html_edit_02.js
@@ +77,5 @@
> +      ] = [
> +        yield testActor.eval(querySelector + "').textContent"),
> +        yield testActor.eval(querySelector + "').tagName"),
> +        yield testActor.eval(querySelector + " p').textContent"),
> +        yield testActor.eval(querySelector + " p').tagName"),

I'm really not sure that this formatting and the querySelector variable help reading this portion of the test.
Even if more verbose, I think this just reads easier:

let divTextContent = yield testActor.eval(
  "document.querySelector('#badMarkup4').textContent");
let divTagName = yield testActor.eval(
  "document.querySelector('#badMarkup4').tagName");
...

You can also create a little helper function that would help make this less verbose/even easier to read:

function* getTextContent(selector, testActor) {
  return yield testActor.eval(
    `document.querySelector("${selector}").textContent`);
}

function* getTagName(selector, testActor) {
  return yield testActor.eval(
    `document.querySelector("${selector}").tagName`);
}

@@ +106,5 @@
> +        yield testActor.eval(querySelector + " div')"),
> +        yield testActor.eval(querySelector + "').tagName"),
> +        yield testActor.eval(querySelector + "').textContent"),
> +        yield testActor.eval(querySelector + " ~ div').tagName"),
> +        yield testActor.eval(querySelector + " ~ div').textContent")

Same comment here, I find that a bit hard to read.

::: devtools/client/markupview/test/browser_markupview_tag_edit_05.js
@@ +71,5 @@
>    }
>  }];
>  
>  add_task(function*() {
> +  let {inspector, testActor} = yield addTab(TEST_URL).then(openInspector);

testActor isn't used here, no need to define it in the destructuring assignment.
let {inspector} = yield addTab(TEST_URL).then(openInspector);

::: devtools/client/markupview/test/helper_outerhtml_test_runner.js
@@ +11,5 @@
>   * a few things. Each test may also provide its own validate function to perform
>   * assertions and verify that the new outer html is correct.
>   * @param {Array} tests See runEditOuterHTMLTest for the structure
>   * @param {InspectorPanel} inspector The instance of InspectorPanel currently
>   * opened

Please add the @param doc for testActor

@@ +33,5 @@
>   *        - newHTML {String}
>   *        - validate {Function} will be executed when the edition test is done,
>   *        after the new outer-html has been inserted. Should be used to verify
>   *        the actual DOM, see if it corresponds to the newHTML string provided
>   * @param {InspectorPanel} inspector The instance of InspectorPanel currently

Please add the @param doc for testActor

::: devtools/client/shared/test/test-actor.js
@@ +3,5 @@
>   http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  
> +// A helper actor for inspector and markupview tests.

nit: No need to mention inspector and markupview here. True it's only used there, but we want this to become a shared test helper, not just for these two.
So please expand a bit on this comment, explain what this actor is, why we have it, and what we expect to add to it over time.
Note that I really like the way you chose to balance which methods you added to the actor vs. using eval in tests.
I think this should be described here too.
Basically, I think this comment should be a sort of summary of comment 3 and comment 4.

Comment 12

[Grisha Pushkov]

Attachment: 1223527-migrate-markup-view-to-testActor.patch

In this patch changes for comment above and bunch of style fixes.
> without this patch ESLint shows (excluding head.js):
> ✖ 456 problems (65 errors, 391 warnings)
> and with patch: 
> ✖ 91 problems (10 errors, 81 warnings)
It will be easier to notice lint errors, because most of the files are error-free now. 

> I'm really not sure that this formatting and the querySelector variable help reading this portion of the test.
Agree. It was compromise for readability and 80 line length. I replaced it with testActor.getProperty.

> Please add the @param doc for testActor ✔️
But I didn't write description for it. It is used in every markup-view's test, so I think it's known as inspector, for example.
And descriptions like 'selector The selector for node' were removed, because they are self-explanatory already.

> So please expand a bit on this comment ✔️

As I understand, my comments for previous patch are visible only for me. I'm going to paste lines (without filename and line number, but hope it's all right) and comment right after. But it would be convenient to have an ability to leave comment right on review. Maybe mozreview allows that?

> +    "max-len": [1, 80, 4, {"ignorePattern": ".{100,}"}],
With this change ESLint ignores lines with 100 and more characters. Helpful for inlined images, where we can't do anything.

> -  return Promise.all([onMutated, onModified]);
> +  yield onMutation;
Dont't understand why we need to wait on two events at once. I checked that "markupmutation" is fired after "modifyattributes", so I keep "markupmutation".

> -  return Promise.all([onUpdated, onNodeHighlighted]);
> +  yield onUpdated;
Here "inspector-updated" is fired last. 

> +  is((yield testActor.hasNode("#executed")), false,
> +    "Script has not been executed");
window.foo was used before, but with testActor eval is sandboxed and every execution has newly created window. That's why "executed" id is toggled on body.

> +    document.popupNode = content.document.querySelector("div");
> +  } catch (e) {
This is reverted from previous testActor usage. Because document != content.document.
I don't get what "testing on remote devices" is, only guess that we need to disable this test on luciddream later.

> +    numMutations: 5,
> +    test: function*(testActor) {
> +      yield testActor.addClass("#node1", "pseudo");
numMutation = 2, as it was before, is not enough and markup change doesn't happen. I tried to set numMutation to 1000 and it reached this number! While-true-loop (of course no)? We need to add event in markup-view.js for pseudo-elements mutation? Because even with numMutations = 5 I see failures from time to time.

>     * Get a JS property on a DOM Node.
> +   * Remove attribute from DOM Node.
> +   * Add a class to a DOM Node.
I'm little bit confused with the articles :)

In markup-view's head.js I found ​executeInContent, and functions like setNodeAttribute uses it.
​executeInContent seems to be e10s compatible, but shouldn't we replace setNodeAttribute with testActor.setAttribute?

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Grisha Pushkov from comment #12)
> In markup-view's head.js I found ​executeInContent, and functions like
> setNodeAttribute uses it.
> ​executeInContent seems to be e10s compatible, but shouldn't we replace
> setNodeAttribute with testActor.setAttribute?
Yes, executeInContent was introduced in the previous test refactor that aimed at making tests e10s compatible. Before this, we were just accessing the DOM directly. Since this isn't possible with e10s, we introduced this message-passing mechanism wrapped in executeInContent.
Since you're introducing the testActor now here, we shouldn't use executeInContent anymore.

Comment 14

[Grisha Pushkov]

> Since you're introducing the testActor now here, we shouldn't use executeInContent anymore.
Now there is bug 1229558 for tracking migration to testActor for all components.
And in this bug I will replace frame-scripts with testActor. Is it going the right way?

Comment 15

[Grisha Pushkov]

And also will write at the top of frame-script-utils.js that it's usage is deprecated.

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Grisha Pushkov from comment #14)
> > Since you're introducing the testActor now here, we shouldn't use executeInContent anymore.
> Now there is bug 1229558 for tracking migration to testActor for all
> components.
> And in this bug I will replace frame-scripts with testActor. Is it going the
> right way?
Yes it is, thanks!
Having said this, it's still a long way ahead before we can actually run all of our tests remotely.
So there's no rush with converting all tests to using the testActor. If you do decide to start converting other tests, make sure you ping the relevant people about it as it's quite a big change.

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8693275 [details] [diff] [review]
1223527-migrate-markup-view-to-testActor.patch

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

Looks great. I have a few comments, nothing really serious though.

Can you please change the commit message to something like:
Bug 1223527 - Migrate markup-view tests to use testActor and fix eslint errors

(about my comments regarding unrelated changes, you could also simply move them into a second, separate, commit with an appropriate commit message).

::: devtools/.eslintrc
@@ +95,5 @@
>      // Don't enforce the maximum depth that blocks can be nested. The complexity
>      // rule is a better rule to check this.
>      "max-depth": 0,
>      // Maximum length of a line.
> +    "max-len": [1, 80, 4, {"ignorePattern": ".{100,}"}],

I'm not comfortable with this change because that means people used to coding for 120 chars (which is pretty common) will see no warnings in many places.

So I'd advised to only ignore if the line is longer than 120, or maybe let's just start simple and only ignore lines that contain this pattern: data:image/png because it seems to be the only case so far that we really want to ignore.

::: devtools/.eslintrc.mochitests
@@ +62,5 @@
> +    "undoChange": true,
> +    "unregisterActor": true,
> +    "wait": true,
> +    "waitForChildrenUpdated": true,
> +    "waitForMultipleChildrenUpdates": true,

Please remove these new globals. We have a custom eslint plugins that should take care of this.
The way it works is whenever you lint a test file, it tries to find the nearest head.js (as well as the shared one I think), and imports all globals from there so that eslint knows they're not undefined.
This was done specifically to avoid having to manually maintain this list of globals as you just did.

This custom rule is import-headjs-globals: https://dxr.mozilla.org/mozilla-central/source/testing/eslint-plugin-mozilla/lib/rules/import-headjs-globals.js

Make sure you install it by first running './mach eslint --setup'.
If this does not work as it should, please report a bug.

::: devtools/client/markupview/test/.eslintrc
@@ +20,5 @@
> +    "setEditableFieldValue": true,
> +    "setNodeAttribute": true,
> +    "simulateNodeDrag": true,
> +    "simulateNodeDragAndDrop": true,
> +    "simulateNodeDrop": true,

Same comment here, we do not need these globals to be manually defined. The eslint rule should do it automatically.

::: devtools/client/markupview/test/browser_markupview_dragdrop_reorder.js
@@ +84,5 @@
>  
>    let container = yield getContainerForSelector(next, inspector);
>    let height = container.tagLine.getBoundingClientRect().height;
>  
> +  let onMutation = inspector.once("markupmutation");

Your patch is already quite big as it addresses 2 big things: reducing the number of eslint errors, and using the test-actor.
So let's try and stick to doing those 2 things only and avoid unrelated change.
I know this seems minor, but it's important to keep a clean enough mercurial history, and this particular change does not seem like it's addressing either of those 2 things.

::: devtools/client/markupview/test/browser_markupview_html_edit_03.js
@@ +102,5 @@
>  
> +  // Reselect nodeFront because markup was changed.
> +  nodeFront = yield getNodeFront(SELECTOR, inspector);
> +  ok(!inspector.markup.htmlEditor._visible, "HTML Editor is not visible");
> +  is((yield inspector.markup.getNodeOuterHTML(nodeFront)), NEW_HTML,

I think we should be using the testActor to get the node's outerHTML here.
We want to make sure that the actual node got the right HTML put into it. Using the markup-view and the walker to do this might hide bugs where they *think* the HTML has been set but really has not.

This used to be done with getNode(SELECTOR).outerHTML, so should be changed to the equivalent using the testActor.

@@ +120,5 @@
>    yield onReselected;
>  
> +  // Reselect bodyFront because markup was changed.
> +  bodyFront = yield getNodeFront("body", inspector);
> +  is((yield inspector.markup.getNodeOuterHTML(bodyFront)), bodyHTML,

Same comment here.

@@ +122,5 @@
> +  // Reselect bodyFront because markup was changed.
> +  bodyFront = yield getNodeFront("body", inspector);
> +  is((yield inspector.markup.getNodeOuterHTML(bodyFront)), bodyHTML,
> +    "<body> HTML has been updated");
> +  is((yield walker.querySelectorAll(walker.rootNode, "head")).length, 1,

Same comment for checking the number of <head> elements, please use the testActor to get that info.

@@ +149,5 @@
> +    "Script has not been executed");
> +
> +  // Reselect headFront because markup was changed.
> +  headFront = yield getNodeFront("head", inspector);
> +  is((yield inspector.markup.getNodeOuterHTML(headFront)), headHTML,

Same here.

@@ +151,5 @@
> +  // Reselect headFront because markup was changed.
> +  headFront = yield getNodeFront("head", inspector);
> +  is((yield inspector.markup.getNodeOuterHTML(headFront)), headHTML,
> +    "<head> HTML has been updated");
> +  is((yield walker.querySelectorAll(walker.rootNode, "body")).length, 1,

And here.

@@ +185,5 @@
> +  is((yield inspector.markup.getNodeOuterHTML(docElementFront)), docElementHTML,
> +    "<head> HTML has been updated");
> +  is((yield walker.querySelectorAll(walker.rootNode, "head")).length, 1,
> +    "no extra <heads>s have been added");
> +  is((yield walker.querySelectorAll(walker.rootNode, "body")).length, 1,

And same comment for the last 3 is()

::: devtools/client/markupview/test/browser_markupview_image_tooltip_mutations.js
@@ +62,4 @@
>      attributeName: "src",
>      newValue: newSrc
>    }]);
> +  yield onMutation;

I'm wondering if this test doesn't throw an exception. The image element is selected in the inspector, and we're mutating it here, so in theory, we should have to also wait for the inspector-updated event to prevent hanging requests when the test ends.

::: devtools/client/markupview/test/browser_markupview_keybindings_04.js
@@ +42,3 @@
>    let onUpdated = inspector.once("inspector-updated");
>    EventUtils.synthesizeKey("VK_UP", {});
> +  yield onUpdated;

Why are you not waiting for the node-highlight event anymore?
I think it races with the inspector-update event and so there's a chance that the test will end before the nodehighlight request has been completed, hence throwing an error.

@@ +55,5 @@
>    // nsContextMenu won't use the property anyway, so just try/catching is ok.
>    try {
> +    document.popupNode = content.document.querySelector("div");
> +  } catch (e) {
> +    return;

The test shouldn't stop there with e10s, should it?
This entire test however should be disabled when the test suite runs on a remote device (with LucidDream), but otherwise it should run as before.
Please ping ochameau for details about this.

::: devtools/client/markupview/test/browser_markupview_links_03.js
@@ +28,2 @@
>    yield setNodeAttribute("body", "contextmenu", "menu2");
> +  yield onMutation;

Same remark about unrelated changes that I made earlier. I'm not against changing onMutated to onMutation, but I don't think you should mix this in with your already big patch.

::: devtools/client/markupview/test/browser_markupview_load_01.js
@@ +5,5 @@
>  "use strict";
>  
>  // Tests that selecting an element with the 'Inspect Element' context
>  // menu during a page reload doesn't cause the markup view to become empty.
> +// See https://bugzilla.mozilla.org/show_bug.cgi?id=1036324.

Same comment here (and in other similar cases in other files): 
if you were updating this file for other reasons (fixing eslint errors or introducing the testActor), then it'd be totally fine to fix minor spelling errors or adding periods, or changing variable names.
But since adding just one character in a comment is all your doing in this file, I'd suggest not doing it (again, for the sake of keeping history simple and clean).

::: devtools/client/markupview/test/browser_markupview_mutation_01.js
@@ +64,5 @@
>      }
>    },
>    {
>      desc: "Adding ::after element",
> +    numMutations: 5,

I see you've changed a couple of these numMutations in the test. I wonder why the number would suddenly be 5 where really it should be 2.
Could you explain what's happening here? Are we catching mutations from previous tests perhaps?

::: devtools/client/markupview/test/browser_markupview_tag_edit_09.js
@@ +60,5 @@
>    info("Editing the attribute value and waiting for the mutation event");
>    let input = inplaceEditor(attr).input;
>    input.value = "viewBox=\"<>\"";
>    EventUtils.sendKey("return", inspector.panelWin);
> +  yield onMutation;

Same comment about unrelated changes.

Comment 18

[Grisha Pushkov]

> about my comments regarding unrelated changes, you could also simply move them into a second, separate, commit with an appropriate commit message
Definitely will do it this way. I doubted about whether it should be done here at all, but separate bug for 'dots and formatting' looks oddly.

> I'm not comfortable with this change because that means people used to coding for 120 chars (which is pretty common) will see no warnings in many places.
> So I'd advised to only ignore if the line is longer than 120, or maybe let's just start simple and only ignore lines that contain this pattern: data:image/png because it seems to be the only case so far that we really want to ignore.
I found bug 1195354, so will let this change to be there.

> I think we should be using the testActor to get the node's outerHTML here.
As I remember, I found cases where inspector.walker can't be replaced with testActor (not this case).
I will look again, but main idea is that we should use testActor as much as possible?

> I'm wondering if this test doesn't throw an exception. The image element is selected in the inspector, and we're mutating it here, so in theory, we should have to also wait for the inspector-updated event to prevent hanging requests when the test ends.
> Why are you not waiting for the node-highlight event anymore?
> I think it races with the inspector-update event and so there's a chance that the test will end before the nodehighlight request has been completed, hence throwing an error.
Yes, I was thinking about races too. After removing and trying to determine event order, I got that 'inspector-updated' is always fired last.
Wrote about it in comment 12, "Dont't understand why we need to wait on two events at once...".

We should determine if these events have race condition, and I will write a comment why we need to wait for two events if it is.
I thought that "inspector-update" event is universal, that it aggregates all other mutations into one.

> The test shouldn't stop there with e10s, should it?
Hm, I thought "return" would return execution only for exception handler and not for outer function. Will check it.
I added return because of ESLint error for empty function.

> I see you've changed a couple of these numMutations in the test. I wonder why the number would suddenly be 5 where really it should be 2.
> Could you explain what's happening here? Are we catching mutations from previous tests perhaps?
Wrote about it in 12, "numMutation = 2, as it was before, is not enough and markup change doesn't happen...".
I can't understand this change in behaviour after introducing testActor.

Comment 19

[Patrick Brosset <:pbro>]

(In reply to Grisha Pushkov from comment #18)
> > I think we should be using the testActor to get the node's outerHTML here.
> As I remember, I found cases where inspector.walker can't be replaced with
> testActor (not this case).
> I will look again, but main idea is that we should use testActor as much as
> possible?
So the idea is that everytime we want to assert that something happened in the DOM of the page as a result of using devtools, we should not be using devtools itself to check it. Say you're using devtools in a test to remove a node, if you then use devtools again to check if the node was really removed, then you're not testing it fully. Maybe devtools thinks the node was removed, but really it wasn't. So it's all a question of *what* we're testing here. If we're only concerned about the UI logic, it might be fine to use the walker to check things are correct. But if we want an integration test, that ensures all parts work well together, then we really want to access the dom directly to check (and that means using the testActor in this case).

> > I'm wondering if this test doesn't throw an exception. The image element is selected in the inspector, and we're mutating it here, so in theory, we should have to also wait for the inspector-updated event to prevent hanging requests when the test ends.
> > Why are you not waiting for the node-highlight event anymore?
> > I think it races with the inspector-update event and so there's a chance that the test will end before the nodehighlight request has been completed, hence throwing an error.
> Yes, I was thinking about races too. After removing and trying to determine
> event order, I got that 'inspector-updated' is always fired last.
> Wrote about it in comment 12, "Dont't understand why we need to wait on two
> events at once...".
About your remark in comment 12, I agree no need to wait for both onMutated and onModified. What we really care about in this test is if the markup-mutation event was fired. And we know for sure that it's fired after the modification has been done. So we can safely avoid waiting for onModified.
Having said this, we know that after a mutation was handled in the markup-view, this triggers other things to happen: the breadcrumbs widget may change, the rule-view and computed-view too (because other css rules might apply). So there's a lot of subsequent async requests that follow a markup-mutation event.
In this test, we don't really care about them *but* the problem is that if we dont wait for them to be done, we might be ending the test before they had a chance to finish. So I think here, you should wait for both markup-mutation (because that's what makes sense for the test) *and* inspector-updated (which signals that all panels have updated, because that's what prevents the test from breaking).

> I thought that "inspector-update" event is universal, that it aggregates all
> other mutations into one.
No it does not. It waits for all inspector sub-panels to have finished updating when something causes them to update.

> > The test shouldn't stop there with e10s, should it?
> Hm, I thought "return" would return execution only for exception handler and
> not for outer function. Will check it.
> I added return because of ESLint error for empty function.
Ah ok, no, return does not simply exit from catch, but from the outer function scope.
And yes, ESLint does complain about empty catch blocks, because they're generally a sign that something fishy is going on.
Sometimes we do need to try something and not do anything in the catch block (although that's rare), but when we do this, we usually put a comment inside the catch block to explain when it can fail and why we do nothing when that happens (a comment in there will make sure ESLint doesn't complain).

> > I see you've changed a couple of these numMutations in the test. I wonder why the number would suddenly be 5 where really it should be 2.
> > Could you explain what's happening here? Are we catching mutations from previous tests perhaps?
> Wrote about it in 12, "numMutation = 2, as it was before, is not enough and
> markup change doesn't happen...".
> I can't understand this change in behaviour after introducing testActor.
Right, I did read comment 12 some days ago but had forgotten about that part. We do need to understand what causes this change though. I'll keep the needinfo here to try and spend some time on this test. But we just can't land it with this change, or if we do, we need to understand why.

Comment 20

[Patrick Brosset <:pbro>]

Found out the problem in browser_markupview_mutation_01.js:
The way you've refactored the code in the for TEST_DATA loop makes you call the 'test' function of each test case over and over again.
So if you pass 1000 as the numMutations, it will call 'test' at every iteration of the loop. This wasn't the case before.
You could wrap `yield test(testActor)` in `if (!seenMutations) {}`

Comment 21

[Grisha Pushkov]

Thanks! Happy that it's not a specific to testActor problem.
Will update separated patches for testActor, ESLint and small style fixes soon.

Comment 22

[Patrick Brosset <:pbro>]

This has been inactive for a while, and most of the markup-view tests are now using the testActor since bug 1252099.