Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the capability to change the language between tests #873

Merged
merged 38 commits into from
Sep 29, 2018

Conversation

luisnaranjo733
Copy link
Contributor

This PR implements the capability to change the language between tests for iOS by adding flags to xcrun. It looks like this is straightforward for Android as well. I didn't make that change because I'm not set up to verify it works on Android at the moment. I'm hoping someone else can make a change in my branch, or can just make another PR later.

Basically I just added a new optional language field to the params object that gets passed to device.launchApp().

Example usage:

init.js

beforeAll(async () => {
  await detox.init(config, {launchApp: false});
});

test.spec.js

const LOCALES = ["es-MX", "fr-FR"];

LOCALES.forEach(locale => {

  describe("Run tests in various languages", () => {

    beforeAll(async () => {
      await device.launchApp({
        newInstance: true,
        language: locale
      });
    });

    beforeEach(async () => {
      await device.reloadReactNative();
    });

    it(`Example test in ${locale}`, doStuff());

  });


});

@LeoNatan
Copy link
Contributor

LeoNatan commented Aug 3, 2018

Thanks for this PR.

Locale is something different from language and it is incorrect to tie them together in all cases. I think it would be better to give an option for providing a different locale than the language, and if no locale is provided, use the system one (nothing passed, like it is now for language).

@@ -86,7 +86,7 @@ class AndroidDriver extends DeviceDriverBase {
}
}

async launch(deviceId, bundleId, launchArgs) {
async launch(deviceId, bundleId, launchArgs, language) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a locale option as well.

@rotemmiz
Copy link
Member

rotemmiz commented Aug 5, 2018

Thanks for taking the time to submit this PR!

There are a few things that need to be resolved before we can continue:

  1. Regarding the API, Android must be taken into account. It seems to not be straightforward (as far as I tested). I'm not sure these params can be passed at launch. There are a few resources regarding changing locale during tests
    https://developer.android.com/guide/topics/resources/localization#testing
    https://stackoverflow.com/questions/16760194/locale-during-unit-test-on-android
  2. Every feature must be tested with the test app. This means, that we need to add language support to the test app, and use the new API to test it actually triggers language/locale change in the app.
  3. @LeoNatan's comment is very important, and on top of that, the two platforms seem to construct the locale/language/country in a slightly different way.
    Android: Locale locale = new Locale(language, country); "de", "DE"
    iOS: -AppleLanguages "(de)" or -AppleLocale "fr_FR"

@LeoNatan
Copy link
Contributor

LeoNatan commented Aug 5, 2018

Testing on the test suite can be very easy to handle: Just put the app language and app locales to text fields and test that they are correct.

@LeoNatan
Copy link
Contributor

LeoNatan commented Aug 5, 2018

@rotemmiz Regarding API, I am not sure I understand.

In iOS settings, setting the country is really setting the locale. So en_GB, en_CA, en_US, de_DE, etc. Setting the language changes the interface language, but it's also given in the same notation, so en_GB, en_CA, en_US, de_DE, etc. It might also accept shorter notation ("de") and that is mapped to a default country.

In Android, is there no way to define the locale different than device/app language? Seems like a big design flaw as the two are really unrelated.

@luisnaranjo733
Copy link
Contributor Author

Hey, just thought I'd pop in and give an update -

Basically I haven't had any bandwidth to work on this in a while. I'm hoping I'll have some time in 1-2 weeks. If anyone else is free to work on this, feel free to fork my fork or work directly in my fork (I'll grant anyone access that asks me).

I still plan on doing my best to address all the requested changes and feedback, just haven't had any time :(

@luisnaranjo733
Copy link
Contributor Author

luisnaranjo733 commented Sep 9, 2018

Hi again everyone. Finally had some time to come work on this. I just pushed an update to address some of the previous feedback. A few things to note:

  • The links that @rotemmiz shared were promising, but it turns out that technique requires a rooted device, even on an emulator.
luisnaranjo: ~/code/detox/detox/test$ adb shell
generic_x86:/ $ setprop persist.sys.locale fr-CA;stop;sleep 5;start
setprop: failed to set property 'persist.sys.locale' to 'fr-CA'
stop: must be root
start: must be root
  • This technique seems like the most feasible option to accomplish changing the locale programmatically without rooting the device. It would require installing a 3rd party app, and then deep linking to it via adb to change the system language. I decided not to implement Android for now, because I think we should discuss what the best option is before moving forward.

  • Updated unit tests to maintain 100% code coverage

  • Added language support to the test app (new language screen that tells you the system language/locale)

    • I was hoping to use the react-native-device-info package on both platforms, but something is wrong with the automatic native module linking on iOS. I had the package working on Android, but not iOS. Luckily, react-native itself has language+locale support for iOS (and partially Android). I decided to strip out this 3rd party package and instead rely on the built in API.
  • Added an e2e test to test the new API. It passes on iOS. Obviously doesn't pass on Android because the feature isn't implemented. I decided to comment it out for now for the sake of passing CI.

I propose that we go forward with implementing this on iOS, and file a separate issue to add Android support later. We can document this as an iOS only feature for the time being.

What are your thoughts? @LeoNatan @rotemmiz

@luisnaranjo733
Copy link
Contributor Author

luisnaranjo733 commented Sep 10, 2018

BTW, I've tested that 3rd party app and can confirm that that technique works as advertised.

Here is one approach we could take for implementing this on Android

  1. Check this apk into the Detox source code. Document its usage and provide credit to the author
  2. Use adb to install this APK and grant it CHANGE_CONFIGURATION permission when this language API is invoked
adb install path/to/adb-change-language.apk
adb shell pm grant net.sanapeli.adbchangelanguage android.permission.CHANGE_CONFIGURATION
  1. Change the language by deeplinking to this 3rd party app via adb
adb shell am start -n net.sanapeli.adbchangelanguage/.AdbChangeLanguage -e language pt-rBR
  1. The API would need to be slightly different for Android. The deep-linking syntax for this language switching app takes a single 'language' parameter in language:locale syntax (e.g. "es-MX"). As @LeoNatan pointed out, the language/locale system in Android is not as robust as iOS.

I still think this should be done in a separate PR, but I just wanted to share how I think this could be achieved :)

@luisnaranjo733
Copy link
Contributor Author

luisnaranjo733 commented Sep 22, 2018

Oops - accidentally closed the PR but I've re-opened it.
@LeoNatan and @rotemmiz I'd love to get some feedback on this when you have some time.

@luisnaranjo733
Copy link
Contributor Author

Looks like the CI results went from passing to failing when I closed and re-opened the PR.
Some of the e2e animations started failing. Anyone have any idea why? Seems completely unrelated.

22:21:21 ✕ should detect loops with final number of iterations (driver: Native) (481822ms)
22:21:21 ✕ should not wait during delays longer than 1.5s (driver: Native) (960341ms)
22:21:21 ✕ :ios: should wait during delays shorter than 1.5s (driver: Native) (960301ms)

@LeoNatan
Copy link
Contributor

I am OK with this change. @luisnaranjo733 Could you please update documentation with the new API? Thanks

@luisnaranjo733
Copy link
Contributor Author

Awesome! I just merged the latest from master into my branch, and confirmed that unit and e2e tests are still passing.

I've also added some doc changes to the device API page.

@LeoNatan LeoNatan merged commit 50ee994 into wix:master Sep 29, 2018
@LeoNatan
Copy link
Contributor

Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Oct 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants