test: fix flaky e2e tests #6231
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
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6231 +/- ##
=========================================
Coverage 80.81% 80.81%
=========================================
Files 185 185
Lines 17964 17964
Branches 1082 1082
=========================================
Hits 14517 14517
Misses 3421 3421
Partials 26 26 |
jeluard
reviewed
Dec 22, 2023
| } catch (e) { | ||
| expect.fail(message); | ||
| } | ||
| expect(a).toEqualWithMessage(expect.arrayContaining(b), message); |
There was a problem hiding this comment.
Would it be sufficient to ensure that both array have the same size, then check with arrayContaining only once? Could act as a fail fast too.
Also not sure what should be the expected behavior with duplicates.
There was a problem hiding this comment.
This gives you a better error message if one array contains additional elements, instead of just a size is different error you will get the extraneous element printed out
There was a problem hiding this comment.
Also not sure what should be the expected behavior with duplicates.
Duplicates are fine as long as both arrays contain same count of elements. Or any specific case you had in mind?
There was a problem hiding this comment.
That's very much a cornercase, but on the top of my head I would expect expectDeepEquals(['a', 'a', 'b'], ['a', 'b', 'b']) to pass.
There was a problem hiding this comment.
I guess you mean expectDeepEqualsUnordered but yeah that would pass, the built-in assertions in vitest are just sad... we might wanna consider adding something like https://github.com/jest-community/jest-extended which has toIncludeSameMembers.
I mean the fact that there is no builtin way to do this (or at least I did not find any) makes no sense to me. There is a stackoverflow thread on the topic https://stackoverflow.com/questions/32103252/expect-arrays-to-be-equal-ignoring-order.
There was a problem hiding this comment.
seems to be quite easy to add but not sure if it's worth just for this case https://jest-extended.jestcommunity.dev/docs/getting-started/setup#use-with-vitest
There was a problem hiding this comment.
Added an additional length check now to catch cases like
expectDeepEqualsUnordered([{a: "a"}, {a: "a"}, {b: "b"}], [{a: "a"}, {b: "b"}, {b: "b"}, {b: "b"}])I honestly think this is sufficient, but let me know if you have another idea
Performance Report✔️ no performance regression detected Full benchmark results
|
nazarhussain
approved these changes
Dec 25, 2023
|
🎉 This PR is included in v1.14.0 🎉 |
ensi321
pushed a commit
to ensi321/lodestar
that referenced
this pull request
Jan 22, 2024
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
Fixes flaky e2e tests since we merged #6192.
As mentioned in #6192 (comment), the
expectDeepEqualsUnorderedfunction does not work as expected. Sorting an array of objects does not work like that, we would have to sort by pubkey for our specific use case but since this is supposed to be a generic function I usedexpect.arrayContainingwhich if called on both arrays works similar toto.have.deep.members.I also noticed that the test does not print the diff on failure and just the message is not very helpful to diagnose the issue.
Description
Previous error on failure
Updated error on failure