-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Continue jestifying tests #11026
Continue jestifying tests #11026
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
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.
This all looks really modern and improved -- just a few comments!
entry3.response === null, | ||
'all entries should be in correct order and without responses', | ||
); | ||
// expect( |
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.
Can we remove this?
EXPECTED_HISTORIES.case1[1], | ||
'should have correct history', | ||
); | ||
expect(permHistory).toStrictEqual(EXPECTED_HISTORIES.case1[1]); |
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.
Are we going to miss comments like 'should have correct history'
?
Builds ready [f909ef2]
Page Load Metrics (664 ± 48 ms)
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 job switching tests many more tests to Jest, I believe it's a great improvement! I have left just one comment for a nice-to-have improvement, otherwise it looks good to me (assuming conflicts are resolved) 🟢
@@ -1,5 +1,5 @@ | |||
/* eslint-disable jest/no-conditional-expect */ |
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.
Could we set this ESLint rule globally instead of having it in individual files?
Ommited
due to a issues with the ganache server instance, poorly written, and general compatibility with jest. Most likely will have to rethink/rewrite these from the ground up.
Pinned jest-enviornment-jsdom to 25 for Uint8Array Buffer issue in jest <= 26, jestjs/jest#7780