browser_google_codes.js doesn't work properly for US/non-US

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Search
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

51 Branch
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
In browser_google_codes.js, we are testing the US engine, but it has codes.

This is because that test hardcodes to use google as the engine (when US should not have codes).

The tests should be written so that they properly get codes/no codes from the list.json file, not hardcoded into the test.
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Blocks: 1328713

Comment 2

11 months ago
mozreview-review
Comment on attachment 8831748 [details]
Bug 1335104 - Use search locale logic to test Google codes.

https://reviewboard.mozilla.org/r/108294/#review109914

Thanks for fixing this. The code duplication is unfortunate here, but it's better than having no coverage.
Attachment #8831748 - Flags: review?(florian) → review+
(Assignee)

Comment 3

11 months ago
mozreview-review-reply
Comment on attachment 8831748 [details]
Bug 1335104 - Use search locale logic to test Google codes.

https://reviewboard.mozilla.org/r/108294/#review109914

Yeah, I thought about that, but didn't think it was worth it at this point. IF we ending up creating more tests that need to uninit/init search, I'll definitely move that code out into its own file.
(Assignee)

Comment 4

11 months ago
Unfortunately I just discovered a problem when I retested.

By clearing these prefs:

    Services.prefs.clearUserPref("browser.search.countryCode");
    Services.prefs.clearUserPref("browser.search.region");

The next time the search service is inited for a different test, it hits the location server which causes:

FATAL ERROR: Non-local network connections are disabled and a connection attempt to location.services.mozilla.com (54.154.207.225) was made.

So I need to store and reset them.
Comment hidden (mozreview-request)

Comment 6

11 months ago
mozreview-review
Comment on attachment 8831748 [details]
Bug 1335104 - Use search locale logic to test Google codes.

https://reviewboard.mozilla.org/r/108294/#review109946

::: browser/components/search/test/browser_google_codes.js:108
(Diff revision 2)
>  
>      Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", true);
>  
> -    // Make the new Google the only engine
> +    // Avoid going to the server for the geo lookup. We take the defaults
>      originalGeoURL = Services.prefs.getCharPref(BROWSER_SEARCH_PREF + kUrlPref);
> -    let geoUrl = 'data:application/json,{"interval": 31536000, "settings": {"searchDefault": "Google", "visibleDefaultEngines": ["google"]}}';
> +    let geoUrl = 'data:application/json,{}';

This will likely fail eslint, strings should use " except when using ' is a way to avoid needing to escape the " character.
Comment hidden (mozreview-request)
(Assignee)

Comment 8

11 months ago
Florian: I've fixed your changes in the patch in mozreview.

Want to doublecheck?
(In reply to Mike Kaply [:mkaply] from comment #8)
> Florian: I've fixed your changes in the patch in mozreview.
> 
> Want to doublecheck?

Looks good to me.

Comment 10

10 months ago
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/81bd515bd9d9
Use search locale logic to test Google codes. r=florian

Comment 11

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/81bd515bd9d9
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.