Introduce snapshot testing utilities #11375
Conversation
jeffsee55
requested review from
TooTallNate,
EndangeredMassa,
trek,
onsclom and
erikareads
as code owners
🦋 Changeset detectedLatest commit: 8a1fc89 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| await expect(exitCodePromise).resolves.toEqual(0); | ||
|
|
||
| await expect(client.getFullOutput()).not.toContain(emoji('tip')); |
There was a problem hiding this comment.
When we do .not.toOutput it seems like that would guarantee that our test will wait the full 3 seconds for each one of those statements. But i wonder if we can instead just assert that the "full output" doesn't contain the string, as shown here
There was a problem hiding this comment.
Correct, .not.toOutput has to wait for a timeout, so we generally try to avoid it.
| // This is probably fine since we don't care about those for testing; but this could become | ||
| // an issue if we ever want to test for those. | ||
| if (stripAnsi(str).trim().length > 0) { | ||
| this.#_chunks.push(str); |
There was a problem hiding this comment.
question (non-blocking): should this push the stripped version instead of the original str?
There was a problem hiding this comment.
I believe the idea here is to determine if we should log this string. As in, if we remove any ansi sequences from the string, is there anything to show? If so, add it. And then the actual stripping is done in the getScreen call.
For context, this is copied from inquirer test https://github.com/SBoudrias/Inquirer.js/blob/master/packages/testing/src/index.mts
packages/cli/test/mocks/client.ts
Outdated
| getScreen({ raw }: { raw?: boolean } = {}): string { | ||
| const stderr = client.stderr as any as MockStream; | ||
| const lastScreen = stderr.getLastChunk({ raw }); | ||
| return raw ? lastScreen : stripAnsi(lastScreen).trim(); | ||
| } | ||
| getFullOutput(): string { | ||
| const stderr = client.stderr as any as MockStream; | ||
| return stderr.getFullOutput(); | ||
| } |
There was a problem hiding this comment.
suggestion (soft-blocking): if we can remove type assertions, that'd be great
When I remove this locally, it seems to work fine.
| getScreen({ raw }: { raw?: boolean } = {}): string { | |
| const stderr = client.stderr as any as MockStream; | |
| const lastScreen = stderr.getLastChunk({ raw }); | |
| return raw ? lastScreen : stripAnsi(lastScreen).trim(); | |
| } | |
| getFullOutput(): string { | |
| const stderr = client.stderr as any as MockStream; | |
| return stderr.getFullOutput(); | |
| } | |
| getScreen({ raw }: { raw?: boolean } = {}): string { | |
| const lastScreen = client.stderr.getLastChunk({ raw }); | |
| return raw ? lastScreen : stripAnsi(lastScreen).trim(); | |
| } | |
| getFullOutput(): string { | |
| return client.stderr.getFullOutput(); | |
| } |
There was a problem hiding this comment.
Yes makes sense. I think I was trying to do some type fixes to the MockClient (that didn't work) and this was some leftover code.
There was a problem hiding this comment.
Regarding the type errors, these are kind of everywhere and have been for a little while as far as I know. Not related to this PR, unless you're seeing something different. I don't think there's a great solution to fix this, have a draft PR here that I've been meaning to revisit.
…ub.com/vercel/vercel into jeffsee55/introduce-snapshot-utilities
jeffsee55
dismissed stale reviews from EndangeredMassa and TooTallNate
via
4b6b939
jeffsee55
dismissed stale reviews from TooTallNate and EndangeredMassa
via
8fa2646
| // it incorrectly detects that unicode is not supported, | ||
| // and thus prints ascii characters, which breaks these tests. | ||
| // https://github.com/SBoudrias/Inquirer.js/issues/1386 | ||
| describe.skipIf(isWindows)('client.input', () => { |
There was a problem hiding this comment.
Just skipping these on Windows
There was a problem hiding this comment.
Thanks for the comment in code about this. Makes sense.
| // it incorrectly detects that unicode is not supported, | ||
| // and thus prints ascii characters, which breaks these tests. | ||
| // https://github.com/SBoudrias/Inquirer.js/issues/1386 | ||
| describe.skipIf(isWindows)('client.input', () => { |
There was a problem hiding this comment.
Thanks for the comment in code about this. Makes sense.
This introduces some additional capabilities on the
MockClientandMockStreamclasses used in our unit tests.The bulk of this code is copied over from https://github.com/SBoudrias/Inquirer.js/tree/master/packages/testing, but uses our existing stdin/stdout mocks instead of what's used there.
You can now use
client.getScreen()to see the "last" screen pushed to stdin, which is super handy for snapshot tests. I've added a few as part of this PR, but many more can be written for our client output as well in the near future.The
client.getScreen()is also a handy debug tool, use it withraw: trueto see a pretty printed output during tests:Screen.Recording.2024-04-03.at.12.13.42.PM.mov
This also adds an
eventsobject on theMockClientwhich allows us to writeclient.events.keypress("down")instead of the less readableclient.stdin.write('\x1B[B')throughout our testing. This could also be moved to so that it'sclient.stdin.keypress(...or something similar. Note that I've only tested that this works in a login test, included in this PR. If we're happy to make this change I will make update the rest of the unit tests to use this syntax for the inputs.