Screenshots, logs and video recordings of tests #734

noomorph · 2018-05-15T17:34:41Z

The pull request aims to add the following features:

Automatic screen recording on iOS/Android with two configurable options: --artifacts-location [rootDir] (where videos will be saved) and --record-videos [all|failing|none] behavior (none is default and means that recording is disabled).
Automatic screenshots (beforeEach and afterEach) on iOS/Android with two configurable options: --artifacts-location [rootDir] (where screenshots will be saved) and --take-screenshots [all|failing|none] behavior (none is default and means that screenshotting is disabled).
Automatic log recording during test on iOS/Android with two configurable options: --artifacts-location [rootDir] (where logs will be saved) and --record-logs [all|failing|none] behavior (none is default and means that recording is disabled).
Out-of-box support for [mocha] and [jest] test runners:

detox init -r mocha and detox init -r jest should scaffold configuration files which are sufficient for the new artifact recording features and (hopefully) flexible enough not to require manual updates on the user side.
the latter is being solved by introducing detox<->mocha and detox<->jest adapters which encapsulate the tricky parts to enable executing long-running hooks (e.g. before (beforeAll), beforeEach, afterEach, after (afterAll) in mocha and jest) together with ability to receive current running test status and name (not trivial thing in jest). That's a thing strongly required by the artifacts feature.

Artifacts feature should be fairly decoupled from detox internals - decoupled enough to grant a possibility to extract it to a separate plugin with ease in more distant future (if we'll decide we need it).

Current status

The PR’s author (@noomorph) has returned from the vacation. He is working on documentation this week, stabilizing code, improving error messages and unit tests also.

TODO list

Documentation:

update Getting Started.
update Migration Guide.
update test lifecycle about beforeEach and afterEach
update detox object about detox.beforeEach(testSummary) and detox.afterEach(testSummary).
update Jest Guide about detox init -r jest.
update detox-cli with new artifact flags and breaking change that --record-logs all should be specified explicitly in order to retain the previous behavior.
update Enabling artifacts section with first steps and troubleshooting
add a pitfalls subsection: e.g. Ctrl+C does not work correctly with Jest, Metal is not supported on VM, or maybe Increased flakiness with artifacts recording, and so on...

Nice to haves:

extend configuration for screenshot plugin - add an ability to specify at which phase you want screenshots to be taken: beforeEach, afterEach or both. E.g., all:beforeEach,afterEach means that screenshots will be taken in every test two times: in a global beforeEach and in a global afterEach. One more example, failing:afterEach will mean that screenshots will be taken only on failing tests in a global afterEach. In future we might add beforeTest and afterTest phases which will surround test() or it() statement.
integration tests for artifact plugins
merge from branch e2e suite fordetox/src/runners/jest/DetoxJestAdapter.js
merge from branch e2e suite for detox/src/runners/mocha/DetoxMochaAdapter.js
improve logging in verbose mode (e.g. add prefix by process name and pid to exec logs)
add extensive logging for artifacts plugins
specific exception handling (e.g. Metal is not supported on VM)

Links:

the former pull request: [WIP] Screenshots and screen recordings of tests #541 .
the parent issue: Add support for test artifacts (videos and screenshots) #171

Hopefully, I'll be moving quite-quite fast.
Stay tuned.

noomorph · 2018-05-15T18:39:54Z

I thought it might be curious to share a video of detox/test project run (from yesterday) with android video recording turned on.

https://youtu.be/mLVw3jowr4k

A nice improvement (since the former PR) is that Detox copies recorded videos in parallel. Namely, we don't wait for all tests to complete before we start pulling videos from the device.

P. S. On the other hand, I have a silly concern whether framerates are apt to drop (...or maybe not?) when adb screenrecord and adb pull run concurrently. If this proves to be an issue, I might revert the behavior to the former one or alternatively, add an extra flag, but, in any case, this "gut feeling" needs to be either proved or disproved.

rotemmiz

Nice work, and thanks for the new and improved candy, adapters, detox-init, custom errors and prints.

I do have some questions regarding implementation of the artifact manager and peripherals (see notes).

rotemmiz · 2018-05-16T21:17:18Z

detox/src/artifacts/v2/ArtifactsManager.js

@@ -0,0 +1,44 @@
+const _ = require('lodash');


This is the thing, not v1 vs v2, its a replacement, no going back. just call the kid in its name (and path): detox/src/artifacts/ArtifactsManager.js

rotemmiz · 2018-05-16T21:19:07Z

detox/src/artifacts/v2/ArtifactsManager.js

+    this.hooks = [];
+ }
+
+  registerHooks(hooks) {


interesting handling of malformed hooks

rotemmiz · 2018-05-16T21:19:59Z

detox/src/artifacts/v2/ArtifactsManager.js

+  }
+}
+
+module.exports = ArtifactsManager;


Nice and clean, I like it <3

rotemmiz · 2018-05-16T21:28:24Z

detox/src/artifacts/v2/index.js

+        .registerHooks(resolve.artifacts.hooks.video(detoxApi));
+    }),
+  },
+};


I'm a bit confused with index.js

Why did you choose resolveing once (using lodash)? can't this be a simple object that will be instantiated once somewhere in the init process?

Android and iOS logic are mixed in this class, why is that ?

There's a factory (actual). What is the reason to go over the selection phase again if DeviceDriver is already initiated ?

It also looks like VideoRecorderHooks.js and ScreenshotterHooks.js share some logic, but mostly, if we want to add a new artifact (let's say performance counters from Detox Instruments), it seems as though we would need to clone one of those classes implementation. right ?

Wouldn't it make sense to put most of this hooks logic in ArtifactsManager, and only implement start(), stop(), copy(), discard() methods (did I forget anything) for each artifact ?

rotemmiz · 2018-05-16T21:38:36Z

detox/src/artifacts/v2/loggers/ADBLogcatLogger.js

@@ -0,0 +1,15 @@
+const ADBLogcatTailRecording = require('./ADBLogcatTailRecording');


What is the use of ADBLogcatLogger ? What's the benefits of using it on just using ADBLogcatTailRecording.js instance controlled by ArtifactManager ?

rotemmiz · 2018-05-16T21:52:24Z

detox/src/artifacts/v2/videoRecorders/NoopVideoRecorder.js

@@ -0,0 +1,14 @@
+class NoopVideoRecording {


Why do we need those empty noop classes ? can we just not instantiate the real classes when users ask not to do generate artifacts ?

rotemmiz · 2018-05-16T21:59:28Z

detox/src/devices/android/AndroidToolsLocator.js

+const _ = require('lodash');
+const fsext = require('fs-extra');
+
+class AndroidToolsLocator {


I see you don't use this class, what is the thinking behind it? what makes it better than the current implementation where every command line wrapper handles finding the bin path by itself ?

detox/src/utils/constructSafeFilename.js

@@ -0,0 +1,30 @@
+const DetoxRuntimeError = require('../errors/DetoxRuntimeError');


rotemmiz · 2018-05-16T22:13:52Z

detox/src/utils/constructSafeFilename.js

+/*
+  Trim filename to match filesystem limits (usually, not longer than 255 chars)
+ */
+function constructSafeFilename(prefix = '', trimmable = '', suffix = '') {


Usage is a bit convoluted, it took me too much time to understand how this works.
Where does the suffix comes from? It's hard for me to follow.

const testArtifactsDirname = constructSafeFilename(testIndexPrefix, testSummary.fullName); const artifactFilename = constructSafeFilename(artifactName);

There are no use cases with suffixes at the moment, to be honest. I just thought it would make more sense to write a signature like (prefix, trimmable, suffix) so it tells about the intent of function which is basically return prefix + trimmable + suffix or in a worse case return prefix + trimmable.slice(-maxLen) + suffix.

Maybe the source of your convolution is a multimethod implementation which treats a single parameter as a second parameter? I have no objections to simplify it, so it will look like:

const testArtifactsDirname = constructSafeFilename(testIndexPrefix, testSummary.fullName); const artifactFilename = constructSafeFilename('', artifactName);

Removed multimethod like I said before. Now signature should look less sophisticated, hopefully.

rotemmiz · 2018-05-16T22:15:08Z

detox/src/utils/sleep.test.js

@@ -0,0 +1,10 @@
+const sleep = require('./sleep');


no need to test this. you can put it in ignore list

noomorph · 2018-05-18T16:51:23Z

@rotemmiz , I made a (relatively) minor change to artifacts API.

record('artifact path').start().stop().save() became record().start().stop().save('artifact path').

It gives us a superpower to give different names to artifacts depending on test status (passed|failed). Hope you'll like it: ✔ 1. Passing test and ✗ 2. Failing test. 😛

This becomes very handy with option --record-whatever all.

noomorph · 2018-05-18T18:02:20Z

🎉 I've got iOS simulator logging working, hooray!

…upled way

… from .record(path).stop().save() to .record().stop().save(path)

…on the rest of the project

…unload device

noomorph · 2018-05-21T15:45:05Z

@rotemmiz, check out the verbosity level I did for Android logs:

test.log

noomorph · 2018-05-21T16:03:33Z

Quick update. I've removed artifacts folders' numbering to avoid potential issues with parallel e2e test execution on multiple devices.

Now the folders look this way:

rotemmiz · 2018-05-21T14:56:34Z

detox/src/artifacts/ArtifactsManager.js

+    ]);
+  }
+
+  _createLogRecorderLifecycle(logRecorder) {


The three functions look so similar, can't they be merged into one function ?

It’s not that easy. 2 are using RecorderLifecycle (different params: startup video is not recorded, opposed to the startup log) and 1 is using SnapshotterLifecycle. Merging all lifecycle into one heavy and parameter-rich is a doubtful endeavor, to my mind. Might complicate understanding of the implementation.

rotemmiz · 2018-05-21T14:57:58Z

detox/src/artifacts/ArtifactsManager.js

@@ -0,0 +1,137 @@
+const _ = require('lodash');
+const npmlog = require('npmlog');


we usually use const log = require('npmlog');

rotemmiz · 2018-05-21T15:04:52Z

detox/src/artifacts/core/artifact/RecordingArtifact.js

+
+  async doDiscard() {}
+
+  _assertRecordingIsNotBeingResumed() {


this is not a user facing API, it's a one time implementation, do we really need all those assertions ?

You’re right. Now it is not user-facing, but honestly I was thinking that we wanted to make it available in next versions.

I think we indeed can remove most of the checks, but on other hand they served me a great debugging value whilst writing lifecycles. If anybody goes to change something there or adds a new lifecycle, it can help while testing.

Secondly, sometimes we might need a smarter logic inside. E.g., if on SIGINT we have to clean up allocated temporary artifact files, it is easier to call .discard() from ArtifactsManager for each artifact rather than implement status checks to see if we need to .stop() it before .discard()-ing it. Smarter implementation of .discard() gives less headache when you just need to .discard() something.

I though that lifecycle is controlled by ArtifactManager, its a one time implementation, am I wrong ?

It is, but indirectly.
I prefer to keep lifecycles of a SnapshotArtifact and a RecordingArtifact as separate entities.
They are different enough in my opinion to not try to stitch them together.
In any case, ArtifactsManager is responsible for overall hooks (onStart, onExit, ...) execution and it has a couple of global (cross-lifecycle) concerns:

It keeps registering artifacts, so it can discard them in an emergency.

It provides enqueueFinalizationTask method to lifecycles - to coordinate finalization, e.g. to make it less CPU-intensive.

In other words, ArtifactsManager alone has enough of a headache to handle, so it delegates minutiae to "smaller" and more specific lifecycles per artifact type.

rotemmiz · 2018-05-21T15:06:35Z

detox/src/artifacts/core/artifact/RecordingArtifact.test.js

@@ -0,0 +1,7 @@
+const RecordingArtifact = require('./RecordingArtifact');


If this is the test, we may just as well ignore it.

and add to ignored files in package.json

added a unit test.

rotemmiz · 2018-05-21T15:11:48Z

detox/src/artifacts/core/artifact/SnapshotArtifact.test.js

@@ -0,0 +1,7 @@
+const SnapshotArtifact = require('./SnapshotArtifact');


same goes here, we can ignore it

added a unit test.

rotemmiz · 2018-05-21T16:02:06Z

detox/src/runners/jest/DetoxJestAdapter.js

+      return;
+    }
+
+    const spec = Object.freeze({


This object is going inside Detox implementation, which just reads from it (correct me if I'm wrong please)
Why do we need to freeze it ?

I suggest we get rid of all Object.freeze, especially if the consumers are us.

Valid point.

rotemmiz · 2018-05-21T16:05:58Z

detox/src/devices/android/ADB.js

+    return parseInt(stdout, 10);
+  }
+
+  screencap(deviceId, path) {


is this supposed to be an async function ?

Yep. Missed. Thanks.

rotemmiz · 2018-05-21T16:06:21Z

detox/src/devices/android/ADB.js

+    return this.spawn(deviceId, ['logcat', ...logcatArgs]);
+  }
+
+  pull(deviceId, src, dst = '') {


rotemmiz · 2018-05-21T16:06:26Z

detox/src/devices/android/ADB.js

+    return this.adbCmd(deviceId, `pull "${src}" "${dst}"`);
+  }
+
+  rm(deviceId, path, force = false) {


rotemmiz · 2018-05-21T16:11:23Z

detox/src/artifacts/log/ios/SimulatorLogRecording.js

+  }
+
+  _createTail(file, prefix) {
+    const tail = new Tail(file, {


I'm afraid tailing those files might be CPU intensive, let's think together if there's something out of process that we can do to avoid reading line by line.
Previously we have redirected std to files, and collected the files after each test. It's ugly, I agree, but much more efficient.

We can delegate it to a real *nix tail > test.log, since it should be more lightweight than Node.js.

I'm not entirely sure if this is too intensive. Need to measure. And I'll check it on underpowered macbook 12 for issues.

rotemmiz · 2018-06-20T07:39:54Z

🎉

luisnaranjo733 · 2018-06-23T20:38:48Z

This is awesome. I think this will bring a ton of value to detox and enable QA scenarios that were previously not possible.

I'm very interested in using detox specifically for this functionality.
Judging by when this was merged and what I'm seeing in npmjs.com, I assume this will be available for the first time in 8.0.0-alpha.2.

Is there an ETA on that? Also, do you know when you are expecting to ship a stable 8.0.0 release?

juddey · 2018-06-27T23:04:23Z

This is fantastic. Thank you. ❤️

mCodex · 2018-07-19T14:00:09Z

Awesome work, thank you!

noomorph requested a review from rotemmiz as a code owner May 15, 2018 17:34

rotemmiz reviewed May 16, 2018

View reviewed changes

noomorph force-pushed the feature/test-artifacts branch from 0cac087 to 20e419d Compare May 18, 2018 07:48

fpersico mentioned this pull request May 18, 2018

Experimental detox based tests pagopa/io-app#145

Merged

noomorph added 15 commits May 20, 2018 18:44

wip: logs, screenshots, videos recording on iOS and Android in a deco…

5a5c157

…upled way

fix: unit tests fix

dcdc93d

fix: android video recording

51df0eb

fix: ensuring defaults in detoxApi.getConfig()

9f76d04

feat: completed screenshot taking on android

3aefbc8

code: moved artifacts out of v2 folder

bbc6dce

fix: enables taking screenshots on iOS

d5b87eb

fix: enables recording videos on iOS

77299da

refactor: after review with the stakeholder

422a2fa

code: disabled coverage on utils/sleep.js

9f9b713

refactor: new base classes Recorder and Snapshotter; signature change…

260b0ca

… from .record(path).stop().save() to .record().stop().save(path)

test: temporarily disabled coverage for artifacts/ and restored 100% …

ae203cb

…on the rest of the project

fix: iOS simulator issues with video recorder and screenshotter

f4869ae

feat: iOS brand new simulator logging

cc8e91a

fix: bugfix for log beginning copied to every test log

a7cf806

noomorph force-pushed the feature/test-artifacts branch from 7c6ecd5 to a7cf806 Compare May 20, 2018 15:45

feat: Android logcat recording and centralized finalization queue to …

e3edd5e

…unload device

rotemmiz mentioned this pull request May 21, 2018

[WIP] Screenshots and screen recordings of tests #541

Closed

feat: adb logcat is using --pid argument by default

8eaae6b

feat: removed numbering in artifacts paths

e769085

rotemmiz reviewed May 21, 2018

View reviewed changes

noomorph added 5 commits June 14, 2018 16:52

fix: snapshot calculation in artifacts verification aftertest

62472f7

fix: trying to fix first screenshot error thing

5964098

ci: trigger commit

e5523cd

revert: screenshot fix attempt (commit 5964098)

4bb6978

fix: first screenshot error

38f03d7

noomorph force-pushed the feature/test-artifacts branch from 8b201c6 to 38f03d7 Compare June 16, 2018 16:09

yershalom and others added 16 commits June 17, 2018 16:34

Add Jenkins constrains

b2f89c9

Update APIRef.Artifacts.md

d468028

Update APIRef.Artifacts.md

0e25350

Update APIRef.Artifacts.md

c93d37c

Merge branch 'master' into feature/test-artifacts

a409ac6

docs: update Guide.Migration.md

0a14ff1

Update Guide.Migration.md

e230190

Update Guide.Migration.md

6e557f1

Update Guide.Migration.md

a33fb92

Update APIRef.DetoxObjectAPI.md

fef8298

Update Guide.Migration.md

dcd5c25

Update Guide.Migration.md

5aad839

Update Guide.Migration.md

1fe4f36

Update Guide.Migration.md

f422bff

Update Guide.Migration.md

0c0c175

Update Guide.Migration.md

93e3975

rotemmiz merged commit e0f4614 into master Jun 20, 2018

rotemmiz deleted the feature/test-artifacts branch June 20, 2018 07:39

rotemmiz changed the title ~~[WIP] [new] Screenshots and screen recordings of tests~~ Screenshots, logs and video recordings of tests Jun 27, 2018

wix locked and limited conversation to collaborators Jul 23, 2018

		@@ -0,0 +1,15 @@
		const ADBLogcatTailRecording = require('./ADBLogcatTailRecording');

		@@ -0,0 +1,30 @@
		const DetoxRuntimeError = require('../errors/DetoxRuntimeError');

		@@ -0,0 +1,137 @@
		const _ = require('lodash');
		const npmlog = require('npmlog');

		@@ -0,0 +1,7 @@
		const RecordingArtifact = require('./RecordingArtifact');

		@@ -0,0 +1,7 @@
		const SnapshotArtifact = require('./SnapshotArtifact');

Screenshots, logs and video recordings of tests #734

Screenshots, logs and video recordings of tests #734

Conversation

noomorph commented May 15, 2018 • edited Loading

Current status

TODO list

noomorph commented May 15, 2018

rotemmiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noomorph commented May 18, 2018

noomorph commented May 18, 2018

noomorph commented May 21, 2018

noomorph commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noomorph May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rotemmiz commented Jun 20, 2018

luisnaranjo733 commented Jun 23, 2018

juddey commented Jun 27, 2018 • edited Loading

mCodex commented Jul 19, 2018

noomorph commented May 15, 2018 •

edited

Loading

noomorph May 22, 2018 •

edited

Loading

juddey commented Jun 27, 2018 •

edited

Loading