Create unit tests for testing locale specific search engines

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Search
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mkaply, Assigned: florian)

Tracking

Trunk
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: [test] fxsearch)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
We have a google engine that is in the tree, but it is not used by the default locale.

As a result, it doesn't get tested.

We need to add a unit test that uses the geo specific code to add the hidden engine and verify it.
(Reporter)

Comment 1

2 years ago
Created attachment 8744393 [details] [diff] [review]
First testcase pass

This is my first pass at a unit test.

It borrows heavily from:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_hidden.js?force=1
Attachment #8744393 - Flags: feedback?(florian)
(Reporter)

Updated

2 years ago
Attachment #8744393 - Attachment is patch: true
Attachment #8744393 - Attachment mime type: application/x-javascript → text/plain
(Reporter)

Comment 2

2 years ago
Created attachment 8744401 [details]
MozReview Request: Bug 1266783 - Initial patch at unit test

Review commit: https://reviewboard.mozilla.org/r/48519/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48519/
(Assignee)

Comment 3

2 years ago
https://reviewboard.mozilla.org/r/48519/#review45227

Looks reasonable overall.

::: browser/components/search/test/browser_google_codes.js:57
(Diff revision 1)
> +          .observe(null, "nsPref:changed", kLocalePref);
> +
> +  return promise;
> +}
> +
> +add_task(function* () {

The split in 3 parts would be more readable if you gave the tasks explicit names, eg:
 add_task(function* preparation() {

::: browser/components/search/test/browser_google_codes.js:59
(Diff revision 1)
> +  return promise;
> +}
> +
> +add_task(function* () {
> +  const kUrlPref = "geoSpecificDefaults.url";
> +  const BROWSER_SEARCH_PREF = "browser.search.";

These 2 constants should be shared at the top of the file.

::: browser/components/search/test/browser_google_codes.js:72
(Diff revision 1)
> +
> +  removeCacheFile();
> +
> +  // Geo specific defaults won't be fetched if there's no country code.
> +  Services.prefs.setCharPref("browser.search.geoip.url",
> +                             'data:application/json,{"country_code": "US"}');

Should we clear this at the end of the test?

::: browser/components/search/test/browser_google_codes.js:75
(Diff revision 1)
> +  // Geo specific defaults won't be fetched if there's no country code.
> +  Services.prefs.setCharPref("browser.search.geoip.url",
> +                             'data:application/json,{"country_code": "US"}');
> +
> +  Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", true);
> +                             

Please ensure you remove all the trailing whitespace for the final version of the patch.

::: browser/components/search/test/browser_google_codes.js:128
(Diff revision 1)
> +  // Verify search service is not initialized
> +  is(Services.search.isInitialized, false, "Search service should NOT be initialized");
> +  removeCacheFile();
> +
> +  // Geo specific defaults won't be fetched if there's no country code.
> +  Services.prefs.clearUserPref("browser.search.geoip.url");

This line doesn't do anything, the url is on the default branch.

::: browser/components/search/test/browser_google_codes.js:135
(Diff revision 1)
> +  Services.prefs.clearUserPref("browser.search.geoSpecificDefaults");
> +
> +try {
> +// We can't set the pref back to the original because it references search.services.mozilla.com
> +// which causes the tests to fail.
> +//  Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF).setCharPref(kUrlPref, originalGeoURL);

This line looks correct, why is it causing test failures? Services.prefs.clearUserPref("browser.search.geoSpecificDefaults"); should disable geo-defaults, right?
(Reporter)

Comment 4

2 years ago
> This line looks correct, why is it causing test failures? Services.prefs.clearUserPref("browser.search.geoSpecificDefaults"); should disable geo-defaults, right?

That's what I thought, but when the search service is reinited by the next test (browser_healthreport), I get this failure:

FATAL ERROR: Non-local network connections are disabled and a connection attempt to search.services.mozilla.com (52.10.203.178) was made.
You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.
TEST-INFO | Main app process: exit 1
TEST-UNEXPECTED-FAIL | browser/components/search/test/browser_healthreport.js | application terminated with exit code 1

Another test before that fails as well:

537 INFO TEST-UNEXPECTED-FAIL | browser/components/search/test/browser_healthreport.js | Current engine is Foo - Got Google, expected Foo

So clearly my cleanup isn't working as expected...
(Reporter)

Comment 5

2 years ago
browser.search.geoSpecificDefaults defaults to true.

It's set to false for testing.

We set it to true for our testing, so when we reset the value, it goes back to true.

IT should go back to false...
(Assignee)

Comment 6

2 years ago
Ah, I missed that http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js is setting prefs on the user branch.
(Reporter)

Comment 7

2 years ago
Created attachment 8744467 [details] [diff] [review]
Updated patch with comments addressed

OK, I think I got it all figured out.

The one weird thing I had to do was to cause a Sync reinit of search at the end, not an async reinit.

Async reinit reused the Google stuff from the testcases, which broke all the testcases after.
Attachment #8744393 - Attachment is obsolete: true
Attachment #8744401 - Attachment is obsolete: true
Attachment #8744393 - Flags: feedback?(florian)
Attachment #8744467 - Flags: review?(florian)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8744467 [details] [diff] [review]
Updated patch with comments addressed

Review of attachment 8744467 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall, but having to force a sync re-init doesn't sound great, especially if we don't really know why it's needed.

::: browser/components/search/test/browser_google_codes.js
@@ +128,5 @@
> +
> +  Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF).setCharPref(kUrlPref, originalGeoURL);
> +
> +  // Force sync reinit
> +  // Async reinit reuses the stored Google information

I would like to understand why. Seems like a bug.

@@ +129,5 @@
> +  Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF).setCharPref(kUrlPref, originalGeoURL);
> +
> +  // Force sync reinit
> +  // Async reinit reuses the stored Google information
> +  is(Services.search.currentEngine.name, "Google", "Search engine should be back to Google");

I don't think we want to hardcode the current default in a test that isn't actually about testing that. And are you sure it's always google? Is there something forcing isUS to be false?
Attachment #8744467 - Flags: feedback+
(Reporter)

Comment 9

2 years ago
> I would like to understand why. Seems like a bug.

I'll do some more looking. I'm pretty stumped.

> I don't think we want to hardcode the current default in a test that isn't actually about testing that. And are you sure it's always google? Is there something forcing isUS to be false?

Honestly, just having Services.search.currentEngine is all we need to cause the reinit of the search service.

So I don't need a check at all.
(Reporter)

Comment 10

2 years ago
Here's what happens after my test during the asyncReinit:

1128 INFO Console message: _asyncReInit
1129 INFO Console message: finalizing batch task
1130 INFO Console message: batchTask: Invalidating engine cache
1131 INFO Console message: _buildCache: Writing to cache file.
1132 INFO Console message: _asyncReInit
1133 INFO Console message: _init: Initing search plugin from [app]/chrome/en-US/locale/browser/searchplugins/google.xml
1134 INFO Console message: _parseImage: Image textContent: "data:image/x-icon;base64,AAABAAIAEBAAAAEAIABoBAAAJgAAACAgAAABACAAqBAAAI4EAAAoAAAAEAAAACAAAAABACAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA..."
1135 INFO Console message: _setIcon: Setting icon url "data:image/x-icon;base64,AAABAAIAEBAAAAEAIABoBAAAJgAAACAgAAABACAAqBAAAI4EAAAoAAAAEAAAACAAAAABACAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA..." for engine "Google".
1136 INFO Console message: _addEngineToStore: Adding engine: "Google"
1137 INFO Console message: _asyncLoadEngines: loading user-installed engines from the obsolete cache
1138 INFO Console message: _buildCache: Writing to cache file.
MEMORY STAT | vsize 4517MB | residentFast 479MB | heapAllocated 194MB
1139 INFO TEST-OK | browser/components/search/test/browser_google_codes.js | took 54ms

Looking at the code, it seems to be going to the cache file even though I am explicitly removing it.
(Reporter)

Comment 11

2 years ago
it's looking like race condition. When I start instrumenting search service, it goes away...
(Assignee)

Comment 12

2 years ago
Created attachment 8744969 [details] [diff] [review]
Attempt to debug

MozReview-Commit-ID: IpVnoRvjE9n
(Assignee)

Updated

2 years ago
Assignee: mozilla → florian
Status: NEW → ASSIGNED
(Reporter)

Comment 13

2 years ago
I'm to a point where I pass intermittently (by removing the Atomic TMP file).

The difference between passing and not passing is that if you do a check to see if the file exists after the file.remove in removeCacheFile, it didn't actually go away.

So I think we definitely have a race condition here where the cache file is still in use and doesn't delete.
(Assignee)

Updated

2 years ago
Assignee: florian → mozilla

Updated

2 years ago
Priority: -- → P1
Whiteboard: [test] fxsearch
(Assignee)

Comment 14

2 years ago
Created attachment 8746550 [details] [diff] [review]
strace diff between a pass and a fail

On my mac the test fails all the time for me. On Linux opt, it passes most of the time (fails about once in 50 runs). On Linux debug, it fails about half the time, which makes looking at strace for both cases possible. Attached is a diff of the strace for a passing and a failing run on the linux debug build.

The passing run is writing to the cache file one more time before we unlink it.
(Assignee)

Comment 16

2 years ago
Created attachment 8746786 [details] [diff] [review]
Add unit tests for Google engine with codes,

This is Mike's test that I've finished debugging and cleaned up a little bit, so r=me on this attachment.
Attachment #8746786 - Flags: feedback?(mozilla)
(Assignee)

Updated

2 years ago
Attachment #8744969 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Assignee: mozilla → florian
(Assignee)

Updated

2 years ago
Attachment #8744467 - Attachment is obsolete: true
Attachment #8744467 - Flags: review?(florian)
(Assignee)

Comment 17

2 years ago
Created attachment 8746794 [details] [diff] [review]
avoid double cache flush

The _buildCache method is called either at the end of the initialization of the search service when the cache has to be rebuilt, or from a DeferredTask armed by any metadata change. Our test here was (sometimes) failing because after a re-initialization of the search service, the cache was written to the disk twice: once immediately at the end of the re-initialization, and a second time from the DeferredTask. The test was waiting for the cache to be flushed to disk before continuing, but wasn't waiting for a second flush... which could sometimes inconveniently happen right after we attempted to clear the cache by removing the file.

This double flush of the cache is completely useless, and the straightforward solution is to always disarm the DeferredTask (if it exists) from the _buildCache method.
Attachment #8746794 - Flags: review?(adw)
(Assignee)

Comment 18

2 years ago
Comment on attachment 8746786 [details] [diff] [review]
Add unit tests for Google engine with codes,

r=me, the feedback request on Mike is just to double check that the changes I made make sense.
Attachment #8746786 - Flags: review+

Updated

2 years ago
Attachment #8746794 - Flags: review?(adw) → review+
(Reporter)

Comment 19

2 years ago
Don't forget that for this to pass, it needs the new google.xml from release and for that to work, the other two google tests have to be updated as well. I'll attach a patch for the test update.
(Reporter)

Comment 20

2 years ago
Ignore that last comment. Those two tests would use google-nocodes.

Note to future self - artifact builds use the wrong list.txt, so tests don't work properly.
(Reporter)

Comment 21

2 years ago
Everything is testing great for me on fx-team.
(Assignee)

Comment 22

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/1647dd64832e5247d47340aefab91126a3267ef2
Bug 1266783 - prevent the search cache file from being flushed twice in a row after a re-initialization, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/70443ad6bcc6c58f5e76ed71bf3f2bd075d17be8
Bug 1266783 - Add unit tests for Google engine with codes, r=florian.

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1647dd64832e
https://hg.mozilla.org/mozilla-central/rev/70443ad6bcc6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
(Assignee)

Comment 24

2 years ago
Comment on attachment 8746786 [details] [diff] [review]
Add unit tests for Google engine with codes,

Approval Request Comment
[Feature/regressing bug #]: bug 1264786
[User impact if declined]: The changes to the Google search plugin landed in bug 1264786 may regress.
[Describe test coverage new/current, TreeHerder]: Current test coverage: none. The point of this patch is to add test coverage.
[Risks and why]: none for this specific patch as the change is tests-only, but we'll need the low risk associated search service patch.
[String/UUID change made/needed]: none.
Attachment #8746786 - Flags: feedback?(mozilla)
Attachment #8746786 - Flags: approval-mozilla-beta?
Attachment #8746786 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 25

2 years ago
Comment on attachment 8746794 [details] [diff] [review]
avoid double cache flush

Approval Request Comment
[Feature/regressing bug #]: Unclear, but likely a regression from bug 1203167, in the sense that the affected code didn't really exist or behave that way before bug 1203167.
[User impact if declined]: For users, small overhead during the first startup, due to pointlessly writing the search cache file a second time. This patch is required to make the browser_google_codes.js test pass consistently.
[Describe test coverage new/current, TreeHerder]: No test coverage for this specific change, but the code area it touches has a lot of xpcshell tests.
[Risks and why]: Low risk: self contained patch that will only prevent writing the search cache file twice in a row.
[String/UUID change made/needed]: none.
Attachment #8746794 - Flags: approval-mozilla-beta?
Attachment #8746794 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 26

2 years ago
Note: the patch from bug 1181645 should be uplifted at the same time as the patches here.
Comment on attachment 8746786 [details] [diff] [review]
Add unit tests for Google engine with codes,

Test changes are auto approved and do not need release management review.
Attachment #8746786 - Flags: approval-mozilla-beta?
Attachment #8746786 - Flags: approval-mozilla-aurora?
Comment on attachment 8746794 [details] [diff] [review]
avoid double cache flush

Search plugin related changes, Beta47+, Aurora48+
Attachment #8746794 - Flags: approval-mozilla-beta?
Attachment #8746794 - Flags: approval-mozilla-beta+
Attachment #8746794 - Flags: approval-mozilla-aurora?
Attachment #8746794 - Flags: approval-mozilla-aurora+

Updated

2 years ago
status-firefox47: --- → affected
You need to log in before you can comment on or make changes to this bug.