Internal changes to artifacts subsystem in Detox and bugfixes

[noomorph]

Resolves #841, #856.

1. Removed accidentally exposed device.on and device.off methods.
2. Removed events emitting from Device class.
3. Moved events emitting into *DeviceDriver classes. Emitting is made via a separated and tested AsyncEmitter class.
4. Renamed launch() to launchApp in *DeviceDriver classes for more consistency.
5. Removed events beforeResetContentAndSettings and resetContentAndSettings in favor of shutdownDevice and bootDevice, because that are the actual events that we are interested in.
6. On a low level, device boot functions return true/false (true - we had a cold boot, false - we did not have to boot, rather reuse a ready-to-go device). This is used as coldBoot flag in event objects for bootDevice event.
7. Removed methods from artifacts manager plugin api: getDeviceId, getBundleId, getPid. Most of them are not used, and anyways plugin can remember the last deviceId inside itself, so it is fine.
8. Added silent: boolean flag to execWithRetriesAndLogs method, so that if we bump into an error - we can output the error to the log.debug instead of log.error.
9. When taking iOS screenshot into /dev/null, the silent flag is true (sort of convention). Also, we print a notice about the fact it is an expected error to log.debug.
10. Refactored ADB.acquireFreeDevice into two smaller functions.
11. Stability improvements to artifacts/log/ios/SimulatorLogPlugin.js.
12. The simulator log plugin has got a first integration test that verifies the consistency of produced artifacts.

[rotemmiz]

Let's consider moving all events to DeviceDriverBase, instead of spreading them in Device and multiple deviceDrivers

[noomorph]

@rotemmiz , it is not really easy to move things inside DeviceDriver itself. I think it needs a bigger refactor together with a Device class. But, at least I managed to do emitting only on drivers level - base class creates and possess a protected emitter instance which derived classes can use. It is a compromise, but I don't think we can do better without a heavier rewriting right now.

Currently, some unit tests are not passing, but the code is almost ready to the merge from my point of view, so I'd ask you to take a look again. I updated the description of the PR, btw.

[noomorph]

Tests are fixed, btw.

[noomorph]

Also resolves: #856 .

[rotemmiz]

Let's try to make the note less personal, and more like documentation, something like:

NOTE: For an unknown reason, when booting an iOS Simulator in a hidden window mode (or on CI), the first screenshot always fails. Detox tries to overcome this by taking a dummy screenshot to ensure that future ones are going to work fine. This dummy screenshot is not being saved to filesystem and the error is suppressed (except for debugging log levels).

[rotemmiz]

isn't const coldBoot = !isAlreadyRunning; enough ?

[rotemmiz]

or even better, return true only if the device was booted (aka, cold boot), and return false if the device is already booted

[rotemmiz]

if (childProcessOutput.includes(`There's another emulator instance running with the current AVD`)) {
  return false;
}

[rotemmiz]

shouldn't await super.cleanup(deviceId, bundleId); be the last line in this function?

[noomorph]

Right.

[rotemmiz]

I thought we agreed on creating base wrappers in DeviceDriverBase that will emit all events, something like:

DeviceDriverBase.js
async install(deviceId, bundleId) {
  await this.emitter.emit('beforeInstall', { anObject });
  await this.onInstall();
  await this.emitter.emit('afterInstall', { anObject });
}

*Driver.js
async onInstall() {
  //actual code
}

[noomorph]

Yes. I checked, but I can't implement it so easily because of the coldBoot complication.

I clearly see across the project that .boot() methods in --DeviceDriver classes are obligated to return string which semantically means a real device id (a concept which is contrary to device name, e.g.: Nexus_5X_API_26 -> emulator-5556 or iPhone 6s -> 7483CE6E-01CF-4B95-82DA-11E22AA94729.

That would mean that if I start returning a complex object {deviceId, coldBoot} from a child driver, then signatures between DriverBase and ChildDriver methods will start to diverge, and I don't like this idea very much.

Also, we've got around 10-14 methods like that on DeviceDriverBase.js. It is going to pollute the class with x2 signatures count. Also, if you check the code of Device and DeviceDriverBase, you will see that the good half of methods are very dumb and repetitive there:

async sendToHome() {
    await this.deviceDriver.sendToHome(this._deviceId);
  async shake() {
    await this.deviceDriver.shake(this._deviceId);
  async terminateApp(bundleId) {
    const _bundleId = bundleId || this._bundleId;
    await this.deviceDriver.terminate(this._deviceId, _bundleId);
  async installApp(binaryPath) {
    const _binaryPath = binaryPath || this._binaryPath;
    await this.deviceDriver.installApp(this._deviceId, _binaryPath);
  async uninstallApp(bundleId) {
    const _bundleId = bundleId || this._bundleId;
    await this.deviceDriver.uninstallApp(this._deviceId, _bundleId);
  async reloadReactNative() { await this.deviceDriver.reloadReactNative();

I think that architecture in all these classes is quite ad-hocish. and can (and is worth to) be rethought and improved, but since the emitter physically lives inside DeviceDriverBase class right, I think we can postpone a heavier rewrite. I'm afraid if I really start touch it, I won't be lucky to finish it till the next week.

[rotemmiz]

Where do we need to control process.env.HOME ?

[noomorph]

I'm returning it back, normally 😄
And that's to simplify tests below. I do them with snapshots. There's too much effort with manual editing the tests code after generic changes to exec calls.

[noomorph]

If I don't "freeze" somehow $HOME between CI and my computer, snapshots will break.

[noomorph]

Probably, we could do that via environment module (get $HOME variable). Shall I rewrite this way?

[noomorph]

Marked as WIP, since there is an issue with the suggested SimulatorLogPlugin implementation on high loads.

[noomorph]

Removed [WIP] label since the iOS simulator log plugin was stabilized.