853865 - Remove node.hasAttributes() code from "lib/localization.js"

Status: RESOLVED INVALID

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Andrei Eftimie]

Since bug 849661 has just landed node.hasAttributes() has been removed.
We need to remove it from our tests and replace it with "node.attributes && node.attributes.length".

We use it in "lib/localization.js" which will break all l10n tests.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Is this causing our l10n tests to fail? If so, this is a high priority. Please confirm ASAP.

Comment 2

[Andrei Eftimie]

It should make them fail.

ATM the l10n runs have also another failure ("Disconnect Error: Application unexpectedly closed").
Something that I've fixed in bug 831297 but regressed in newer FF versions and needs a trivial fix.

Both of them should be a quick fix to make l10n pass.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Please raise a bug for the other failure. I think we should work on these ASAP.

Comment 4

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

We do not run l10n tests on Nightly builds so there is no urge in fixing that. Do also other tests make use of that removed method?

Comment 5

[Andrei Eftimie]

No, it is only used in "lib/localization.js"

Comment 6

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

Ok, so there is no urgency in doing that. If you have the time please upload this simple patch and we can get it landed. Otherwise maybe Jonathan has interest in doing it?

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The issue mentioned in comment 2 is affecting our Aurora test runs.

Comment 8

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

Which is totally unrelated for this bug. :)

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) [away 03/15 - 03/22] from comment #8)
> Which is totally unrelated for this bug. :)

I know, but afaik it still hasn't been raised, and therefore has only been mentioned here. I don't want anyone to imply from these comments that both bugs are not urgent.

Comment 10

[Andrei Eftimie]

Raised bug 854393 for the Disconnect issue.
Will upload patch for that shortly.

Comment 11

[Andrei Eftimie]

Attachment: Patch v1

Fixed the issue.

Comment 12

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) [away 03/15 - 03/22] from comment #6)
> Ok, so there is no urgency in doing that. If you have the time please upload
> this simple patch and we can get it landed. Otherwise maybe Jonathan has
> interest in doing it?

Looks like Andrei is on it and done, but if I can help let me know.

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 728961 [details] [diff] [review]
Patch v1

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/d77671ddd455 (default)

Comment 14

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

We should revert that given that the support of hasAttribute() is back. See bug 849661. Please verify once landed that it works.

Comment 15

[Andrei Eftimie]

This somehow slipped from our radar, lets make a quick check if the change still needs to be backed out.

Comment 16

[Andrei Eftimie]

Attachment: reenable_1.patch

Can't backport the original patch due to conflicts.
Here is a new one, just reverted the change.

Works fine:
http://mozmill-crowd.blargon7.com/#/l10n/report/6d6f6a58b02eeffc06eafa8beaf4e8f1

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 815833 [details] [diff] [review]
reenable_1.patch

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

Pushed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/0d80391087de (default)

Comment 18

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

You might also want to land it on Aurora given that we do not run l10n tests on Nightly. Otherwise trigger a test which was known to fail manually.

Comment 19

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> Otherwise trigger a test which was known to fail manually.

Nothing should fail prior or post this change.

Comment 20

[Andrei Eftimie]

Landed in Aurora so it will be run with the daily testruns:
http://hg.mozilla.org/qa/mozmill-tests/rev/b3b5b5aac962 (mozilla-aurora)

Comment 21

[Andrei Eftimie]

Aurora ran without problems.
I'll land this on the rest of the affected branches.

Here are reports:
Beta: http://mozmill-crowd.blargon7.com/#/l10n/report/ec449c026814fd64783d738c02d1b874
Release: http://mozmill-crowd.blargon7.com/#/l10n/report/ec449c026814fd64783d738c02d2a9b7
ESR24: http://mozmill-crowd.blargon7.com/#/l10n/report/ec449c026814fd64783d738c02d3202f

Comment 22

[Andrei Eftimie]

Landed: 
http://hg.mozilla.org/qa/mozmill-tests/rev/8790a791d296 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/aad6a88f8e19 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/212494ad6330 (mozilla-esr24)

Comment 23

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

This is not fixed, cause the patch has been backed-out, and the temporary feature is no longer available. So this in invalid.