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

Emulator wrapper fixes #702

Merged
merged 3 commits into from
May 3, 2018
Merged

Emulator wrapper fixes #702

merged 3 commits into from
May 3, 2018

Conversation

noomorph
Copy link
Collaborator

@noomorph noomorph commented Apr 30, 2018

  1. fix: emulator should always get non-empty command line arguments (regression from Add no window option for running android emulator with no window #690)
  2. fix: detox fails to start when another emulator is running
  3. feat: logging emulator output on errors

Regarding (2), there was an issue (likely on Linux, but not sure if it is the reason) with a race condition.

tail.on("line", function(data) { was called too late, when the promise had already rejected.
Since the function was returning return promise; without a catch (e.g. return promise.catch(function () {), the promise._cpResolve() call was happening too late inside tail.on, when the consumer of boot logic actually had gotten into a negative conditional branch.

fix: detox fails to start when another emulator is running
@noomorph noomorph requested a review from rotemmiz April 30, 2018 13:43
const cmd = _.compact([
'-verbose',
'-gpu',
'host',
Copy link
Member

Choose a reason for hiding this comment

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

Change to gpu auto for better support. Current command breaks on some machines

@noomorph
Copy link
Collaborator Author

@rotemmiz , done.

@noomorph noomorph force-pushed the fix/emulator-stdout-logging branch from 7422075 to 8855370 Compare April 30, 2018 14:36
@noomorph
Copy link
Collaborator Author

@rotemmiz , the build is green.

@noomorph noomorph force-pushed the fix/emulator-stdout-logging branch from 8f93eea to 444477f Compare May 2, 2018 10:06
@noomorph
Copy link
Collaborator Author

noomorph commented May 2, 2018

The behavior after this pull request is going to be the following:

  1. non-verbose mode

    1. when emulator is running
      screenshot from 2018-05-02 13-01-21
    2. on error (had to artificially emulate this error by commenting out if statement in my code)
      screenshot from 2018-05-02 13-01-39
    3. when no emulator was running
      screenshot from 2018-05-02 13-12-05
  2. verbose mode

    1. when emulator is running:
      screenshot from 2018-05-02 12-55-42
    2. on error (had to artificially emulate this error by commenting out if statement in my code)
      screenshot from 2018-05-02 13-02-00
    3. when no emulator was running:
      screenshot from 2018-05-02 12-58-56

const childProcessPromise = spawn(this.emulatorBin, emulatorArgs, { detached: true, stdio: ['ignore', stdout, stderr] });
childProcessPromise.childProcess.unref();

return childProcessPromise.catch((err) => {
Copy link
Member

Choose a reason for hiding this comment

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

can't this be separated into two declarations ?

childProcessPromise.catch((err) => {
}
....
return childProcessPromise;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But this is completely different logic then. What do you mean?
screenshot from 2018-05-02 15-14-50

@rotemmiz rotemmiz merged commit 25e59e6 into master May 3, 2018
@noomorph noomorph deleted the fix/emulator-stdout-logging branch May 4, 2018 04:58
@wix wix locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants