Closed Bug 1358653 Opened 8 years ago Closed 7 years ago

Make it possible to mock appLocales for testing purposes

Categories

(Core :: Internationalization, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: zbraniecki, Unassigned)

References

(Blocks 3 open bugs)

Details

A common scenario in many tests is to do sth like: ``` setLocale('pl'); test(); setLocale('de')(; test2(); ``` That works well with requested locales, as you can just change them at will, but doesn't work well for appLocales since we restrict them to the result of language negotiation. So in order to test how your code reacts to language change, you'd have to add mock packages to chrome registry, and soon to l10nregistry too. I see two potential solutions: 1) Make it possible to switch LocaleService to client mode: ``` LocaleService.setMode('client'); LocaleService.assignAppLocales('pl'); test(); LocaleService.assignAppLocales('de'); test2(); ``` 2) Turn off the negotiation and let LocaleService use any locale from requested: ``` LocaleService.enterTestMode(); LocaleService.setRequestedLocales('pl'); // GetAppLocales == ['pl'] test(); LocaleService.setRequestedLocales('de'); // GetAppLocales == ['de'] test(); ``` :jfkthame, what's your take?
Flags: needinfo?(jfkthame)
Umm... I'm not sure I have any strong opinion here. Actually, I probably don't really understand what the intention is, exactly. I don't know what tests will expect to happen if getAppLocales starts returning locales that aren't really present; that sounds like a scenario that's inherently broken, and normal code shouldn't need to be dealing with it (e.g. if it just throws an error, that's fine). To properly test switching among -available- locales, ISTM you'd need to have a "real" alternative present to switch to -- even if it's as simple as an automatically-generated copy of the en-US locale with some transform applied to all the strings (e.g. pig-latin!) so that the difference can clearly be seen.
Flags: needinfo?(jfkthame)
An example is bug 1357902 - We're moving WebExtensions to use appLocales instead of requestedLocales. But with requestedLocales, they can just easily do ``` LocaleService.setRequestedLocales(['pl']); assert.eq(webext.LanguageCode, "pl"); assert.eq(webext.getUILocale(), "pl"); LocaleService.setRequestedLocales(['de']); assert.eq(webext.LanguageCode, "de"); assert.eq(webext.getUILocale(), "de"); ``` Do you have any suggestion on how such test should work with appLocales?
Flags: needinfo?(jfkthame)
Well... presumably webext.LanguageCode is supposed to be the user's requested language for web content? In that case, appLocales aren't really relevant to it; you should be able to call LocaleService.setRequestedLocales(...) with various args, including both single locales and lists, and check in each case that `assert.eq(webext.LanguageCode, LocaleService.getRequestedLocale())` to confirm that it tracks the requested locale as expected. As for webext.getUILocale(), I assume that should -not- directly follow changes to requestedLocales, but to the result of request/available negotiation. So you'd check something like `assert.eq(webext.getUILocale(), LocaleService.getAppLocaleAsBCP47())` each time. If there's only a single locale installed, this will always be testing against the same single value; to test changes, you'd need an additional locale package to be present so that we can actually switch to it.
Flags: needinfo?(jfkthame)
My point is that it's hard/impossible to test if your API reacts to language change if our test environment is constrained to just one locale ('en-US') which it is. In result, the code like `assert.eq(webext.getUILocale(), LocaleService.getAppLocaleAsBCP47())` works, and does test if your API picks up the right locale, but doesn't allow you to test reacting to locale change scenario. My idea was that for the sake of unit-tests, we could turn off the LocaleService's constrains on appLocales and emulate the successful locale change on requestedLocale change without negotiation inbetween. That would allow customers of LocaleService test if their code reacts to updates to appLocales. I'll keep it open for now in case WebExtensions team bring it up :)
Priority: -- → P3
It came back with bug 1385416 where I had to use getRequestedLocales because of tests that want to check picking the right localized manifest entries from extensions.manifest. The test is assigning different locale and testing if the right entries are picked up. Such tests cannot be written against AppLocales, since that would require registering "global" package for the given locale into ChromeRegistry. Can we revisit this? I believe that inability to mock locale change in order to test code behavior on locale change is going to have negative effect on the code quality since we'll lack tests for those scenarios.
Flags: needinfo?(jfkthame)
AFAICS, the kind of test hooks suggested here (comment 0) could help with low-level testing of APIs like webext.getUILocale(), but ideally we should have tests that cover higher-level behavior as well -- e.g. to confirm that some UI presented by an extension with support for multiple locales actually gets refreshed at the proper time. That would (AFAICS) require running tests with an actual multi-locale browser package, so that we can switch locales during the test and verify that the expected UI changes occur -- which won't happen if we're just returning changed values from LocaleService APIs, but the new locale isn't actually available in the build. Is this sort of testing something we can potentially do?
Flags: needinfo?(jfkthame)
> That would (AFAICS) require running tests with an actual multi-locale browser package, so that we can switch locales during the test and verify that the expected UI changes occur That made me realize that yes, that's exactly what the new L10nRegistry allows for and I'm already using it in tests. For example here: http://searchfox.org/mozilla-central/source/intl/l10n/test/test_localization.js#30 Once we fully switch to L10nRegistry, the problem will disappear. But until then, adding mock resources to ChromeRegistry is either hard or impossible. So we can only do the webext.getUILocale testing :( Would you be ok with adding a way to overload setting availablelocales to a mock value from tests for now? I believe we would remove it once we switch to linking availablelocales to L10nRegistry instead of ChromeRegistry.
Depends on: 1431260
With bug 1431260 in tree there are now two ways to mock available locales: 1) If you want to test that Firefox UI actually handles localization in a given locale, you should create a new FileSource and register it with L10nRegistry: ``` const fs = new FileSource("myMock", ["de", "fr"], "resource://my-module/localization/{locale}/"); L10nRegistry.registerSource(fs); // Test // Cleanup L10nRegistry.removeSource(fs); ``` This will tell L10nRegistry that we now have l10n resources for Firefox in "de" and "fr". You could even mock L10nRegistry.load method (using sinon?) to make it fetch your resources when requested. 2) If you just want to test that your code acts correctly when Firefox is set to a given locale, you should mock setAvailableLocales: ``` const originalLocales = Services.locale.getAvailableLocales(); Services.locale.setAvailableLocales(["de", "fr"]); // Test // Cleanup Services.locale.setAvailableLocales(originalLocales); ``` I'll document it in bug 1438687, but this is a good start. If in doubt, please, NI me and I'll help with the test :)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
The bug title refers to "mock appLocales" but comment 8 seems to refer to "mock available locales." Both seem to match up with Services.locale methods: > Services.locale.getAppLocalesAsLangTags() Array [ "en-US" ] > Services.locale.getAvailableLocales() Array [ "en-US", "de", "fr" ] (this is after running `Services.locale.setAvailableLocales(["de", "fr"])`) Is there a way to get the former (or any of the .getAppLocale* methods) to return something other than "en-US" from a mochitest?
Flags: needinfo?(gandalf)
I'm in the middle of writing documentation for all of that. With ASCII pictures and examples. Sorry it takes so long! For now, comment 8 gives you a couple examples. To reiterate, a simple one for an addon locale testing scenario would be: ``` const originalAvLocales = Services.locale.getAvailableLocales(); const originalReqLocales = Services.locale.getRequestedLocales(); Services.locale.setAvailableLocales(["de", "fr"]); Services.locale.setRequestedLocales(["de", "fr"]); // Here `Services.locale.getAppLocalesAsBCP47` will return ["de", "fr"] // so you can test against them // Cleanup Services.locale.setRequestedLocales(originalReqLocales); Services.locale.setAvailableLocales(originalAvLocales); ``` There's also bug 1440969 which will make it easier in the future by making it possible for addons to not be tied to Firefox!
Blocks: 1440969
Flags: needinfo?(gandalf)
Activity Stream reacts to the locales changed notification, but it looks like that's too early to check? ```js log = msg => console.log(msg, Services.locale.getAppLocalesAsLangTags()); Services.obs.addObserver(() => { log("changed"); setTimeout(() => log("later changed")); }, "intl:requested-locales-changed"); log("before"); Services.locale.setAvailableLocales(["de"]); Services.locale.setRequestedLocales(["de"]); log("synchronous") ``` That prints: before Array [ "en-US" ] changed Array [ "en-US" ] synchronous Array [ "de", "en-US" ] undefined later changed Array [ "de", "en-US" ] So it looks like the notification is sent before it actually changes for `getAppLocalesAsLangTags()`?
Flags: needinfo?(gandalf)
Oh duh. We should watch for app-locales-changed instead ;)
Flags: needinfo?(gandalf)
You need to log in before you can comment on or make changes to this bug.