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

[WIP] Screenshots and screen recordings of tests #541

Closed
wants to merge 19 commits into from

Conversation

donataswix
Copy link
Contributor

No description provided.

--take-screenshots and --record-videos flags can be passed to `detox test' to
produce screenshots and videos of tests.

function spawnRecording(width, height) {
return adb.spawn(deviceId, [
'shell', 'screenrecord', '--size', width + 'x' + height, '--verbose', '/sdcard/recording.mp4'
Copy link
Member

Choose a reason for hiding this comment

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

from https://developer.android.com/studio/command-line/adb.html

"Sets the video size: 1280x720. The default value is the device's native display resolution (if supported), 1280x720 if not. For best results, use a size supported by your device's Advanced Video Coding (AVC) encoder."

Did you encounter any issues with default values ?
Why do you use twice the native resolution for recording ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was left over from my tests, will be removed.

@rotemmiz
Copy link
Member

This PR relates to #171

async stopVideo(deviceId) {
if (this._video) {
this._video.kill(2);
await this.adb.adbCmd(deviceId, 'shell sleep 1');
Copy link
Member

Choose a reason for hiding this comment

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

I removed this line and saw the corrupted videos you talked about.
The thing is, this slows down the tests a lot!
I am trying to think of a way to mitigate the penalty:

  1. Let's create each file on the device with a different name.
  2. when test is done, create a copy+cleanup task
  3. add this task to a queue and let this queue run independently of the test lifecycle.

WDYT ?

@Cederman
Copy link

Awesome job @donataswix! Question - I looked at the PR but I can't see the flexibility to give the screenshot (or video) a name.

As an example, If I want to document a user journey and have screenshots taken at different screens, I might want to name them "login", "profile", "settings" or something similar.

Could you add that to the PR?

@rotemmiz
Copy link
Member

All artifacts are being picked up by ArtifactsCopier, which then renames them with the same name as the test description.

@Cederman
Copy link

Cederman commented Jan 31, 2018

@rotemmiz I see several issues with that, it will conflict (overwrite) or have "test description 1|2|3.png" if you would do several screenshot calls in the same test (assuming you can call it imperatively in the test). It also dictates the name itself which I don't see the need for, defaulting to the test description could work (although it might lead to long file names).

However, providing that functionality can come in a later PR but it would be nice to add it in this one. I can see it adding value for several use cases.

@donataswix
Copy link
Contributor Author

donataswix commented Feb 1, 2018

@Cederman this should work:

it('...', () => {
  // do something
  await device._takeScreenshot('between-things');
  // do another thing
});

As you can tell by the _ prefix, I didn't think of it as public API. It won't hurt to change it, I guess. Screenshot will end up in artifacts location under particular test folder.

rotemmiz and others added 5 commits February 1, 2018 20:53
Now it's possible to record videos and screenshots when running on device.
Screenshots and videos will be saved only for failed tests if "failing" value is
passed to --take-screenshots and --record-videos respectively. This requires
that detox is given success indication in afterEach() callback inside init.js
file.
@haswalt
Copy link
Contributor

haswalt commented Feb 19, 2018

Hello, any more progress on this? We've just picked up Detox and think it's great but we need the screenshot functionality to do visual regression and user flow testing.

@rotemmiz
Copy link
Member

I hope we will merge this by the end of the week.

@haswalt
Copy link
Contributor

haswalt commented Mar 7, 2018

any chance on this getting merged soon? I know you guys are pretty busy, happy to help out fixing the conflicts if needed.

@skovhus
Copy link

skovhus commented Mar 15, 2018

Do you guys need any help with this? Would be really nice to have.

@almostintuitive
Copy link

Hi! It'd be great to get this in! We'd love to use it. We're happy to contribute, if you let us know what needs to be done in order to merge it, we'll look into it as well.

@rotemmiz
Copy link
Member

This is being actively worked on by @noomorph.
Updates can be found here

@noomorph, can you update regarding the current state ? Is there a way to delegate/split some of the work ?

@noomorph
Copy link
Collaborator

@rotemmiz , sure! see my response in #171

@rotemmiz
Copy link
Member

Closing this in favor of #734

@rotemmiz rotemmiz closed this May 21, 2018
@wix wix locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants