Closed
Bug 1371294
Opened 7 years ago
Closed 6 years ago
Add some tests for the follow-on search add-on in mozilla-central
Categories
(Firefox :: Search, enhancement, P2)
Firefox
Search
Tracking
()
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxsearch])
Attachments
(2 files)
We should add some tests for the follow-on search system add-on. The github repo has some, but we should investigate items for in-tree use.
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
This is really P2 until after the main part of 57 is done.
Priority: P1 → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8967684 -
Flags: review?(mdeboer)
Attachment #8967685 -
Flags: review?(mdeboer)
Assignee | ||
Comment 4•6 years ago
|
||
Note: I'll probably land a copy of the test in the follow-on search repository, so that we have it there and linted (although I've checked lint on it already).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8967684 [details] Bug 1371294 - Add a SearchTestUtils.jsm file and stop duplicating promiseNewSearchEngine. https://reviewboard.mozilla.org/r/236410/#review242956 I appreciate the cleanup! Please see the issue, with proposed fix, below: ::: browser/components/search/test/SearchTestUtils.jsm:5 (Diff revision 2) > +"use strict"; > + > +ChromeUtils.import("resource://gre/modules/Services.jsm"); > +ChromeUtils.defineModuleGetter(this, "Assert", > + "resource://gre/modules/Assert.jsm"); This won't work, because the mochitest framework initializes Assert with a custom reporter. The test infrastructure filters the log on specific output, so we'll need to adhere to it. I'd add an `init()` method here to register harness globals is `gTestGlobals` or something. That'd also fix the 'issue' that you're passing in the root dir and `registerCleanup` function everywhere.
Attachment #8967684 -
Flags: review?(mdeboer) → review-
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8967684 [details] Bug 1371294 - Add a SearchTestUtils.jsm file and stop duplicating promiseNewSearchEngine. https://reviewboard.mozilla.org/r/236410/#review242964 ::: browser/extensions/followonsearch/test/browser/test.html:1 (Diff revision 2) > +<!DOCTYPE html> I think you meant to put this file and 'testEngine.xml' in the next patch.
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8967685 [details] Bug 1371294 - Add a basic test for the follow-on search add-on. https://reviewboard.mozilla.org/r/236412/#review242962 ::: browser/extensions/followonsearch/test/browser/browser_followOnTelemetry.js:23 (Diff revision 2) > + > + let histogram = Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS"); > + histogram.clear(); > + > + // Open a tab for the test. > + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser); Why not use ``` registerCleanupFunction(() => Services.search.currentEngine = oldCurrentEngine); let url = BASE_URL + "test.html?searchm=test&mt=TEST"; await BrowserTestUtils.withNewTab(url, async function(browser) { // ... }); ``` ? Although that change would be mainly aesthetical, I think.
Attachment #8967685 -
Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8967684 [details] Bug 1371294 - Add a SearchTestUtils.jsm file and stop duplicating promiseNewSearchEngine. https://reviewboard.mozilla.org/r/236410/#review243010 Two nits, that's all ;-) Thanks! ::: browser/base/content/test/general/browser_contentSearchUI.js:12 (Diff revision 3) > const TEST_ENGINE_BASENAME = "searchSuggestionEngine.xml"; > const TEST_ENGINE_2_BASENAME = "searchSuggestionEngine2.xml"; > > const TEST_MSG = "ContentSearchUIControllerTest"; > > +ChromeUtils.defineModuleGetter(this, "SearchTestUtils", nit: I think you can make this `Cu.import(...);` now, because you're using it right away below. ::: browser/components/search/test/SearchTestUtils.jsm:23 (Diff revision 3) > + > + /** > + * Adds a search engine to the search service. It will remove the engine > + * at the end of the test. > + * > + * @param {String} url The URL of the engine to add. nit: can you align this params' columns with the one below?
Attachment #8967684 -
Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•6 years ago
|
||
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8902ee028f58 Add a SearchTestUtils.jsm file and stop duplicating promiseNewSearchEngine. r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/74e1257f8dfb Add a basic test for the follow-on search add-on. r=mikedeboer
Comment 16•6 years ago
|
||
Backed out for failing browser_contentSearchUI.js backout: https://hg.mozilla.org/integration/autoland/rev/d7a44870dd57 push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=74e1257f8dfb96414ce2a6937cf3b6e1f80d9b36 failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174779533&repo=autoland&lineNumber=5971 14:28:53 INFO - 107 INFO TEST-PASS | browser/base/content/test/general/browser_contentSearchUI.js | State - 14:28:53 INFO - 108 INFO Leaving test bound formHistory 14:28:53 INFO - 109 INFO Entering test bound cycleEngines 14:28:53 INFO - Buffered messages finished 14:28:53 ERROR - 110 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_contentSearchUI.js | Engine cycled correctly - Got undefined, expected browser_searchSuggestionEngine searchSuggestionEngine2.xml 14:28:53 INFO - Stack trace: 14:28:53 INFO - chrome://mochikit/content/browser-test.js:test_is:1280 14:28:53 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_contentSearchUI.js:resolver:399 14:28:53 INFO - jar:file:///Z:/task_1524232617/build/application/firefox/omni.ja!/components/nsSearchService.js:notifyAction:937 14:28:53 INFO - jar:file:///Z:/task_1524232617/build/application/firefox/omni.ja!/components/nsSearchService.js:set currentEngine:4136 14:28:53 INFO - resource:///modules/ContentSearch.jsm:_onMessageSetCurrentEngine:414 14:28:53 INFO - resource:///modules/ContentSearch.jsm:_onMessage:392 14:28:53 INFO - GECKO(2256) | JavaScript error: chrome://browser/content/contentSearchUI.js, line 618: TypeError: eng is undefined 14:28:53 INFO - 111 INFO TEST-PASS | browser/base/content/test/general/browser_contentSearchUI.js | Engine cycled correctly -
Flags: needinfo?(standard8)
Assignee | ||
Comment 17•6 years ago
|
||
I'm stumped. At best it seems a highly frequent intermittent on my machine. If I run browser_contentSearchUI manually, it runs fine. If I use test-verify it takes a few cycles to actually fail. I've checked the engines are added fine, I've also check that latest master passes. The other TypeErrors etc we get also happen when the test passes. I'll take another look on Monday.
Assignee | ||
Comment 18•6 years ago
|
||
After some debugging, it looks like browser_contentSearchUI's method to observe the engine-current notification is sometimes receiving an object that has no type information: https://searchfox.org/mozilla-central/rev/fcd5cb3515d0e06592bba42e378f1b9518497e3d/browser/base/content/test/general/browser_contentSearchUI.js#390-397 The wrappedJSObject is present and has the expected data. Doing a QI on subj also fixes it. I'm a little curious/concerned about why the test changes might have changed that, and why it is only intermittent.
Assignee | ||
Comment 19•6 years ago
|
||
Florian, can you take a look at the last few comments, doing a `subj.QueryInterface(Ci.nsISearchEngine);` just before https://searchfox.org/mozilla-central/rev/fcd5cb3515d0e06592bba42e378f1b9518497e3d/browser/base/content/test/general/browser_contentSearchUI.js#394 is enough to fix it, though I'm a bit concerned that's wallpapering over some other issue.
Flags: needinfo?(standard8) → needinfo?(florian)
Comment 20•6 years ago
|
||
The Engine object from nsSearchService.js doesn't implement nsIClassInfo so its interfaces doesn't get exposed automatically when it's passed around through xpconnect (in this case through the observer service). A QueryInterface call is required for code using the aSubject parameter for the engine-current notification. We do it for example at https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/browser/components/preferences/in-content/search.js#248 But it's worrying if the test sometimes passes without the QueryInterface call.
Flags: needinfo?(florian)
Assignee | ||
Comment 21•6 years ago
|
||
I think I'm going to put this down to possibly different orders of calling observers, and xpconnect remembering the class info in some cases. I'm certainly not changing how the observers work here, so I think it is safe to land the patch with the extra QI.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•6 years ago
|
||
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/91f269ab8495 Add a SearchTestUtils.jsm file and stop duplicating promiseNewSearchEngine. r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/c5afc8caa902 Add a basic test for the follow-on search add-on. r=mikedeboer
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/91f269ab8495 https://hg.mozilla.org/mozilla-central/rev/c5afc8caa902
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in
before you can comment on or make changes to this bug.
Description
•