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

fix(avd): consider listed emulator as running even if state !== 'device' #140

Merged
merged 3 commits into from
Feb 7, 2020

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Feb 6, 2020

JIRA: https://jira.appcelerator.org/browse/TIMOB-27750

Tested this with the same emulator that exhibited the issue before and it worked with these changes.

I also wiped the snapshot for the emulator so it had to do a full boot and that also worked.

@build
Copy link

build commented Feb 6, 2020

Warnings
⚠️

lib/adb.js#L81 - lib/adb.js line 81 – 'port' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/adb.js#L310 - lib/adb.js line 310 – Empty block statement. (no-empty)

⚠️

lib/adb.js#L316 - lib/adb.js line 316 – Missing JSDoc for parameter 'config'. (valid-jsdoc)

⚠️

lib/adb.js#L467 - lib/adb.js line 467 – Missing JSDoc for parameter 'callback'. (valid-jsdoc)

⚠️

lib/adb.js#L467 - lib/adb.js line 467 – Missing JSDoc for parameter 'config'. (valid-jsdoc)

⚠️

lib/adb.js#L510 - lib/adb.js line 510 – 'err' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/adb.js#L510 - lib/adb.js line 510 – 'out' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/adb.js#L526 - lib/adb.js line 526 – 'data' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/adb.js#L721 - lib/adb.js line 721 – 'err' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/adb.js#L721 - lib/adb.js line 721 – 'out' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/adb.js#L743 - lib/adb.js line 743 – 'err' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/adb.js#L743 - lib/adb.js line 743 – 'out' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/adb.js#L768 - lib/adb.js line 768 – 'err' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/adb.js#L768 - lib/adb.js line 768 – 'out' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/emulators/avd.js#L21 - lib/emulators/avd.js line 21 – Found require("child_process") (security/detect-child-process)

⚠️

lib/emulators/avd.js#L198 - lib/emulators/avd.js line 198 – 'err' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

Messages
📖

✅ All tests are passing
Nice one! All 26 tests are passing.

📖 🎉 - congrats on your new release

Generated by 🚫 dangerJS against 41307f1

@sgtcoolguy
Copy link
Contributor Author

The idea behind this is that an emulator isn't even listed unless it's "running" in some sort of state.
For a snapshotted android-29 emulator, I get a single adb host:track-devices event/callback with a value like emulator-5554 offline. We do not get another event. And the emulator is effectively up and "booted" instantaneously. So this changes the idea of checking if an avd emulator is running to be simply if it's listed in the device listing (and not that it's reported with a state of 'device").

We have a secondary check for testing if the boot process/animation is done that is performed after we know the emulator is "running". So if the emulator really isn't yet ready, that adb shell getprop command should fail (and assume it's not ready/booted).

@cb1kenobi
Copy link
Contributor

@sgtcoolguy I don't understand how you get only a single track device event. Regardless if I do a cold boot or warm boot from a snapshot, it always emits an "offline" event, followed by a "device" event.

I do agree that if a device is "offline", it doesn't mean the emulator is not "running". If it shows up in adb devices, it's running.

As I see it, there are 2 issues:

  1. isRunning() should be renamed to isReady() and your version should be in a function named isRunning(). When we go to launch the emulator, we should check if it's already running. When we go to install an app, we should check if it's ready (i.e. state=device).

  2. Track devices should be emitting a "device" state event once the emulator is running. When you start it, it should emit an "offline" event before the eventual "device" event. If you are only getting "offline", but not "device", it's possible there's a race condition, but not sure where/why. I need to take a closer look.

@jquick-axway
Copy link
Contributor

@sgtcoolguy, a quick alternative solution for the time being (at least for our Jenkins unit tester) would be to support the emulator command line tool's -no-snapshot argument. This will do a "cold" start and not save a snapshot upon exit.
https://developer.android.com/studio/run/emulator-commandline#common

Ideally we should solve the snapshot launch problem, but if we need a solution now, this might work for us.

@sgtcoolguy
Copy link
Contributor Author

The issue is that the Jenkins stuff doesn't have direct control over the arguments passed to the emulator. This is entirely within this library. The only "fix" fro Jenkins would be to pre-launch an emulator ourselves before starting the build and waiting (or hoping) it boots before it gets to that stage. This is the correct place for the fix, and I think this fix is fairly straight-forward.

@sgtcoolguy
Copy link
Contributor Author

@cb1kenobi The code already has an equivalent to isReady(), it's the checkedBooted() function which gets called inside EmulatorManager.protoype.start() (which is always called from the android cli hook).

The SDK code for installing an app waits until the emulator emits a ready event. The node-titanium-sdk code emits that after it emits the booted event (either right after, or after waiting for the sdcard to mount). It emits the booted event after the emulator is deemed running, and after the boot animation is done.

So the flow is:

  • SDK CLI run hook calls EmulatorManager.protoype.start()
  • That calls isRunning() to see if the emulator is already running - if not, it calls start on the avd code to start it.
  • in both cases it then calls checkedBooted()
  • that code calls adb.trackDevices to wait for listings. When one is found, it checks that it's running. If so, then it checks to see if the boot animation is done. If so, we emit booted
  • once we emit booted, we optionally wait for the sdcard to mount, then emit ready
  • the SDK CLI code gets the ready event and moves on to install the app.

@sgtcoolguy sgtcoolguy merged commit 72a612b into master Feb 7, 2020
@sgtcoolguy sgtcoolguy deleted the TIMOB-27750 branch February 7, 2020 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants