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

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v1

Automation coverage is needed for bug 963639 - CustomizeMode _onToolbarVisibilityChange sets @customizing on non-customizable toolbars

Comment 1

[:Gijs (he/him)]

So is this test no longer breaking for you? :-)

Comment 2

[Mihaela Velimiroviciu (:mihaelav)]

Yes, it passes on latest mozilla-central (clean repo - yey!) and fails on an older version (mozilla-central-5376a2ce7b29).

Comment 3

[:Gijs (he/him)]

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)

Comment 4

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v2

Updated based on review

Comment 5

[Mihaela Velimiroviciu (:mihaelav)]

remote:   https://tbpl.mozilla.org/?tree=Try&rev=cba018fa2e70

Comment 6

[Mihaela Velimiroviciu (:mihaelav)]

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

Comment 7

[Mihaela Velimiroviciu (:mihaelav)]

(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?

Comment 8

[:Gijs (he/him)]

(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!

Comment 9

[Mihaela Velimiroviciu (:mihaelav)]

(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!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d36aeeed76a9

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/d36aeeed76a9

Comment 12

[Matthew N. [:MattN]]

Thanks Mihaela for writing the test!