1236554 - Enable e10s tests in toolkit/components/places/tests/browser

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Comment 1

[Neil Deakin]

Attachment: browser_settitle

Comment 2

[Neil Deakin]

Attachment: browser_notfound.js

Comment 3

[Neil Deakin]

Attachment: browser_bug646422.js

Comment 4

[Neil Deakin]

Attachment: browser_redirect

Comment 5

[Neil Deakin]

Attachment: browser_visituri

Comment 6

[Neil Deakin]

Attachment: browser_visituri_nohistory

Comment 7

[Neil Deakin]

Attachment: browser_visituri_privatebrowsing_perwindowpb

Comment 8

[Neil Deakin]

Attachment: browser_bug399606

Comment 9

[Neil Deakin]

Attachment: browser_places_cleanup

This patch removes Promise.jsm usage.

Comment 10

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2bd0398b854&selectedJob=15019458

Only browser_bug680727.js remains disabled in this directory.

Comment 11

[Tony Mechelynck [:tonymec]]

Added by mistake? (some bot operation?) as bug 399606 comment #31 (shortly _before_ this bug was reported):

2016-01-04 07:46:47 PST Neil Deakin:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=54e559c97ed1

See also the following comments in that same bug (FIXED in 2008).

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8703637 [details] [diff] [review]
browser_settitle

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

::: toolkit/components/places/tests/browser/browser_settitle.js
@@ +22,5 @@
>      stmt.finalize();
>    }
>  }
>  
> +add_task(function *()

nit: I think the most common style is to put the * close to function (function* something)

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8703638 [details] [diff] [review]
browser_notfound.js

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

::: toolkit/components/places/tests/browser/browser_notfound.js
@@ +40,5 @@
> +
> +  let newTabPromise = BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL);
> +  yield Promise.all([visitedPromise, newTabPromise]);
> +
> +  PlacesTestUtils.clearHistory();

should yield

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8703639 [details] [diff] [review]
browser_bug646422.js

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

::: toolkit/components/places/tests/browser/browser_bug646422.js
@@ +18,2 @@
>  
> +  ok(title, 'Content window should initially have a title.');

can we check what the title is, rather than just check it has any title?

@@ +20,2 @@
>  
> +  let newtitle = yield new Promise(resolve => {

he scope of the test seems to check history.pushstate causes an onTitleChanged notification, as such I feel like we should start observing before we call pushState?
I wonder if the test is currently passing just because setting the title is async, but sounds intermittent-failure-prone

@@ +46,5 @@
> +    return content.document.title;
> +  });
> +  is(newtitle, title, 'Title after pushstate.');
> +
> +  gBrowser.removeTab(tab);

yield PlacesTestUtils.clearHistory();

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8703641 [details] [diff] [review]
browser_visituri

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

::: toolkit/components/places/tests/browser/browser_visituri.js
@@ +24,2 @@
>  
> +    observerService.addObserver(observer, name, false);

nit: could probably compact to
Services.obs.addObserver(function observe(subject, topic, data) {
  ...
}, name, false);

@@ +47,5 @@
>      stmt.reset();
>    }
>  }
>  
> +add_task(function *()

nit: star towards function.
If you wish, feel free to bring back the braces in line with "function" when touching these tests.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8703642 [details] [diff] [review]
browser_visituri_nohistory

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

::: toolkit/components/places/tests/browser/browser_visituri_nohistory.js
@@ +23,3 @@
>  
> +    Services.obs.addObserver(observer, name, false);
> +  });

nit: may be compacted using Services.obs and just a function

@@ +26,3 @@
>  }
>  
> +add_task(function *()

nit: arrow towards function

@@ +33,5 @@
>  
> +  yield BrowserTestUtils.openNewForegroundTab(gBrowser, INITIAL_URL);
> +
> +  try {
> +    Services.prefs.clearUserPref("places.history.enabled");

this looks wrong, the test should disable history and clear the pref in a registerCleanupFunction

Comment 17

[Neil Deakin]

> ::: toolkit/components/places/tests/browser/browser_bug646422.js
> > +  ok(title, 'Content window should initially have a title.');
> 
> can we check what the title is, rather than just check it has any title?
> 

Since the title comes from the test framework (the title is "mochitest index /") I don't think we should have a test rely on this.

Comment 18

[Marco Bonardo [:mak]]

ok, makes sense.

Comment 19

[Neil Deakin]

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

Comment 20

[Marco Bonardo [:mak]]

I just enabled ESLint checks in Places, so please check mach eslint does not find errors, or I can do that for you if you didn't set it up yet.

Comment 21

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd033af22d121e2ce58242e75ebaea4b4574b0b9
Bug 1236554 - enable browser_settitle in e10s, r=mak

https://hg.mozilla.org/integration/mozilla-inbound/rev/607774de446ffa77592d6770a51fa7e2ff2a794f
Bug 1236554 - enable browser_notfound.js in e10s, r=mak

https://hg.mozilla.org/integration/mozilla-inbound/rev/9409b1908f8d1eee3f3ec0c0b32cec38b813047b
Bug 1236554 - enable browser_bug646422.js in e10s, r=mak

https://hg.mozilla.org/integration/mozilla-inbound/rev/530d3160f8e775af4efde14aa9985cfe31333c6d
Bug 1236554 - enable browser_redirect in e10s, r=mak

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d2c02559cdf964aa1a8d0da1c61f6b1780cdf9f
Bug 1236554 - enable browser_visituri in e10s, r=mak

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d55178b297d3b120ef659c9b24aaf67c24f6d5c
Bug 1236554 - enable browser_visituri_nohistory in e10s, r=mak

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b5edd360f70a8a16b73ea895fa168ae8cab4bd5
Bug 1236554 - enable browser_visituri_privatebrowsing_perwindowpb in e10s, r=mak

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b1a1c9bdea3ee9ecfb24f9c678aa6fb2de82937
Bug 1236554 - enable browser_bug399606 in e10s, r=mak

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb5dbb033ea9f599e6c625cde297401cf3d5d33
Bug 1236554 - remove Promise.jsm usage, r=mak

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd033af22d12
https://hg.mozilla.org/mozilla-central/rev/607774de446f
https://hg.mozilla.org/mozilla-central/rev/9409b1908f8d
https://hg.mozilla.org/mozilla-central/rev/530d3160f8e7
https://hg.mozilla.org/mozilla-central/rev/9d2c02559cdf
https://hg.mozilla.org/mozilla-central/rev/7d55178b297d
https://hg.mozilla.org/mozilla-central/rev/5b5edd360f70
https://hg.mozilla.org/mozilla-central/rev/2b1a1c9bdea3
https://hg.mozilla.org/mozilla-central/rev/7fb5dbb033ea