-
Notifications
You must be signed in to change notification settings - Fork 20
Conversation
…sts into try/video-recording # Conflicts: # lib/after.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, Brent!
I left a few minor comments, but they are more like personal preference.
One thing that bothering me - is the execution time. Looks like it is increased for ~2 min: 11:01 vs 8:59
looks like run-wrapper.sh
used only in this repo, but I think should be careful not to affect canaries and/or e2e-tests-for-branches without need.
lib/video-recorder.js
Outdated
} | ||
|
||
export function startDisplay() { | ||
if ( process.env.TEST_VIDEO === 'true' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using guard clause, in this case, will remove not needed nesting which IMO more readable
lib/video-recorder.js
Outdated
} | ||
|
||
export function startVideo() { | ||
if ( process.env.TEST_VIDEO === 'true' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
lib/video-recorder.js
Outdated
} | ||
|
||
export function stopVideo( currentTest = null ) { | ||
if ( process.env.TEST_VIDEO === 'true' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
@brbrr I think the delay is more an issue with tests that are re-tried. Of course I can't get a smooth run with no retries when I want to, but when desktop tests ran without issues, that finished in 9:22, which is right where I would expect it with 15-20 seconds of overhead for installing ffmpeg. |
Updated to use guard clauses as suggested by @brbrr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really nice, it will be much helpful. Great work! 👍
This PR adds recording of test runs when run in CircleCI. All runs are recorded, but passing runs are deleted so that there aren't a bunch of useless videos. The videos are named after the step in which the test failed, similar to screenshots.
Here is a build where I added some intentional test failures- https://circleci.com/gh/Automattic/wp-e2e-tests/21836
Here is an example video - https://21836-57936731-gh.circle-artifacts.com/1/home/circleci/wp-e2e-tests/screenshots/videos/can-see-the-correct-blog-url-displayed-2018-08-03T18-54-20.mpg
Testing this one requires getting tests to fail in CircleCI. I have done that in the links above, but it would also be good to check that tests pass and fail as expected when run locally, even though videos will not be created.
Please let me know if you see any room for improvements or have any questions!