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

RESOLVED INVALID

Status

Mozilla QA
Mozmill Tests
P1
normal
RESOLVED INVALID
3 years ago
9 months ago

People

(Reporter: cosmin, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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

3 years ago
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 :|
Priority: -- → P1
So in which case does this fail for l10n tests?
(Reporter)

Comment 3

3 years ago
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

3 years ago
(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 :|
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.

Updated

3 years ago
Blocks: 799149
(Reporter)

Comment 6

3 years ago
Created attachment 8505441 [details] [diff] [review]
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
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

3 years ago
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.
Attachment #8505441 - Flags: feedback+
Mozmill tests have been superseded by Marionette tests.
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.