Make snapshots usable again #1317
Conversation
novemberborn
force-pushed
the
test-overhaul
branch
from
a0bbb19 to
950e71d
Compare
novemberborn
force-pushed
the
make-snapshot-work
branch
from
f664563 to
2952ff1
Compare
novemberborn
force-pushed
the
make-snapshot-work
branch
2 times, most recently
from
b21d2a3 to
ac73897
Compare
lib/runner.js
Outdated
|
|
||
| return Promise.resolve(this.tests.build().run()).then(this._buildStats); | ||
| this.tests.on('test', result => this.addTestResult(result)); | ||
| return new Promise(resolve => { |
There was a problem hiding this comment.
This is exactly what Promise#try is useful for.
There was a problem hiding this comment.
Ah Promise here is indeed from bluebird. I guess I'll rename it to Bluebird.try().
lib/runner.js
Outdated
| this.hasStarted = true; | ||
|
|
||
| return Promise.resolve(this.tests.build().run()).then(this._buildStats); | ||
| this.tests.on('test', result => this.addTestResult(result)); |
There was a problem hiding this comment.
I bit unrelated, but I usually only use inline arrow functions when I actually make use of the return value. This makes it clear whether something is returned or not. What do you think of such approach?
| @@ -0,0 +1,19 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
There was a problem hiding this comment.
We need to change this to AVA Snapshot v1. We don't want to mislead people with being fully compatible with Jest and it's also confusing for AVA users that it doesn't say AVA.
We might need to open an issue on Jest about getting an option for this, but for now I guess we can just read the file in, modify it, and write it out again.
There was a problem hiding this comment.
I think jest-snapshot just treats these files as CommonJS modules so rewriting the header should be harmless. Will do.
There was a problem hiding this comment.
Actually no, jest-snapshot requires snapshot files to start with this header: https://github.com/facebook/jest/blob/38ca942f6f2979ed25f54510173dbd2761dd7e04/packages/jest-snapshot/src/utils.js#L37
We could add a comment after but that becomes annoyingly complex.
There was a problem hiding this comment.
@novemberborn Can you open an issue on Jest to make it configurable? They seem open to making it more generic when needed.
I'm happy to land this as is for now, but long-term we'll want our own naming and versioning.
There was a problem hiding this comment.
Can you open an issue on Jest to make it configurable? They seem open to making it more generic when needed.
I don't know, if you look at the error messages it very much assumes running inside Jest.
I'm happy to land this as is for now, but long-term we'll want our own naming and versioning.
I think we want our own alternative, really.
There was a problem hiding this comment.
Too bad, would have been nice to reuse that code.
// @cpojer
|
This looks very good. I almost couldn't tell the difference with our diff. Usable now is better than perfect at some point. |
* Don't mix option chain with the prototype, simply expose it on the instance. Actual code only exposed the `test` method to the user, and that happens to be the default starting point anyway * `run()` doesn't need to return stats * Ensure `run()` always returns a promise, and does not throw * Simplify `buildStats()` * Derive chainable methods from the actual chain when generating the TypeScript definition * Don't bother prefixing methods / properties with underscores
* Make the Runner manage the snapshot state. Thread an accessor to the `t.snapshot()` assertion. * Save snapshots when runner has finished. Fixes #1218. * Use jest-snapshot directly, without serializing values. Use jest-diff to generate the diff if the snapshot doesn't match. This does mean the output is not colored and has other subtle differences with how other assertions format values, but that's OK for now. Fixes #1220, #1254. * Pin jest-snapshot and jest-diff versions. This isn't ideal but we're using private APIs and making other assumptions, so I'm not comfortable with using a loose SemVer range.
novemberborn
force-pushed
the
make-snapshot-work
branch
from
d2fc018 to
c047d4e
Compare
|
They are still Jest snapshots though, aren't they? If we can, we should keep the header because it looks to me like they are compatible. We can update error messages and the documentation link that it links to, though. Feel free to send PRs to Jest to make the public API more usable as we'll undoubtedly break private APIs in the future. |
Had to start with another refactor, this time of
Runner.Anyway, the cool part is making snapshots work again:
Make the Runner manage the snapshot state. Thread an accessor to the
t.snapshot()assertion.Save snapshots when runner has finished. Fixes Snapshot feature will modify existing snapshots without passing
--update-snapshotsflag #1218.Use jest-snapshot directly, without serializing values. Use jest-diff
to generate the diff if the snapshot doesn't match. This does mean the
output is not colored and has other subtle differences with how other
assertions format values, but that's OK for now. Fixes Snapshot feature shows empty diff when changing order of keys on object #1220, fixes Snapshot is doubly-escaped / hard to see diffs #1254.
Pin jest-snapshot and jest-diff versions. This isn't ideal but we're
using private APIs and making other assumptions, so I'm not comfortable
with using a loose SemVer range.
It's still not ideal (see #1275), especially since the diff output is different from other assertions, but at least it makes snapshots usable.