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

RESOLVED FIXED in Firefox 46

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

(Blocks 1 bug)

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 fixed)

Details

Attachments

(9 attachments)

No description provided.
This patch removes Promise.jsm usage.
Blocks: e10s-tests
tracking-e10s: --- → ?
Attachment #8703637 - Flags: review?(mak77)
Attachment #8703638 - Flags: review?(mak77)
Attachment #8703639 - Flags: review?(mak77)
Attachment #8703640 - Flags: review?(mak77)
Attachment #8703641 - Flags: review?(mak77)
Attachment #8703642 - Flags: review?(mak77)
Attachment #8703643 - Flags: review?(mak77)
Attachment #8703644 - Flags: review?(mak77)
Attachment #8703646 - Flags: review?(mak77)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2bd0398b854&selectedJob=15019458

Only browser_bug680727.js remains disabled in this directory.
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 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)
Attachment #8703637 - Flags: review?(mak77) → review+
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
Attachment #8703638 - Flags: review?(mak77) → review+
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();
Attachment #8703639 - Flags: review?(mak77) → review-
Attachment #8703640 - Flags: review?(mak77) → review+
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.
Attachment #8703641 - Flags: review?(mak77) → review+
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
Attachment #8703642 - Flags: review?(mak77) → review-
Attachment #8703643 - Flags: review?(mak77) → review+
Attachment #8703644 - Flags: review?(mak77) → review+
Attachment #8703646 - Flags: review?(mak77) → review+
Priority: -- → P1
> ::: 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.
ok, makes sense.
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.
You need to log in before you can comment on or make changes to this bug.