Closed Bug 977033 Opened 6 years ago Closed 6 years ago

Add automated test for bug 963639: customizing attribute on non customizable toolbar

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: mihaelav, Assigned: mihaelav)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
Automation coverage is needed for bug 963639 - CustomizeMode _onToolbarVisibilityChange sets @customizing on non-customizable toolbars
Attachment #8382101 - Flags: review?(gijskruitbosch+bugs)
So is this test no longer breaking for you? :-)
Flags: needinfo?(mihaela.velimiroviciu)
Yes, it passes on latest mozilla-central (clean repo - yey!) and fails on an older version (mozilla-central-5376a2ce7b29).
Flags: needinfo?(mihaela.velimiroviciu)
Comment on attachment 8382101 [details] [diff] [review]
v1

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

Almost there, please address the following:

::: browser/components/customizableui/test/browser_963639_customizing_attribute_non_customizable_toolbar.js
@@ +8,5 @@
> +
> +add_task(function() {
> +  info("Test for Bug 963639 - CustomizeMode _onToolbarVisibilityChange sets @customizing on non-customizable toolbars");
> +
> +  let toolbar = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "toolbar");

I actually think

document.createElement("toolbar");

will work just fine, but please doublecheck that.

@@ +15,5 @@
> +
> +  let testToolbar = document.getElementById(kToolbar)
> +  ok(testToolbar, "Toolbar was created.");
> +
> +  var navBar = document.getElementById("navigator-toolbox");

This isn't a navBar, that's the toolbar with id = 'nav-bar';

@@ +16,5 @@
> +  let testToolbar = document.getElementById(kToolbar)
> +  ok(testToolbar, "Toolbar was created.");
> +
> +  var navBar = document.getElementById("navigator-toolbox");
> +  ok(navBar.innerHTML.contains(kToolbar), "Toolbar was added to the navigator toolbox");

innerHTML is evil, especially in XUL. Please use:

is(gNavToolbox.getElementsByAttribute("id", kToolbar).length, 1, ...)

instead.

(and ok(!gNavToolbox..., ...) below)
Attachment #8382101 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch v2Splinter Review
Updated based on review
Attachment #8382101 - Attachment is obsolete: true
Attachment #8382190 - Flags: review?(gijskruitbosch+bugs)
Attachment #8382190 - Flags: review?(gijskruitbosch+bugs) → review+
There are some failures but I don't think they are caused by this test:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35660761&tree=Try
(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #6)
> There are some failures but I don't think they are caused by this test:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35660761&tree=Try

Gijs, any thoughts on this?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #7)
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #6)
> > There are some failures but I don't think they are caused by this test:
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=35660761&tree=Try
> 
> Gijs, any thoughts on this?

I agree. Please set checkin-needed so this can land. Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #7)
> > (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #6)
> > > There are some failures but I don't think they are caused by this test:
> > > https://tbpl.mozilla.org/php/getParsedLog.php?id=35660761&tree=Try
> > 
> > Gijs, any thoughts on this?
> 
> I agree. Please set checkin-needed so this can land. Thanks!

Thank you!
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/d36aeeed76a9
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d36aeeed76a9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Thanks Mihaela for writing the test!
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.