Closed Bug 1371294 Opened 3 years ago Closed 2 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: 2 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.