-
Notifications
You must be signed in to change notification settings - Fork 42
chore(test): collect cli output in tests #2198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
chore(test): collect cli output in tests #2198
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2198 +/- ##
==========================================
+ Coverage 76.52% 76.59% +0.07%
==========================================
Files 168 168
Lines 7109 7131 +22
Branches 1092 1101 +9
==========================================
+ Hits 5440 5462 +22
Misses 1663 1663
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
await runMutationTest(caseDir); | ||
await expect(actualContent).toMatchFileSnapshot(expectedFilePath); | ||
expect(options).toMatchSnapshot("options"); | ||
expect.assertions(3); |
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.
[Refactor] 😬 I really, really dislike expect.assertions
. It's super brittle: relying on implementation details cross-test. If you ever rework a test, add a utility that runs multiple assertions, etc. you end up having to change a bunch of otherwise-seemingly-arbitrary numbers. Plus it's not comprehensive - if some async logic gets added to tests and the author forgets to update a number, it can fail silently or in some future tests. Spooky.
Request: remove the expect.assertions(...)
calls. If there are places that would need that logic, we can always look at them individually. (are there?)
await expect(actualContent).toMatchFileSnapshot(expectedFilePath); | ||
expect(options).toMatchSnapshot("options"); | ||
expect(output).toMatchSnapshot("output"); |
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.
[Refactor] Having multiple assertions in a test means we only get insights from one failure at a time. If assertion 1 of 3 fails, we don't know at a glance if 2 or 3 did.
My normal trick for this kind of thing is:
await expect(actualContent).toMatchFileSnapshot(expectedFilePath); | |
expect(options).toMatchSnapshot("options"); | |
expect(output).toMatchSnapshot("output"); | |
await expect({ actualContent, options, output }).toMatchFileSnapshot(expectedFilePath); |
PR Checklist
status: accepting prs
Overview
Collect CLI output in integration tests.
Added mutation stats to "log" output so we can see what kind of changes there are in integration tests.
I also added
checkTestResult
function so I didn't have to change every test to include output snapshotting. Which meant that I had to change every test. 😅 If no snapshot is reported as unneeded, then I should have changed every test successfully.