Closed Bug 1371294 Opened 8 years ago Closed 7 years ago

Add some tests for the follow-on search add-on in mozilla-central

Categories

(Firefox :: Search, enhancement, P2)

Product:
Firefox
Component:
Search
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

Bug 1371294 - Add a SearchTestUtils.jsm file and stop duplicating promiseNewSearchEngine.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1371294 - Add a basic test for the follow-on search add-on.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
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.
Status: NEW → ASSIGNED
This is really P2 until after the main part of 57 is done.
Priority: P1 → P2
Attachment #8967684 - Flags: review?(mdeboer)
Attachment #8967685 - Flags: review?(mdeboer)
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 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 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 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 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+
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
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)
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.
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.
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)
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)
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.
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
https://hg.mozilla.org/mozilla-central/rev/91f269ab8495 https://hg.mozilla.org/mozilla-central/rev/c5afc8caa902
Status: ASSIGNED → RESOLVED
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: