-
-
Notifications
You must be signed in to change notification settings - Fork 508
refactor: migrate to jest assertion #698
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?
Conversation
3b36cb0 to
cc15a42
Compare
|
@valscion ready for review |
valscion
left a comment
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.
👏 great work!
| // On node.js v16 and lower, the calculated gzip is one byte larger. Nice. | ||
| gzipSize: | ||
| parseInt(process.versions.node.split(".")[0]) <= 16 ? 360 : 358, | ||
| parsedSize: 1349, |
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.
Huh. Why did the parsedSize change here and in some other assertions here as well?
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.
Looks like chai has bugs here, as you can see we don't change something here except migration to jest assertion
| await expectValidReport({ parsedSize: 1317, gzipSize: 341 }); | ||
| }); | ||
|
|
||
| it.only("should support brotli", async function () { |
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.
Was this the reason? We had accidentally left a it.only here? 😬
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.
Yes, it is a fix
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.
We also have skip tests in some places, I will fix them in other PRs
Summary
Use jest assertion
What kind of change does this PR introduce?
refactor test
Did you add tests for your changes?
Existing
Does this PR introduce a breaking change?
No
If relevant, what needs to be documented once your changes are merged or what have you already documented?
Nothing