Running on windows support #628

simonbuchan · 2018-03-19T01:02:49Z

Just a few issues I ran into running on windows. Boils down to:

npm scripts (that might run on windows) should be either:
- node some/script.js (what I've done) or
- add cross-env dev dep, and use cross-env some/script where there is a some/script with a #! and some/script.bat written for windows cmd.exe. This one is no fun.
A few cases of exec('node_modules/.bin/foo'...) which doesn't work in cmd.exe (it tries to run node_modules with /.bin/foo). Not quite sure about my fixes for these, I'm happy to change things.
several cases of .split('\n'), which broke at least starting the emulator (looking for foo, not finding foo\r) - fixed this at the source, by replacing \r\n on windows.
android.emulator (as opposed to .attached) would start emulator.exe even if it was already running which was for some reason causing it to get stuck (~~probably due to the output redirection to the log file~~ looks like some weird emulator internal issue) - workaround the issue by first checking for a running device matching the name.

I also noticed detox/scripts/build.sh android was a little out of date while porting, is that code still used?

Been running this for a while now and everything seems to be good, but there will always be something hiding.

simonbuchan · 2018-03-19T01:05:28Z

Also I didn't touch detox-cli - I'm not quite sure what the point is? yarn <bin> and npx <bin> obsolete packages like that. I'm happy to port it to node or batch and test if you like?

DanielMSchmidt · 2018-03-19T10:06:50Z

@simonbuchan Thank you for taking care of this <3 👍
Would you mind adding some docs? Do you know if we can test some of these things in our CI? Do we need another solution there?

simonbuchan · 2018-03-19T10:31:01Z

There's a lot more rough edges I'd like to iron out first, e.g. when using android.emulator it gets stuck
trying to start the emulator again on the second run (not sure what's happening there), and I'm not sure how to approach the build config step yet, but these get rid of the big blockers for the basic -> detox test flow.

I've got a couple of hacks that could make all the tests pass on windows (all just path separators, AFAICT), but I don't know if that means they actually work right! At least until I've gotten on top of those I'm not sure where a windows CI would help, nor how that would work. You might have noticed I had to ignore the windows branch in exec for coverage to pass, and I'm not sure how that would work cross-platform (collect on all platforms and merge in a final step? Sounds complex!)

So far not sure what docs would change? "Windows will probably mostly work" isn't a great line! And I'd like to see some other users experience with this first before promising more. The big difference would probably be whatever happens with build.

Ideally we will be using this cross-platform quite a bit more in the future, and I'll be happy to follow this up with more support on request (though keep in mind I'm in UTC+12:00).

simonbuchan · 2018-03-19T10:32:23Z

Talking of CI, actually, I noticed there's a pretty complex eslint config that is not being used or followed at all? What's up with that, and should I be following it in scripts?

simonbuchan · 2018-03-20T06:05:01Z

Let me know if the above fix is OK in theory, it seems to be required for windows android.emulator (vs .attached).

rotemmiz · 2018-03-20T09:53:25Z

This is something we'll have to test locally, there's no test in CI that verifies that a second e2e works, or is there....

The demo-projects CI job runs two subsequent tests on two different projects, with the same devices.
but there still is no Android E2E in CI, we're currently working on moving to our own CI solution (Jenkins with Mac virtual machines capable of running x86 emulators for those who ask)

simonbuchan · 2018-03-20T09:56:32Z

It seems like it does, since it still has 100% coverage? I might be misunderstanding branch coverage.

rotemmiz · 2018-03-21T06:52:15Z

I will be reviewing this soon, sorry for the delay.

tachtevrenidis · 2018-04-16T21:03:59Z

is this going to be merged soon? :-)

rotemmiz · 2018-04-16T12:38:57Z

detox/src/devices/EmulatorDriver.js

+      await this.emulator.boot(name);
+    }
+
+    adbDevices = await this.adb.devices();


Why is this being run twice ?
Does the original implementation cause issues with Windows ?

Oops, should be inside the if here - it's refreshing adbDevices after the requested name is booted. See the comments above as to why this is happening, but roughly: on windows trying to start the emulator the way it currently is being done when it's already running causes issues for some reason.

rotemmiz · 2018-04-17T18:44:26Z

detox/local-cli/detox-run-server.js

 const path = require('path');

 program.parse(process.argv);

-if (fs.existsSync(path.join(process.cwd(), 'node_modules/.bin/detox-server'))) {


npm and yarn install detox-server at different locations.
it might be either it node_modules or in node_modules/detox/node_modules

I am not familiar with require.resolve('detox-server/package.json'); trick. Will it cover both cases ?

Yep, it's the standard hack to find the location of an NPM package the same way node does, since every NPM package has a package.json. Remember:

require / require.resolve('foo/bar') looks up foo in node_modules, then adds bar to that path

without the .../bar it would use the main field, which is likely to be in a nested subdir

package.json always exists.

rotemmiz · 2018-04-17T18:45:39Z

detox/scripts/postinstall.js

@@ -0,0 +1,8 @@
+#!/usr/bin/env node
+
+// Javascript for windows support.


Unnecessary comment

More clearly Using JS instead of shell script to avoid breaking on windows, but I'm happy to remove it!

rotemmiz · 2018-04-17T18:50:15Z

Sorry for the delay in reviewing, but it is now done :)

Unfortunately, we have no capacity to run Detox on Windows in CI, therefore I cannot guarantee this will continue working without breaking. We will need to add a disclaimer in the docs, and rely on the community to report issues and fix them. :(
Not ideal, but this is the best we can do right now.

simonbuchan · 2018-04-18T06:49:12Z

If you want to look into CI later, the standard I've seen in open source is https://www.appveyor.com/

simonbuchan · 2018-04-18T07:17:13Z

Looking around, seems like AppVeyor is mostly NuGet packages. Node packages (including typescript, babel and react-native itself) all use https://circleci.com/

simonbuchan · 2018-04-18T07:43:49Z

CI failure

Hmm, not sure what's up: only one config, so seems like it flaked?

CI

Blorg, never mind, CircleCI is mac/linux only.

Looks like:

Typescript doesn't run CI on windows? They have CircleCI and Travis running, but both are linux.
Same for babel
react-native only have circleci.

Weird. Seems like AppVeyor do have decent-ish node support, even if the docs are old. I could look into setting this up, I think the main thing would be installing the android SDK on windows reliably.

Tests

Note that the tests do not all pass on windows yet - here's an example, all 4 failures are trivial path things like this:

  ● APKPath › simple path

    expect(received).toEqual(expected)
    
    Expected value to equal:
      "~/somePath/build/outputs/apk/androidTest/debug/app-debug-androidTest.apk"
    Received:
      "androidTest\\debug\\app-debug-androidTest.apk"

       9 |     const inputApkPath = '~/somePath/build/outputs/apk/debug/app-debug.apk';
      10 |     const expectedTestApkPath = '~/somePath/build/outputs/apk/androidTest/debug/app-debug-androidTest.apk';
    > 11 |     expect(APKPath.getTestApkPath(inputApkPath)).toEqual(expectedTestApkPath);
      12 |   });
      13 | 
      14 |   it(`path for a gradle build flavor`, async () => {
      
      at Object.it (src/devices/android/APKPath.test.js:11:50)

I'm not sure what you want to do here, some options are:

switch app and tests to consistently using require('path').posix - these paths normally work fine on windows (except for the executable, hence the binPath changes)
have the tests .replace(/\\\\/g, '/'), and get rid of ~ usage.
separate windows and posix path tests.
input / expected paths based on platform
leave it how it is for now, follow up on it later

I do have a Mac Mini for iOS dev now (it's absolutely terrible to work on, but it works 🤷), so I can be more confident I'm not breaking things, I guess.

rotemmiz · 2018-04-19T20:28:52Z

Re tests, it sounds reasonable to just use Node's compatibility layer.
BTW, we intend to move our CI to a home brewed Jenkins server on top of MacStadium infrastructure next week, no Windows support is planned though.

simonbuchan · 2018-04-23T03:58:28Z

Re tests, it sounds reasonable to just use Node's compatibility layer.

Are you referring to require('path').posix? (It's not really a compatibility layer, but the implementation of path used by default on POSIX, there's a .win32 also) - that would be pretty easy, but it's not really correct so it might break stuff. In particular it means the win32 absolute path C:\foo\bar\baz\file.apk gets treated as a relative path, and other bad stuff due to \ not being a path separator, but that's solvable by replacing \ with / on user input.

Another, cleaner answer would be to change the tests so they look something like:

const inputApkPath = path.join(_dirname, 'build/outputs/apk/debug/app-debug.apk');
const expectedTestApkPath = path.join(_dirname, 'build/outputs/apk/androidTest/debug/app-debug-androidTest.apk');

but this means the actual diffs depend on the repo working directory path.

Use path module in tests to normalize paths. Use process.platform to test with actual absolute paths on windows.

simonbuchan · 2018-04-24T07:33:26Z

OK updated tests - should be safe and straightforward changes.

Were there specific things you wanted in terms of doc changes? At the moment, all I can think of is something like "windows should probably work, but be aware that binaryPath and build might not work cross-platform" - eg. cd android && ./gradlew assembleDebug assembleAndroidTest will not work on windows (it needs to be gradlew, no ./) That doesn't seem enough for a whole page, where would you put that?

Other than that I think this is good to go modulo follow-up reviews.

simonbuchan · 2018-04-24T09:43:25Z

While writing some docs noticed I hadn't gotten around to detox-cli (which I don't use myself, because yarn/npx detox works fine for me).

Let me know if these docs are OK, I'm not 100% on the tone or placement.

Hopefully that CI failure was just a flake? Not sure how I could have broken it.

simonbuchan · 2018-04-24T09:45:39Z

Incidentally, while testing build hit the fact that react-native's :ReactAndroid gradle project doesn't seem to even configure on windows, meaning detox/test can't be built. 🙁

rotemmiz · 2018-04-24T09:51:27Z

We have two flavors for the test app, fromBin and fromSource. The thing is, even if you build from bin, the fact that the settings.gradle includes ReactAndroid source, it will evaluate its build scripts.

You can remark it in settings.gradle and build.gradle, and build the fromBin flavour.

Tell me if it works.

simonbuchan · 2018-04-24T10:06:35Z

Yeah. that seems to work fine, and the tests pass. (It was complaining about a missing repositoryUrl on the :ReactAndroid project, don't suppose that makes any sense to you?)
Meh, It's only a problem if you want Windows CI at some point.

rotemmiz · 2018-05-06T12:06:23Z

detox/src/devices/Device.test.js

  });

  it(`should accept relative path for binary`, async () => {
    const actualPath = await launchAndTestBinaryPath('relativePath');
-    expect(actualPath).toEqual(`${process.cwd()}/abcdef/123`);
+    expect(actualPath).toEqual(path.join(process.cwd(), 'abcdef/123'));


Shouldn't this be split to path.join(process.cwd(), 'abcdef', '123') ?

Path will normalise separators, and windows in fine with either except for binary paths (where they are ambiguous with switches)

rotemmiz · 2018-05-06T12:08:21Z

detox/src/devices/EmulatorDriver.js

@@ -52,10 +52,18 @@ class EmulatorDriver extends AndroidDriver {
    }

    await this._fixEmulatorConfigIniSkinName(name);
-    await this.emulator.boot(name);


What was the issue here ? boot should return a valid device, either it's shutdown or already booted. If it doesn't we may have issue with Emulator.js

I now see the comments here.

Not sure how to proceed here. This fix is in bad taste IMO.
So the real issue is creating and removing those emulator log files ?

I could experiment a bit, I suspect redirecting to different files should fix it, but this was a less user visible change (eg. .gitignore)
Do you not find it logical to only start an emulator if it's not already running?

Nope, gave it a try and it still hangs on startup with no output to the log file even if it's a new file each time.
Killing the first emulator then will hang it, killing it causes the second to start up and detox to continue successfully.
Seems like there's something lower level specific to the android emulator going on, possibly needs to have a separate AVD directory?

rotemmiz · 2018-05-06T12:08:51Z

docs/More.Roadmap.md

@@ -11,6 +11,14 @@ The current API supports addition of multiple platforms, Android is next. This i
 ### iOS physical device support
 Currently detox only supports running on iOS simulators, we plan on adding support for running on devices as well.

+### Windows support
+There is some work done for running detox on Windows, but it's still fairly untested. Please open issues for anything you run into, but be aware of these limitations:


Detox, not detox

rotemmiz · 2018-05-06T12:09:41Z

docs/More.Roadmap.md

+- `binaryPath` can be left as a relative path with `/`, or use `\\` if you don't need cross-platform support.
+- `build` should not use `./gradlew ...`, but simply `gradlew ...` - you may prefer scripting the build outside of
+  detox if you want to maintain cross-platform support - or simply have two configurations!
+


LeoNatan · 2018-05-07T09:13:32Z

docs/More.Roadmap.md

+- Apple doesn't support iOS apps on Windows, obviously, so you're limited to the in-progress Android support.
+- `binaryPath` can be left as a relative path with `/`, or use `\\` if you don't need cross-platform support.
+- `build` should not use `./gradlew ...`, but simply `gradlew ...` - you may prefer scripting the build outside of
+  detox if you want to maintain cross-platform support - or simply have two configurations!


Why is there a line break here?

Also, should be “Detox”.

LeoNatan · 2018-05-07T09:14:38Z

docs/More.Roadmap.md

+### Windows support
+There is some work done for running Detox on Windows, but it's still fairly untested. Please open issues for anything you run into, but be aware of these limitations:
+
+- Apple doesn't support iOS apps on Windows, obviously, so you're limited to the in-progress Android support.


Let’s please remove the “obviously”.

simonbuchan · 2018-05-07T09:48:07Z

Gah. 🤦‍♂️ Thanks for the reviews guys!
@rotemmiz If the emulator boot is an issue, I could only skip the second boot on win32? I suspect passing a temp dir to -datadir would also work, but that will very likely break people (but would be a neat option to know you're getting a fresh OS install each time / run in parallel?)

simonbuchan · 2018-05-07T10:00:16Z

Hah, just looked the jenkins log and saw a big fat crash stack at the end... then noticed it was the app crash test 😅

# Conflicts: # detox/.eslintrc

simonbuchan · 2018-05-25T09:50:07Z

🎉🕊 Thanks so much for all the discussion and feedback! This turned out a lot bigger and gnarlier than I expected at the start, but isn't that always the way!

LeoNatan · 2018-05-25T09:51:25Z

Thank you for all the good work!

rotemmiz · 2018-05-25T10:13:41Z

Thanks for the great work, hope you stick around.

simonbuchan added 3 commits March 19, 2018 12:57

Update package scripts to use node for cross-plattform.

989e45e

Fix detox test / run-server using paths that don't work on windows.

b950f2a

Replace CRLFs in exec.js, fixes running the emulator on windows.

5982396

simonbuchan requested review from LeoNatan and rotemmiz as code owners March 19, 2018 01:02

simonbuchan added 5 commits March 19, 2018 15:31

Should fix build.js on mac.

4430484

Remove some paranoia.

9093c2d

Make eslint pass on JS scripts.

a6169b2

Ignore exec hack in coverage to get CI passing.

a50ebe7

Walk back detox test change so it works with detox-cli.

3288a98

Don't try to start emulator if it's already running.

2aace19

LeoNatan mentioned this pull request Mar 20, 2018

fix: post install script failure on windows #631

Closed

rotemmiz requested changes Apr 17, 2018

View reviewed changes

simonbuchan added 2 commits April 18, 2018 18:54

Merge remote-tracking branch 'upstream/master' into windows

b0374e8

Remove unneeded comments, only refresh ADB devices if needed.

d809041

Update detox unit tests to pass on windows.

80d20cb

Use path module in tests to normalize paths. Use process.platform to test with actual absolute paths on windows.

simonbuchan added 2 commits April 24, 2018 21:40

Add windows support to detox-cli.

3ff87ab

Some basic documentation in roadmap.

0b6912c

Merge branch 'master' into windows

8e4a325

rotemmiz reviewed May 6, 2018

View reviewed changes

docs: 'detox' -> 'Detox'

9e9fa8a

LeoNatan suggested changes May 7, 2018

View reviewed changes

simonbuchan and others added 2 commits May 7, 2018 21:33

More docs fixes.

dcc7bc5

Merge branch 'master' into windows

86e7f7f

simonbuchan added 3 commits May 22, 2018 10:29

Oops, missed building Detox-ios-src.tbz

7f03f0b

Merge remote-tracking branch 'upstream/master' into windows

610c947

# Conflicts: # detox/.eslintrc

Lint complained about hashbangs in detox scripts.

140ad0e

rotemmiz merged commit cde5fd3 into wix:master May 25, 2018

simonbuchan deleted the windows branch May 25, 2018 09:50

wix locked and limited conversation to collaborators Jul 23, 2018

		@@ -0,0 +1,8 @@
		#!/usr/bin/env node

		// Javascript for windows support.

Running on windows support #628

Running on windows support #628

Conversation

simonbuchan commented Mar 19, 2018 • edited Loading

simonbuchan commented Mar 19, 2018

DanielMSchmidt commented Mar 19, 2018

simonbuchan commented Mar 19, 2018

simonbuchan commented Mar 19, 2018

simonbuchan commented Mar 20, 2018

rotemmiz commented Mar 20, 2018

simonbuchan commented Mar 20, 2018

rotemmiz commented Mar 21, 2018

tachtevrenidis commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rotemmiz commented Apr 17, 2018

simonbuchan commented Apr 18, 2018

simonbuchan commented Apr 18, 2018

simonbuchan commented Apr 18, 2018

CI failure

CI

Tests

rotemmiz commented Apr 19, 2018

simonbuchan commented Apr 23, 2018

simonbuchan commented Apr 24, 2018

simonbuchan commented Apr 24, 2018

simonbuchan commented Apr 24, 2018

rotemmiz commented Apr 24, 2018

simonbuchan commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbuchan commented May 7, 2018

simonbuchan commented May 7, 2018

simonbuchan commented May 25, 2018

LeoNatan commented May 25, 2018

rotemmiz commented May 25, 2018

simonbuchan commented Mar 19, 2018 •

edited

Loading