1079210 - DOMWalker silently passes if the node that needs to be walked doesn't exists

Status: RESOLVED INVALID

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Cosmin Malutan, [:cosmin-malutan]]

This has been found while working on bug 799149.
At the line below DOMWalker should make an assertion that the node exists otherwise  all tests might pass even if they don't do nothing, this is the case for all other l10n tests. 
http://hg.mozilla.org/qa/mozmill-tests/file/f17ca5e4fae9/lib/dom-utils.js#l242

Comment 1

[Andrei Eftimie]

This seems to be really bad. 

I've just checked and `firefox/tests/l10n/testAccessKeys/test1.js` does literally nothing and the test is green :|

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So in which case does this fail for l10n tests?

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

If you add an assertion at line http://hg.mozilla.org/qa/mozmill-tests/file/f17ca5e4fae9/lib/dom-utils.js#l242 and check that the node we operate on exists, the basic idea is that the tests passes if they don't find the node so we have nothing to tests, if the locator for example changed over the time we never found out because the DOMWalker silently passed then.

Comment 4

[Andrei Eftimie]

(In reply to Cosmin Malutan from comment #3)
> If you add an assertion at line
> http://hg.mozilla.org/qa/mozmill-tests/file/f17ca5e4fae9/lib/dom-utils.
> js#l242 and check that the node we operate on exists, the basic idea is that
> the tests passes if they don't find the node so we have nothing to tests, if
> the locator for example changed over the time we never found out because the
> DOMWalker silently passed then.

That code looks bad.

Indeed we should make sure all elements passed in exist, they are specifically mentioned in the test. I expect them to be tested.

Otherwise we end up not running anything and the test is all green, like it is now :|

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So Andrei's information was more helpful. If this if condition tests that the passed in nodes exist, then we really have to make sure this is true.

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0 WIP

This patch is just for showing the issues, is not a FIX PATCH, all selectors had to be fixed in tests before we can land the assertion in the library.

Here is an report with this change.
http://mozmill-crowd.blargon7.com/#/l10n/report/02345d0791a18ffc72a4b042ab591f95

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I agree. If a node as specified in the list of nodes to check is not available, we should not skip the whole logic silently, but assert. Only that way we can see regressions. Good find Cosmin. Lets try to get this fixed soon.

Comment 8

[Andrei Eftimie]

Comment on attachment 8505441 [details] [diff] [review]
patch v1.0 WIP

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

I see 12 failures with this as an expect. With this fix we will also need to fix the tests themselves.

::: lib/dom-utils.js
@@ +238,5 @@
>      // Go through all the provided ids
>      for (var i = 0; i < aIds.length; i++) {
>        var node = this._getNode(aIds[i]);
> +      assert.ok(node, "Node '" + aIds[i].getBy + ":" + aIds[i][aIds[i].getBy] +
> +                "' has been found.");

I would change this into an expect so we see all missing elements.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozmill tests have been superseded by Marionette tests.