test: migrate api unit tests to vitest #6177
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nazarhussain
commented
Dec 11, 2023
Comment on lines
+12
to
+13
| const root = new Uint8Array(32).fill(1); | ||
| const randao = new Uint8Array(32).fill(1); |
There was a problem hiding this comment.
One observation for reviewers, it seems that chai was matching the Buffer and Uint8Array equality. Which is logically wrong, if we are expecting a Buffer instance of a value then assertion should pass otherwise fail.
For this reason have to update a lot of fixture data in this PR, converting Buffer to actual returning Uint8Array instances.
nazarhussain
commented
Dec 11, 2023
Comment on lines
48
to
54
| args: [ | ||
| 32000, | ||
| randaoReveal, | ||
| graffiti, | ||
| undefined, | ||
| {feeRecipient: undefined, builderSelection: undefined, strictFeeRecipientCheck: undefined}, | ||
| ], |
There was a problem hiding this comment.
sinon assertions were passing with partial arguments as well. To pass with the vitest we have to add the full arguments that were used to call that function.
That's because we are reusing reqSerializer for produceBlockV3 for other methods as well.
lodestar/packages/api/src/beacon/routes/validator.ts
Lines 608 to 611 in 1a63b07
To make it logically correct we should have different serializers for each method. Would be nice to know why it's actually written in this way in first place. @g11tech Do you remember any rationale for it?
There was a problem hiding this comment.
because produceBlockV3 serializer is superset of all of them
There was a problem hiding this comment.
you can create separate serializer fns for each of these but then delegate the call to produceBlockV3 inside it with so that you may not need to pass undefined values here
but we anyway will remove these fns and not support them so we can leave it like that as well
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6177 +/- ##
=============================================
+ Coverage 80.02% 90.35% +10.33%
=============================================
Files 19 78 +59
Lines 1717 8089 +6372
Branches 155 490 +335
=============================================
+ Hits 1374 7309 +5935
- Misses 341 772 +431
- Partials 2 8 +6 |
Performance Report✔️ no performance regression detected Full benchmark results
|
g11tech
approved these changes
Dec 11, 2023
nflaig
reviewed
Dec 11, 2023
| await expect(fetch(url, {signal: signalHandler?.()})).to.be.rejected.then((error: FetchError) => { | ||
| expect(error.type).to.be.equal(testCase.errorType); | ||
| expect(error.code).to.be.equal(testCase.errorCode); | ||
| await expect(fetch(url, {signal: signalHandler?.()})).rejects.toSatisfy((err) => { |
There was a problem hiding this comment.
can't this be changed to?
Suggested change
| await expect(fetch(url, {signal: signalHandler?.()})).rejects.toSatisfy((err) => { | |
| await expect(fetch(url, {signal: signalHandler?.()})).rejects.toSatisfy((err: FetchError) => { |
There was a problem hiding this comment.
Not that's the strict type issue we can't assign type inside the function descriptor.
Argument of type '(err: FetchError) => true' is not assignable to parameter of type '(value: unknown) => boolean'
|
🎉 This PR is included in v1.13.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Consolidate the testing frameworks and migrate to vitest.
Description
Migrate
apiunit tests to vitest.Steps to test or reproduce