891224 - sidebar tests fail: check for getAttribute("checked")=='false' rather than empty string

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[nodeless]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

1. "cfx testall"


Actual results:

Various sidebar tests fail when checking for getAttribute('checked') to be false.


Expected results:

All tests pass.

Comment 1

[nodeless]

Attachment: Fix for all broken testcases.

Fix is attached. All tests (8490 tests) pass with it on my win7 box targeting ux nightly.

It seems some of the sidebar testcases already used this "getAttribute('checked') || 'false'" idiom. This patch just uses that idiom in all cases.

Comment 2

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

Comment on attachment 772477 [details] [diff] [review]
Fix for all broken testcases.

Matteo, you'd be the right person to review this, yeah?

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Hi nodeless, in which branch are you testing that? Currently master shouldn't have sidebar module yet; plus the sidebar module and tests, because the dependency from button, requires Australis, so on Firefox 22 you will get definitely some errors.

Could you please let us know which branch are you using; and in case run the `cfx testall` on Australis? (If you need to download it, take a look here: https://wiki.mozilla.org/UX_Branch)

Comment 4

[nodeless]

Sorry I should've been more clear about that.

This patch is from the australis branch (with master merged in, but it should apply cleanly even without that), and I was running the tests against the ux nightly.

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

So, using the latest Firefox UX branch with australis branch, I can't reproduce this issue.

I updated australis branch with master, and I had another issue in the test (related to getter and setter), but not the one here reported. I fixed that in Bug 894624.
Could you please test against the new branch and the latest UX? And what is specifically the command line you're using to run the test against UX branch? Remember that you need to specify both the binary (of course) and the `-o` argument at command line, so something like:

    `cfx testall -b <path-to-australis-binary> -o`

Before running the all test, can you also check if you have this issue just with the sidebar test stuite? You can filter it using -f:

    `cfx test -f sidebar -b <path-to-australis-binary> -o`

Thanks!

Comment 6

[nodeless]

Attachment: cfx test sidebar output

I updated my UX branch to 25.0a1 (2013-07-16), and I just pulled the latest australis branch. Tests fail (see attachment), but all tests pass with my patch applied.

I'm running this on win7 64-bit. Maybe this doesn't fail on other platforms (that would be strange though...)? I don't know why the "checked" attribute would only be an empty string on Windows.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 772477 [details] [diff] [review]
Fix for all broken testcases.

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

::: test/addons/private-browsing-supported/test-sidebar.js
@@ +134,4 @@
>            let sidebarMI = getSidebarMenuitems();
>            for each (let mi in sidebarMI) {
>              assert.ok(BUILTIN_SIDEBAR_MENUITEMS.indexOf(mi.getAttribute('id')) >= 0, 'the menuitem is for a built-in sidebar')
> +            assert.equal(mi.getAttribute('checked') || 'false', 'false', 'no sidebar menuitem is checked');

Instead repeat this all the time, we could have an helper function on top of the test suite - or better, `test/sidebar/util` - similar to this:

    let isChecked = node => node.getAttribute('checked') === 'true';

Adding a comment above the declaration about why this is needed – the differences about platforms, and the reference bug number (Bug 894809, that could be closed / wontfixed / or set as duplicated, but at least we have a reference).

There fore the assertion will be:

    assert.ok(!isChecked(mi), 'no sidebar menuitem is checked');

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

Thanks a lot for the patch nodeless, I "rejected" only because I'd like to document that behavior and explain why we need that; if you could do that it would be great!

Comment 9

[nodeless]

Attachment: 0001-Fix-failing-sidebar-tests.patch

Added helper method to avoid duplicating the false equality idiom everywhere.

Checked that all tests pass on Windows.

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

Thanks! I'm going to land this patch, let me know the name to put in the contributors list, and a link to your site / repo, if you have one:

https://addons.mozilla.org/en-US/developers/docs/sdk/latest/dev-guide/credits.html

Comment 11

[nodeless]

Thanks Matteo.

Name "nodeless" is fine. No link.