feat(testrunner): better matchers #1077
Conversation
This patch re-implements matching and reporting for test runner. Among other improvements: - test failures now show a short snippet from test - test failures now explicitly say what received and what was expected - `expect.toBe()` now does text diff when gets strings as input - `expect.toEqual` now does object diff
| @@ -20,6 +20,7 @@ const Diff = require('text-diff'); | |||
| const PNG = require('pngjs').PNG; | |||
| const jpeg = require('jpeg-js'); | |||
| const pixelmatch = require('pixelmatch'); | |||
| const c = require('colors/safe'); | |||
There was a problem hiding this comment.
nit: s/c/colors
You taught me that we don't abbreviate like that :)
utils/testrunner/Matchers.js
Outdated
| super(message); | ||
| this.name = this.constructor.name; | ||
| this.formatter = formatter; | ||
| this.location = getLocation(__filename); |
There was a problem hiding this comment.
How does this work? Won't __filename be the Matcher.js file? Why do we care about that file, rather than the spec.js that threw this error.
Update: I got to the getLocation code. It makes more sense. Would it be fair to call this method "getParentFile" or something? I assumed it was doing something very different. I guess its fine.
There was a problem hiding this comment.
getCallerLocation!
| this.name = this.constructor.name; | ||
| this.formatter = formatter; | ||
| this.location = getLocation(__filename); | ||
| Error.captureStackTrace(this, this.constructor); |
There was a problem hiding this comment.
What does this line do? Shouldn't calling super already capture a stack trace?
There was a problem hiding this comment.
That's to filter out the custom error constructor from the stack trace. That's a canonical way to create a custom error in node, e.g. see here
There was a problem hiding this comment.
It's not in firefox, which hurts the portability of the test runner, but I can probably polyfill it.
utils/testrunner/Matchers.js
Outdated
| } | ||
| }; | ||
|
|
||
| class MatchError extends Error { | ||
| constructor(message, formatter = null) { |
There was a problem hiding this comment.
nit: formatter = defaultFormatter
Then you can avoid a null check later.
There was a problem hiding this comment.
formatter is bound at least to the received argument; but the = null part is actually never needed. Thanks!
| @@ -422,7 +395,10 @@ class TestRunner extends EventEmitter { | |||
| const runnableTests = this._runnableTests(); | |||
| this.emit(TestRunner.Events.Started, runnableTests); | |||
| this._runningPass = new TestPass(this, this._rootSuite, runnableTests, this._parallel, this._breakOnFailure); | |||
| const termination = await this._runningPass.run(); | |||
| const termination = await this._runningPass.run().catch(e => { | |||
| console.error(e); | |||
There was a problem hiding this comment.
this looks strange? We log an error and then throw it again? Is something consuming errors or something? Leftover debug code?
There was a problem hiding this comment.
if testrunner.run() is not awaited and there's an issue with testrunner itself, then it crashes silently with an unhandled promise rejection that doesn't have any stack.
This gives the stack.
There was a problem hiding this comment.
Is there a case where this gives you a double error message? Or this is always just problems with the test runner itself, to avoid unhandledpromiserejection woes?
There was a problem hiding this comment.
Yeah, that's always for the bugs in the testrunner itself
This patch re-implements matching and reporting for test runner.
Among other improvements:
expect.toBe()now does text diff when gets strings as inputexpect.toEqualnow does object diffExample output of a failed test: