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



Add-on SDK
5 years ago
5 years ago


(Reporter: nodeless, Unassigned)


Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)



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

5 years ago
Created attachment 772477 [details] [diff] [review]
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 on attachment 772477 [details] [diff] [review]
Fix for all broken testcases.

Matteo, you'd be the right person to review this, yeah?
Attachment #772477 - Flags: review?(zer0)
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:
Flags: needinfo?(nodeless)

Comment 4

5 years ago
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.
Flags: needinfo?(nodeless)
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`

Flags: needinfo?(nodeless)

Comment 6

5 years ago
Created attachment 776954 [details]
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.
Flags: needinfo?(nodeless)
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');
Attachment #772477 - Flags: review?(zer0) → review-
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!
Priority: -- → P1

Comment 9

5 years ago
Created attachment 783543 [details] [diff] [review]

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

Checked that all tests pass on Windows.
Attachment #772477 - Attachment is obsolete: true
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:
Flags: needinfo?(nodeless)

Comment 11

5 years ago
Thanks Matteo.

Name "nodeless" is fine. No link.
Flags: needinfo?(nodeless)
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.