946754 - Switch CustomizableUI tests from using homebrewed test runner to built-in add_task

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Our tests for CustomizableUI were written at a time when writing mochitests for asynchronous stuff resulted in much callback spaghetti.

To resolve this, we wrote our own homebrewed testrunner thing to make writing Promise-y tests easier.

It turns out that bug 872229 landed a few months back which gives us this Promise-y goodness in the form of add_task.

We should axe our homebrew testrunner and use add_task instead.

Instructions:

1) Go inside browser/components/customizableui/test/
2) For each file that starts with browser_, open it up, and find the "gTests" object.
3) Extract the "run" functions for each of the objects in gTests, and put those in add_task functions, like this:

add_task(function name_of_test() {
  // Code from "run" function in here.
});

4) Make sure the test runs by executing it from the root mozilla-central folder:

./mach mochitest-browser browser/components/customizableui/test/browser_test_file_name_here.js

We might have to do a little thinking about clean-up functions, but I think this conversion will be relatively straight forward.

New hackers should get a simple Firefox build going: https://developer.mozilla.org/en/docs/Simple_Firefox_build

These videos are also very helpful: http://codefirefox.com/

Comment 1

[Peiyong Lin[:lpy](UTC+8)]

Hi Mike, could you please help me deal with the asyncCleanup? like the one here http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js#144

I have no idea of the asyncCleanup. Where should I put them?

Comment 2

[Peiyong Lin[:lpy](UTC+8)]

Gijs told me I could add another task, I tried and it seemed work.

Comment 3

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug946754-v1.patch

Switch all homebrewed tests to add_task.
I built again on OSX 10.8 and mochitest-browser in browser/components/customizableui passed with no failed tests.
However, the number of passed tests is 1152, but before I started, the number of passed tests is 1258. I could not figure out the reason.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8344151 [details] [diff] [review]
bug946754-v1.patch

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

This looks like the right idea! Just a few notes below. Also, you forgot to remove the test runner stuff from head.js.

Thanks,

-Mike

::: browser/components/customizableui/test/browser_873501_handle_specials.js
@@ +80,2 @@
>  
>  function cleanup() {

Might as well just pass removeCustomToolbars to registerCleanupFunction now:

registerCleanupFunction(removeCustomToolbars);

::: browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js
@@ +44,5 @@
> +  yield endCustomizing();
> +});
> +
> +add_task(function asyncCleanup() {
> +  yield (function() {

Why so complicated? Why can't this be:

add_task(function asyncCleanup() {
  yield endCustomizing();
  try {
    CustomizableUI.destroyWidget(kTestWidget1);
  } catch(ex) {}
  try {
    CustomizableUI.destroyWidget(kTestWidget2);
  } catch(e) {}
  yield resetCustomization();
});

::: browser/components/customizableui/test/browser_877178_unregisterArea.js
@@ +6,2 @@
>  
> +registerCleanupFunction(cleanup);

This can just be registerCleanupFunction(removeCustomToolbars);

::: browser/components/customizableui/test/browser_877447_skip_missing_ids.js
@@ +6,2 @@
>  
> +registerCleanupFunction(cleanup);

registerCleanupFunction(removeCustomToolbars);

@@ +16,5 @@
> +  is(btn.parentNode.parentNode.id, CustomizableUI.AREA_NAVBAR, "Button should be in navbar");
> +  btn.remove();
> +  is(btn.parentNode, null, "Button is no longer in the navbar");
> +  ok(CustomizableUI.inDefaultState, "Should be back in the default state, " +
> +     "despite unknown button ID in placements.");

Let's keep the old formatting here.

@@ +18,5 @@
> +  is(btn.parentNode, null, "Button is no longer in the navbar");
> +  ok(CustomizableUI.inDefaultState, "Should be back in the default state, " +
> +     "despite unknown button ID in placements.");
> +});
> +add_task(function asyncCleanup() {

Nit - newline before the add_task.

::: browser/components/customizableui/test/browser_878452_drag_to_panel.js
@@ +37,5 @@
> +  assertAreaPlacements(CustomizableUI.AREA_PANEL, placementsAfterAppend);
> +  ok(!CustomizableUI.inDefaultState, "Should no longer be in default state.");
> +  let palette = document.getElementById("customization-palette");
> +  simulateItemDrag(btn, palette);
> +  ok(CustomizableUI.inDefaultState, "Should be in default state again."); 

Nit - trailing whitespace

@@ +42,5 @@
> +});
> +
> +// Dragging an item from the palette to an empty panel should also work.
> +add_task(function() {
> +  yield (function() {

This can be simplified to:

add_task(function() {
  let widgetIds = getAreaWidgetIds(CustomizableUI.AREA_PANEL);
  while (widgetIds.length) {
    CustomizableUI.removeWidgetFromArea(widgetIds.shift());
  }
  yield startCustomizing();
  // Rest of test
});

::: browser/components/customizableui/test/browser_880164_customization_context_menus.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

use strict

@@ +123,2 @@
>  
> +// Right-click on the home button while in customization mode should show a context menu with options to move it.

Break this up over two lines.

@@ +171,2 @@
>  
> +// Right-click on an item in the panel while in customization mode should show a context menu with options to move it.

Break this up over two lines.

@@ +193,2 @@
>  
> +// Test the toolbarbutton panel context menu in customization mode without opening the panel before customization mode

Break this up over two lines.

::: browser/components/customizableui/test/browser_886323_buildArea_removable_nodes.js
@@ +2,3 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

use strict

@@ +10,4 @@
>  
> +// Removable nodes shouldn't be moved by buildArea
> +add_task(function() {
> +  yield (function() {

Again, let's drop the function wrapping here. I don't think it gives us anything.

@@ +33,5 @@
> +  CustomizableUI.registerToolbarNode(gLazyArea, []);
> +  assertAreaPlacements(kLazyAreaId, [], "Placements should no longer include widget.");
> +  is(btn.parentNode.id, gNavBar.customizationTarget.id,
> +     "Button shouldn't actually have moved as it's not removable");
> +  yield (function() {

Same as above.

::: browser/components/customizableui/test/browser_887438_currentset_shim.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

use strict

::: browser/components/customizableui/test/browser_888817_currentset_updating.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

use strict

::: browser/components/customizableui/test/browser_890140_orphaned_placeholders.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

use strict

::: browser/components/customizableui/test/browser_890262_destroyWidget_after_add_to_panel.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

use strict

::: browser/components/customizableui/test/browser_892955_isWidgetRemovable_for_removed_widgets.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

use strict

::: browser/components/customizableui/test/browser_892956_destroyWidget_defaultPlacements.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  const kWidgetId = "test-892956-destroyWidget-defaultPlacement";

use strict

::: browser/components/customizableui/test/browser_909779_overflow_toolbars_new_window.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

use strict

::: browser/components/customizableui/test/browser_913972_currentset_overflow.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

use strict - I'll stop mentioning this one now, I think you get it. :)

::: browser/components/customizableui/test/browser_914138_widget_API_overflowable_toolbar.js
@@ +15,5 @@
>  let originalWindowWidth;
>  
> +// Adding a widget should add it next to the widget it's being inserted next to.
> +add_task(function() {
> +  yield (function() {

Drop the unnecessary function wrapping.

@@ +42,5 @@
> +  ok(navbar.querySelector("#" + kHomeBtn), "Home button should be in the navbar");
> +  ok(!overflowList.querySelector("#" + kHomeBtn), "Home button should no longer be overflowing");
> +  ok(navbar.querySelector("#" + kTestBtn1), "Test button should be in the navbar");
> +  ok(!overflowList.querySelector("#" + kTestBtn1), "Test button should no longer be overflowing");
> +  yield (function() {

Drop the unnecessary function wrapping. This applies to all ports of setup/teardown from here on out - I'll stop mentioning it now.

::: browser/components/customizableui/test/browser_934113_menubar_removable.js
@@ +11,1 @@
>        let navbar = document.getElementById("nav-bar");

Please fix the formatting here.

Comment 5

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug946754-V2.patch

Thanks Mike, I update my patch.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8345141 [details] [diff] [review]
bug946754-V2.patch

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

The code looks good, but it's been bitrotted. Could you please regenerate the patch, and fix it for browser_880164_customization_context_menus.js? I think it'll be ready then.

Comment 7

[Peiyong Lin[:lpy](UTC+8)]

I am sorry, I didn't get your point. What do you mean regenerate the patch?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Peiyong Lin[:lpy] from comment #7)
> I am sorry, I didn't get your point. What do you mean regenerate the patch?

A patch from bug 945191 landed recently that changed browser_880164_customization_context_menus.js, which means that your change to that file no longer applies cleanly.

So what I need you to do is pop your patch (if you're using mq), or somehow save your current patch locally. Then revert the changes, and pull the latest changes from the tree, and re-apply your patch (or push your patch if using mq).

You should see conflicts reported for browser_880164_customization_context_menus.js. That's expected, and that's what you need to fix. Open up the file, and clean it up, and then post a new patch when this is done.

It shouldn't be too big of a job - I think what happened was that a new test was added, so that new test will need to be switched to using add_task like the others.

Let me know if you need further guidance on this.

Comment 9

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug946754-v3.patch

Thank you :)

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8347037 [details] [diff] [review]
bug946754-v3.patch

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

I want to get this landed before you get bitrotted again - but there's something missing. I forgot that we flip a pref to fast-track the customization transition for tests. I'm going to update this patch to put that pref flip into the start and endCustomization helper functions in head.js, and fix the last little nit and get this thing landed.

::: browser/components/customizableui/test/browser_885530_showInPrivateBrowsing.js
@@ +133,5 @@
> +  private2.close();
> +
> +  CustomizableUI.destroyWidget("some-widget");
> +});
> +add_task(function asyncCleanup() {

Add a newline above this please.

::: browser/components/customizableui/test/head.js
@@ -268,5 @@
>    return deferred.promise;
>  }
> -
> -function testRunner(testAry, asyncCleanup) {
> -  Services.prefs.setBoolPref("browser.uiCustomization.disableAnimation", true);

Hrm - so I see by switching to add_task, we lose the ability to hook this pref switch into each test.

Just for context, this pref allows us to shave a lot of time off of our test runs by speeding up the customize mode transition.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Finished scrubbing the patch, landed on fx-team:

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

Comment 12

[Peiyong Lin[:lpy](UTC+8)]

Thank you Mike :)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f067279da2c