[ftr] take screenshots on failure #11709
Conversation
|
Whoa you weren't kidding that was non-trivial! I skimmed the code, but most of it was over my head. :) I did sync locally and it ran and stored a screenshot, so LGTM! 🎉 |
|
Haha :) yeah, https://github.com/elastic/kibana/blob/5.3/test/support/utils/bdd_wrapper.js was a lot harder to implement for mocha |
🖌| @@ -11,7 +11,7 @@ import { | |||
| VisualizePageProvider, | |||
| SettingsPageProvider, | |||
| MonitoringPageProvider, | |||
| PointSeriesPageProvider | |||
| PointSeriesPageProvider, | |||
There was a problem hiding this comment.
💓 Thank you, for this, and other changes like it.
| @@ -22,7 +22,8 @@ import { | |||
| KibanaServerProvider, | |||
| EsProvider, | |||
| EsArchiverProvider, | |||
| DocTableProvider | |||
| DocTableProvider, | |||
| ScreenshotsProvider, | |||
There was a problem hiding this comment.
Going through all the files, I got tired of reading "screenshots.take". I'm wondering whether you considered calling it screenshot, singular. ScreenshotProvider, screenshot.take, getService("screenshot"). What do you think?
There was a problem hiding this comment.
Hmm.. Nevermind on this comment. I think it's more consistent to have it plural.
| await fcb(cb => mkdirp(directory, cb)); | ||
| const [screenshot] = await Promise.all([ | ||
| remote.takeScreenshot(), | ||
| fcb(cb => mkdirp(dirname(path), cb)), |
There was a problem hiding this comment.
This is okay to do in parallel, is that why we are making this change? I think it's thoughtful that you noticed this.
| } catch(e) { | ||
| done(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
also line 3, no need to change path to resolve, but thought i'd point it out.
| }) | ||
| .catch(function (error) { | ||
| expect(error.message).to.be('fail.'); | ||
| done(); |
| }) | ||
| .on('testHookFailure', async (err, test) => { | ||
| log.info('testHookFailure %s %s', err.message, test.fullTitle()); | ||
| await delay(10); |
There was a problem hiding this comment.
why the delay? mimicking a screenshot.take?
There was a problem hiding this comment.
Just verifying that it's waiting for the hook to run async.
|
|
||
| expect(tests).to.have.length(0); | ||
| }); | ||
| }); |
| } | ||
|
|
||
| return createAssignmentProxy(context, assignmentInterceptor); | ||
| } |
There was a problem hiding this comment.
Okay, so the original thing that was wrapping our methods was doing it one way for describe functions and another way for all non-describe functions. The new implementation cares about more types of functions because we want to enforce single top level suite and no functions outside of describes. Nice work
* [tests/functional] move screenshots to their own service * [ftr] add testFailure and testHookFailure lifecycle hooks * [tests/functional/screenshots] cleanup old screenshots at startup * [test/functional/screenshots] take screenshots when tests fail * [cli_plugin/install] fix test * [ui/scanner] fix test (cherry picked from commit 2e7fed8)
* [tests/functional] move screenshots to their own service * [ftr] add testFailure and testHookFailure lifecycle hooks * [tests/functional/screenshots] cleanup old screenshots at startup * [test/functional/screenshots] take screenshots when tests fail * [cli_plugin/install] fix test * [ui/scanner] fix test (cherry picked from commit 2e7fed8)
* [tests/functional] move screenshots to their own service * [ftr] add testFailure and testHookFailure lifecycle hooks * [tests/functional/screenshots] cleanup old screenshots at startup * [test/functional/screenshots] take screenshots when tests fail * [cli_plugin/install] fix test * [ui/scanner] fix test
Just like the functional tests used to, these changes update them to take screenshots automatically on any test failure. To accomplish this the lifecycle service now has
testFailureandtestHookFailureevents that can asynchronously do some action at those points. The screenshot service was then updated to use those lifecycle events to take the screenshots.