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.

Sign up for GitHub

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

Jump to bottom

Safe boot emu's #2011

Closed
wants to merge 1 commit into from
Closed

Safe boot emu's #2011

wants to merge 1 commit into from

Conversation

d4vidi
Copy link
Collaborator

@d4vidi d4vidi commented Apr 19, 2020 •
edited

  • This is a small change
  • This change has been discussed in issue #<?> and the solution has been agreed upon with maintainers.

Description:
Add sleep()s in between Android emulator launches so as to heuristically prevent absolute simultaneous launches of emulators of the same AVD. This aims at fixing #2106.

@d4vidi d4vidi requested a review from rotemmiz April 19, 2020 15:11
@d4vidi d4vidi self-assigned this Apr 19, 2020
@noomorph
Copy link
Collaborator

@d4vidi, great idea! I've been hit by this issue more than a few times, btw.

noomorph
noomorph reviewed Apr 21, 2020
View reviewed changes
detox/src/devices/DeviceRegistry.js
this._toggleDeviceStatus(deviceId, true);
return deviceId;
const newState = this._toggleDeviceStatus(deviceId, true);
return {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@d4vidi, please fix JSDoc @returns

Copy link

Choose a reason for hiding this comment

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

@d4vidi could you add samples for two devices connection, please...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Promise<Object>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@admizh wdym by that?

@d4vidi d4vidi marked this pull request as draft April 21, 2020 08:26
@d4vidi d4vidi force-pushed the safe-boot-emus branch 2 times, most recently from 6acf799 to bde7287 Compare April 30, 2020 11:14
@d4vidi d4vidi closed this Apr 30, 2020
@d4vidi d4vidi reopened this Apr 30, 2020
@d4vidi
Copy link
Collaborator Author

d4vidi commented Apr 30, 2020 •
edited

Seems that adb commands other than adb install get jammed when this is applied (though not always):

❌ https://jenkins-oss.wixpress.com/job/multi-detox-pr/2096/
✅ https://jenkins-oss.wixpress.com/job/multi-detox-pr/2101/
❌ https://jenkins-oss.wixpress.com/job/multi-detox-pr/2102/

adb shell getprop command, in particular, but it's probably the timing that matter here, not the command itself

I think we should upgrade our agents' Android SDK component and try again (as explained by the google team, some related scenarios were addressed):

Available Updates:
  ID             | Installed    | Available
  -------        | -------      | -------
  emulator       | 29.3.0       | 30.0.5
  ndk-bundle     | 20.1.5948944 | 21.1.6352462
  platform-tools | 29.0.5       | 30.0.0

or better yet - alpha channel (gives emulator version > 30.0.9):

Available Updates:
  ID             | Installed    | Available
  -------        | -------      | -------
  emulator       | 29.3.0       | 30.0.10
  ndk-bundle     | 20.1.5948944 | 21.1.6363665 rc3
  platform-tools | 29.0.5       | 30.0.0

Need to see however how RNN builds react to this. @guyca
Also, even if everything goes clear, we need to consider limiting Detox' parallel run on Android to emulators version > 30.0.x (x = TBD)

@d4vidi
Copy link
Collaborator Author

d4vidi commented May 4, 2020

Completed an upgrade on CI:

Available Updates:
  ID             | Installed    | Available
  -------        | -------      | -------
  emulator       | 29.3.0       | 30.0.11
  platform-tools | 29.0.5       | 30.0.0

Now retrying to see whether these newer versions happen to solve this.

@d4vidi d4vidi closed this May 4, 2020
@d4vidi d4vidi reopened this May 4, 2020
@d4vidi d4vidi closed this May 4, 2020
@d4vidi d4vidi reopened this May 4, 2020
@d4vidi d4vidi force-pushed the safe-boot-emus branch 2 times, most recently from a4966fb to b0531cc Compare May 5, 2020 07:33
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2110

  • 25 of 25 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 76.975%
Totals Coverage Status
Change from base Build 1171: 0.06%
Covered Lines: 3206
Relevant Lines: 3876
💛 - Coveralls

@d4vidi d4vidi closed this May 5, 2020
@d4vidi d4vidi mentioned this pull request May 5, 2020
Closed
2 tasks
@d4vidi d4vidi mentioned this pull request May 13, 2020
Closed
2 tasks
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
@d4vidi d4vidi changed the title Introduce safety time-margins between launches of multiple android emu's [WIP] Introduce safety time-margins between launches of multiple android emu's Jun 7, 2020
@d4vidi d4vidi changed the title [WIP] Introduce safety time-margins between launches of multiple android emu's Introduce safety time-margins between launches of multiple android emu's Jun 7, 2020
@d4vidi d4vidi changed the title Introduce safety time-margins between launches of multiple android emu's Safe boot emu's Jun 7, 2020
@d4vidi
Copy link
Collaborator Author

d4vidi commented Jun 7, 2020 •
edited

Was closed as this appears to actually increase the frequency of the adb-jam bug on Detox CI. 'Was quite a WTF moment.

@LeoNatan LeoNatan deleted the safe-boot-emus branch November 18, 2020 17:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@noomorph noomorph noomorph left review comments

@admizh admizh admizh left review comments

@rotemmiz rotemmiz Awaiting requested review from rotemmiz

Assignees

@d4vidi d4vidi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@d4vidi @noomorph @coveralls @admizh