1162330 - Remove blanket e10s skip-if from satchel tests

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I have some patches to make this possible.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix small typos in satchel_common

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Convert test_form_autocomplete_with_list

Note that I'm planning on squashing these patches before checkin.

This patch splits the work that has to be done in the parent to a chrome
script that works in both e10s and non-e10s modes.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Convert the rest of satchel_common.js

I worked on test_form_autocomplete.html for a while which meant adding more features to the chrome script, but it won't pass anymore for other reasons.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Convert test_popup_enter_event.html

Try pointed out that I forgot to fix this test.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4d20ca7ee61

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8602450 [details] [diff] [review]
Fix small typos in satchel_common

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

::: toolkit/components/satchel/test/mochitest.ini
@@ +1,2 @@
>  [DEFAULT]
> +skip-if = toolkit == 'android' || buildapp == 'b2g' || os == 'linux' # linux - bug 947531

Removing the skip-if = e10s is more than a typo change ;) Make sure this doesn't get pushed before others so the tests don't all fail. This should ideally be in the same patch that moves skip-if to some of the individual tests (perhaps as the last patch in the series after actually fixing the tests).

::: toolkit/components/satchel/test/satchel_common.js
@@ +55,5 @@
>  
>  
>  function getAutocompletePopup() {
>      var Ci = SpecialPowers.Ci;
> +    var chromeWin = SpecialPowers.wrap(window)

Nit: let

@@ +63,5 @@
>                      .rootTreeItem
>                      .QueryInterface(Ci.nsIInterfaceRequestor)
>                      .getInterface(Ci.nsIDOMWindow)
>                      .QueryInterface(Ci.nsIDOMChromeWindow);
> +    var autocompleteMenu = chromeWin.document.getElementById("PopupAutoComplete");

Nit: let

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8602791 [details] [diff] [review]
Convert test_popup_enter_event.html

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

This should be fine since the timing of the key sent as the result of popupshown isn't important.

Comment 8

[Matthew N. [:MattN]]

Apologies for the delays on the remaining reviews. My laptop died on Monday and I've been helping with some 38.0.5 stuff. I'll try to get to them by the weekend.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8602452 [details] [diff] [review]
Convert test_form_autocomplete_with_list

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

Mostly nits but there are enough of them so I'd like to take another look.

::: toolkit/components/satchel/test/parent_utils.js
@@ +7,5 @@
> +function getAutocompletePopup() {
> +  var chromeWin = Services.ww.activeWindow;
> +  var autocompleteMenu = chromeWin.document.getElementById("PopupAutoComplete");
> +
> +  return autocompleteMenu;

Nit: You could fold the last 2 lines:
  return chromeWin.document.getElementById("PopupAutoComplete");

Looking further down, why bother even having this as a helper if we're just assigning it to a global property and using that throughout.

@@ +10,5 @@
> +
> +  return autocompleteMenu;
> +}
> +
> +let autocompleteMenu = getAutocompletePopup();

If you keep this as a global property, can you rename it with a "g" prefix: gAutocompletePopup? (I prefer the "Popup" suffix over "Menu")

@@ +15,5 @@
> +assert.ok(autocompleteMenu, "Got autocomplete popup");
> +
> +function getMenuEntries() {
> +  // Could perhaps pull values directly from the controller, but it seems
> +  // more reliable to test the values that are actually in the tree?

I agree with this comment and it's probably not necessary.

@@ +17,5 @@
> +function getMenuEntries() {
> +  // Could perhaps pull values directly from the controller, but it seems
> +  // more reliable to test the values that are actually in the tree?
> +  var entries = [];
> +  var column = autocompleteMenu.tree.columns[0];

Please use `let` in new code

@@ +27,5 @@
> +}
> +
> +function popupshownListener() {
> +  var results = getMenuEntries();
> +  sendAsyncMessage("onpopupshown", { results });

It seems like it may be a good idea for all of the messages (in both directions) to have a prefix like we do in shipping code to make it more clear which messages are for testing-only.

@@ +35,5 @@
> +
> +function cleanUpFormHist() {
> +  FormHistory.update({ op : "remove" });
> +}
> +cleanUpFormHist();

I think it would be clearer to keep all of the helper functions separate from the callers of the functions instead of interleaving them. Even better to enforce this would be to put all of the methods on an object. This also means not inlining the functions for some of the listeners.

@@ +81,5 @@
> +});
> +
> +function checkRowCount(expectedCount) {
> +  if (autocompleteMenu.tree.view.rowCount != expectedCount) {
> +    setTimeout(() => checkRowCount(expectedCount), 0);

You should use ContentTaskUtils.waitForCondition(…).then(() => sendAsyncMessage("gotMenuChange")); instead of the possibly infinite loop so this fails faster.

@@ +93,5 @@
> +  checkRowCount(expectedCount);
> +});
> +
> +addMessageListener("cleanup", () => {
> +  autocompleteMenu.removeEventListener("popupshown", popupshownListener);

Should this also call cleanUpFormHist so that the last FH test doesn't leave behind leftovers?

::: toolkit/components/satchel/test/satchel_common.js
@@ +158,5 @@
> +  });
> +}
> +
> +var chromeURL = SimpleTest.getTestFileURL("parent_utils.js");
> +var script = SpecialPowers.loadChromeScript(chromeURL);

s/var /let /

::: toolkit/components/satchel/test/test_form_autocomplete_with_list.html
@@ +86,1 @@
>  {

Nit: mind fixing the style while you're touching this line by moving the curly brace to the end of the previous line.

@@ +475,5 @@
>          is(actualValues[i], expectedValues[i], testNum + " Checking menu entry #"+i);
>  }
>  
>  function getMenuEntries() {
> +    return lastAutoCompleteResults;

It would be safer if `lastAutoCompleteResults` was nulled out after use to avoid accidentally using a stale value.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8602453 [details] [diff] [review]
Convert the rest of satchel_common.js

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

::: toolkit/components/satchel/test/parent_utils.js
@@ +79,5 @@
>  
>    FormHistory.count(obj, listener);
>  });
>  
> +function checkRowCount(expectedCount, expectedFirstValue) {

Make it more clear that the 2nd argument is optional by providing a default value:
function checkRowCount(expectedCount, expectedFirstValue = null) {

@@ +89,5 @@
>      setTimeout(() => checkRowCount(expectedCount), 0);
>      return;
>    }
>  
> +  var results = getMenuEntries();

Nit: `let`

@@ +102,5 @@
>    autocompleteMenu.removeEventListener("popupshown", popupshownListener);
>  });
> +
> +let checkObserver = {
> +  observe(subject, topic, data) {

I think this can also go on the object I proposed all of the functions go on.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Matthew N. [:MattN] from comment #9)
> It seems like it may be a good idea for all of the messages (in both
> directions) to have a prefix like we do in shipping code to make it more
> clear which messages are for testing-only.

Normally I'd agree, but these messages are strictly script to script (they're not normal frame message manager messages and don't trigger generic message listeners). Given that, do you still want a prefix?

ContentTaskUtils isn't currently registered in mochitest-plain. I've filed bug 1167412 and (with ted's help) attached a patch there.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rolled up patch

Here's a rolled-up patch. I've addressed all of the comments.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13874e7fbfda

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13874e7fbfda

Comment 15

[:Paolo Amadini]

Linked this bug to two other bugs on file that may use the helpers added here.