<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

:Gavin Sharp [email: gavin@gavinsharp.com]

Updated

•

11 years ago

Assignee: gavin.sharp → nobody

Status: RESOLVED → REOPENED

Keywords: leave-open

Resolution: FIXED → ---

Target Milestone: Firefox 32 → ---

Comment hidden (Legacy TBPL/Treeherder Robot)

Vaibhav (:vaibhav1994)

Comment 15

•

11 years ago

This test fails intemittently for linux bc1 jobs, as can be seen in a recent try push: https://tbpl.mozilla.org/?tree=Try&rev=43c85fb644ef The goal is to turn these on as --run-by-dir, we don't have too many tests that are problematic(19 total), if we can fix the easy ones now that will help! :) Gavin, can you figure out why these might still be failing intermittently while running the tests in only browser/components/search/test directory?

Flags: needinfo?(gavin.sharp)

Comment 16

•

11 years ago

I'm adding this bug to our priority backlog for the next iteration, so we can expect it to be assigned August 5th.

Flags: needinfo?(gavin.sharp)

Updated

•

11 years ago

Status: REOPENED → NEW

Flags: firefox-backlog+

Updated

•

11 years ago

Points: --- → 3

QA Whiteboard: [qa-]

Updated

•

11 years ago

Assignee: nobody → jaws

Status: NEW → ASSIGNED

Iteration: --- → 34.2

Assignee

Comment 17

•

11 years ago

Attached patch Patch (obsolete) — Details — Splinter Review

Try is closed so I can't push it there. But this patch forces the focus to change before the next test starts, which fixes the failures locally for me.

Attachment #8472732 - Flags: review?(gijskruitbosch+bugs)

:Gijs (he/him)

Comment 18

•

11 years ago

Comment on attachment 8472732 [details] [diff] [review] Patch Review of attachment 8472732 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review for now because of the commented-out/early-return things + waitForFocus. ::: browser/components/search/test/browser_426329.js @@ +97,5 @@ > + searchBar = BrowserSearch.searchBar; > + > + let windowFocused = Promise.defer(); > + SimpleTest.waitForFocus(windowFocused.resolve, window); > + window.focus(); waitForFocus calls window.focus for you if it isn't focused; you shouldn't need to manually call window.focus here. @@ +114,5 @@ > + return deferred.promise; > +} > + > +add_task(function() { > + yield promiseSetEngine(); This is kind of sad... we should really get task-based setup and cleanup functions in the mochitest framework... :-\ @@ +141,5 @@ > + "Alt+Return key loaded results in foreground tab"); > + is(event.originalTarget.URL, expectedURL(searchBar.value), "testAltReturn opened correct search page"); > +}); > + > +//Shift key has no effect for now, so skip it I'm confused - this was not skipped in the old test; why are we skipping it now? @@ +161,5 @@ > +add_task(function testLeftClick() { > + yield prepareTest(); > + simulateClick({ button: 0 }, searchButton); > + let event = yield promiseOnLoad(); > + is(gBrowser.tabs.length, preTabNo, "LeftClick did not open new tab"); I'm confused. How come we're back to preTabNo here, without any tabs being closed since we checked against preTabNo + 1 ? Ideally, it would also be nice if all these numbers weren't going to be off-by-one as soon as a single test fails (ie right now it'd have a knock-on effect). @@ +192,5 @@ > + is(event.originalTarget.URL, expectedURL(searchBar.value), "testShiftMiddleClick opened correct search page"); > +}); > + > +// prevent the search buttonmenu from opening during the drag tests > +function promisePreventPopupShowing() { Hrmpf. Not a big fan of having both this and the 1-line-different promisePopupShowing, but I'm not sure how to have a clear parameter that just does the preventDefault bit. Passing in a function is verbose on the caller's side, and it's weird to pass a callback function to a function returning a promise. The promise callbacks will be called async, so we can't use those. We could use a boolean, but that has clarity issues. String param is icky. Pick one of the above depending on your good judgment? Keeping it like this might be fine. @@ +238,5 @@ > + is(gBrowser.tabs.length, preTabNo, "RightClick did not open new tab"); > + is(gBrowser.currentURI.spec, "about:blank", "RightClick did nothing"); > + }, 5000); > +}); > +*/ Err... why is this commented out? @@ +264,5 @@ > + ok(count > 0, "form history entry '" + searchEntries[i] + "' should exist"); > + } > +}); > + > +function* promisePopupShown(aPopup) { Please put this in head.js if it doesn't exist there already.

Attachment #8472732 - Flags: review?(gijskruitbosch+bugs)

Assignee

Comment 19

•

11 years ago

(In reply to :Gijs Kruitbosch (intermittently here 14-15 August; then away until 19th) from comment #18) > @@ +141,5 @@ > > + "Alt+Return key loaded results in foreground tab"); > > + is(event.originalTarget.URL, expectedURL(searchBar.value), "testAltReturn opened correct search page"); > > +}); > > + > > +//Shift key has no effect for now, so skip it > > I'm confused - this was not skipped in the old test; why are we skipping it > now? Yes, it was skipped in the old test, see http://hg.mozilla.org/mozilla-central/annotate/5299864050ee/browser/components/search/test/browser_426329.js#l84

:Gijs (he/him)

Comment 20

•

11 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19) > (In reply to :Gijs Kruitbosch (intermittently here 14-15 August; then away > until 19th) from comment #18) > > @@ +141,5 @@ > > > + "Alt+Return key loaded results in foreground tab"); > > > + is(event.originalTarget.URL, expectedURL(searchBar.value), "testAltReturn opened correct search page"); > > > +}); > > > + > > > +//Shift key has no effect for now, so skip it > > > > I'm confused - this was not skipped in the old test; why are we skipping it > > now? > > Yes, it was skipped in the old test, see > http://hg.mozilla.org/mozilla-central/annotate/5299864050ee/browser/ > components/search/test/browser_426329.js#l84 D'oh. Sorry I missed that - the diff is pretty... terrible (not the best excuse, but hey). :-(

Assignee

Comment 21

•

11 years ago

(In reply to :Gijs Kruitbosch (intermittently here 14-15 August; then away until 19th) from comment #18) > @@ +161,5 @@ > > +add_task(function testLeftClick() { > > + yield prepareTest(); > > + simulateClick({ button: 0 }, searchButton); > > + let event = yield promiseOnLoad(); > > + is(gBrowser.tabs.length, preTabNo, "LeftClick did not open new tab"); > > I'm confused. How come we're back to preTabNo here, without any tabs being > closed since we checked against preTabNo + 1 ? > > Ideally, it would also be nice if all these numbers weren't going to be > off-by-one as soon as a single test fails (ie right now it'd have a knock-on > effect). prepareTest resets preTabNo at the beginning of each of these tests.

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

Assignee

Comment 22

•

11 years ago

Attached patch Patch v2 (obsolete) — Details — Splinter Review

Attachment #8472732 - Attachment is obsolete: true

Attachment #8473235 - Flags: review?(gijskruitbosch+bugs)

:Gijs (he/him)

Comment 23

•

11 years ago

Comment on attachment 8473235 [details] [diff] [review] Patch v2 Review of attachment 8473235 [details] [diff] [review]: ----------------------------------------------------------------- r=me assuming this is green on try (is open again, AIUI?), and you address the comments below one way or another (for some "no, I'm not going to do that" is perfectly legit; for others, not so much :-) ) ::: browser/components/search/test/browser_426329.js @@ +72,5 @@ > + let deferred = Promise.defer(); > + var ss = Services.search; > + > + function observer(aSub, aTopic, aData) { > + switch (aData) { Nit: if (aData == "engine-removed") { ... } @@ +124,5 @@ > + > + is(gBrowser.tabs.length, preTabNo, "Return key did not open new tab"); > + is(event.originalTarget, preSelectedBrowser.contentDocument, > + "Return key loaded results in current tab"); > + is(event.originalTarget.URL, expectedURL(searchBar.value), "testReturn opened correct search page"); Part of me thinks this can be refactored into a function like: function checkLoadEventForSearch(sourceDescription, expectNewTab, expectForeground) { is(gBrowser.tabs.length, preTabNo + (expectNewTab ? 1 : 0), ...); if (!expectNewTab) { is(event.originalTarget, preSelectedBrowser.contentDocument, ...); } else { isnot(event.originalTarget, preSelectedBrowser.contentDocument, ...); if (expectForeground) { is(event.originalTarget, gBrowser.contentDocument, ...); } else { isnot(event.originalTarget, gBrowser.contentDocument, ...); } } is(event.originalTarget.URL, expectedURL(searchBar.value), ...); } which you can just call from all the different permutations with a sourceDescription of how you caused the load event (click, return, whatever), and which then does the right checks with the right description describing what we expect for what kind of event. Or maybe I'm overthinking this. It's late and I'm trying to parse all the individual comparisons which seem very similar, and so I get crazy ideas like this. :-) @@ +192,5 @@ > +}); > + > +add_task(function testDropText() { > + yield prepareTest(); > + let promisePreventPopup = promiseEvent(searchBar, "popupshowing", true); The previous version of this test also checked that we loaded a page, and then forgot to check the URI (as compared with the click/return tests). You seem to have removed the load check. Was that intentional? Wouldn't it make more sense to both check the load and check its URL? Same for the next test. @@ +213,5 @@ > + yield prepareTest(); > + let promisePreventPopup = promiseEvent(searchBar, "popupshowing", true); > + ChromeUtils.synthesizeDrop(searchBar.searchButton, searchBar.searchButton, [[ {type: "text/uri-list", data: "http://www.mozilla.org" } ]], "copy", window); > + yield promisePreventPopup; > + is(searchBar.value, "More Text", "drop text/uri-list on searchbar"); Nit: this implicitly relies on the previous test. Maybe assign to searchBar.value either in prepareTest or in this test? Further nit: AFAICT the purpose of this (sub)test is to see that this does nothing - perhaps the is() check should clarify that. I was confused for a bit how "More Text" was related to "http://www.mozilla.org/" :-) @@ +253,5 @@ > + > +add_task(function testAutocomplete() { > + var popup = searchBar.textbox.popup; > + searchBar.textbox.showHistoryPopup(); > + yield promiseEvent(popup, "popupshown"); Nit: you should store the promise in an intermediate before calling showHistoryPopup, and then yield on the intermediate. Right now, this could theoretically race if popupshown gets called before you create the event listener. @@ +265,5 @@ > + let count = yield countEntries(); > + ok(count == 0, "History cleared"); > +}); > + > +// modified from toolkit/components/satchel/test/test_form_autocomplete.html You moved the other utility functions up; please move these two, too. :-) @@ +287,5 @@ > + return entries; > +} > + > +add_task(function asyncCleanup() { > + searchBar.value = ""; nit: indentation

Attachment #8473235 - Flags: review?(gijskruitbosch+bugs) → review+

Matthew N. [:MattN]

Comment 24

•

11 years ago

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

Whiteboard: [fixed-in-fx-team]

Comment hidden (Legacy TBPL/Treeherder Robot)

Comment 26

•

11 years ago

Looks like that patch made OSX perma-fail. Backed out. https://hg.mozilla.org/integration/fx-team/rev/a2a7099627f1 https://tbpl.mozilla.org/php/getParsedLog.php?id=46082057&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=46082260&tree=Fx-Team

Whiteboard: [fixed-in-fx-team]

Matthew N. [:MattN]

Comment 27

•

11 years ago

(In reply to Matthew N. [:MattN] from comment #24) > https://hg.mozilla.org/integration/fx-team/rev/70b77e14d448 (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #26) > Looks like that patch made OSX perma-fail. Backed out. FWIW the try push I was given was: https://tbpl.mozilla.org/?tree=Try&rev=aadde0cfa228

Comment 28

•

11 years ago

Yup, looks like other platforms are passing fine. Just OSX was failing.

Updated

•

11 years ago

Iteration: 34.2 → 34.3

QA Whiteboard: [qa-]

Flags: qe-verify-

Assignee

Comment 29

•

11 years ago

Try server push, https://tbpl.mozilla.org/?tree=Try&rev=25dd9d1b8425

Assignee

Comment 30

•

11 years ago

Attached patch Patch v2.1 — Details — Splinter Review

Attachment #8473235 - Attachment is obsolete: true

Attachment #8475553 - Flags: review+

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

Assignee

Comment 31

•

11 years ago

Keywords: leave-open

Whiteboard: [fixed in fx-team]

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

Comment 32

•

11 years ago

Status: ASSIGNED → RESOLVED

Closed: 11 years ago → 11 years ago

Flags: in-testsuite+

Resolution: --- → FIXED

Whiteboard: [fixed in fx-team]

Target Milestone: --- → Firefox 34