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

Genycloud teardown #2471

Merged
merged 4 commits into from
Nov 22, 2020
Merged

Genycloud teardown #2471

merged 4 commits into from
Nov 22, 2020

Conversation

d4vidi
Copy link
Collaborator

@d4vidi d4vidi commented Nov 18, 2020

Description

Another (mandatory) step towards support for Genymotion's SaaS solution (see #2429): supporting tear-down.

One of the most important aspects of using cloud emulators is the proper management of their deletion (in particular, because pricing is usage-time based). This change introduces instances deletion that in a Jest-based environment, is based on the global-teardown hook, with the API invocation looking pretty much like this:

// global-teardown.js
async function globalTeardown() {
  const detox = require('detox');
  await detox.globalCleanup();
}
module.exports = globalTeardown;

Namely, such that the internal details of the shutdown are obscure from the Detox user's perspective, and are available through an API that feels natural.

In Mocha - or any other test runner that does not support parallel-execution, this can be integrated by dispatching detox.globalCleanup() immediately after detox.cleanup(). In due time, we will add this to our Documentation and scaffolding scripts.

Underneath, what happens in a nutshell is this:

  1. A new, custom device-registry is initialized (on top the existing ones), which never clears its content throughout the execution.
  2. Each driver spinning up a new cloud instance in it's init, adds the associated uuid to the device-registry.
  3. Eventually, through global-teardown (at global context), the Genymotion driver statically reads the registry, issues and executes deletion operations in a Promise.all() fashion.
  4. The end.

Important: This technique is quite robust and may seem elegant, but it's important to keep in mind that it is nevertheless not ideal. Being charged by the minute, ideally, we would like to delete each instance once its worker finishes the last bits of its work. I've experimented ways of implementing this by registering a process.on('beforeExit', () => {...}). While it simplified the implementation a great deal, it didn't work, for two reasons:

  1. Jest seems to hold workers up and running right until the very bitter end (global teardown), so the callback isn't called earlier than with the current approach.
  2. Jest doesn't approve of asynchronous work inside each worker as it waits for it to be killed: it warns the user with the famous open handles log, and sometimes force-kills before the beforeExit callback completes.
    As a bottom line, then, in order to introduce the ideal optimization, we would have to push this into Jest itself (see here).

Last but not least, this also adds support for driver.shutdown(), such that genymotion-cloud testing would respect Detox' --cleanup argument.

@d4vidi d4vidi requested a review from noomorph November 18, 2020 14:46
@d4vidi d4vidi self-assigned this Nov 18, 2020
@d4vidi d4vidi closed this Nov 18, 2020
@d4vidi d4vidi reopened this Nov 18, 2020
} catch (error) {
log.warn({ event: 'GLOBAL_CLEANUP' }, 'An error occurred trying to shut down Genymotion-cloud emulator instances!', error);
}
}
Copy link
Collaborator Author

@d4vidi d4vidi Nov 18, 2020

Choose a reason for hiding this comment

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

@noomorph if you have any better suggestions as to how to provide this in a cleaner way (leaving the TODO note aside, however), I'd be happy to hear them.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could add a link to the instance's web GUI, so it would be easy to open and examine it.
Also, it might be a good idea to add that link when a cloud emulator spins up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I will do both!

@@ -47,7 +51,7 @@ class GenyCloudDeviceAllocator extends AndroidDeviceAllocator {
}

const options = {
backoff: 'none',
backoff: 'none', // TODO apply reverse-linear polling
Copy link
Member

@rotemmiz rotemmiz Nov 18, 2020

Choose a reason for hiding this comment

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

What is reverse-linear polling? backoff strategy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly

@d4vidi
Copy link
Collaborator Author

d4vidi commented Nov 22, 2020

@noomorph please review post-factum, along with #2466. Although I'm still not done with all of the comments from the first PR (#2446), I will get to everything, eventually.

@d4vidi d4vidi merged commit da1ce8b into master Nov 22, 2020
@noomorph noomorph deleted the genycloud-teardown branch October 7, 2022 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants