test(prover): move tests for prover to vitest #6074
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
|
For reviewers, please skip the first commit if you intend to review commit by commit. |
nazarhussain
changed the title
nazarhussain
commented
Oct 30, 2023
package.json
Outdated
| }, | ||
| "resolutions": { | ||
| "dns-over-http-resolver": "^2.1.1" | ||
| "dns-over-http-resolver": "^2.1.1", | ||
| "chai": "^4.3.10" |
There was a problem hiding this comment.
As we are still using chai as direct dev-dependency of our packages, so have to add a resolution to solve conflict between vitest deep dependency.
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
nazarhussain
commented
Oct 30, 2023
| "test:browsers": "test:browsers:chrome && test:browsers:firefox && test:browsers:electron", | ||
| "test:browsers:chrome": "vitest --run --browser chrome --config ./vitest.browser.config.ts --dir test/unit", | ||
| "test:browsers:firefox": "vitest --run --browser firefox --config ./vitest.browser.config.ts --dir test/unit", | ||
| "test:browsers:electron": "echo 'Electron tests will be introduced back in the future as soon vitest supports electron.'", |
There was a problem hiding this comment.
We can bring back electron to our stack, soon when vitest bring it support, else writing a custom BrowserProvider will not be difficult for it.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6074 +/- ##
=============================================
+ Coverage 0 80.02% +80.02%
=============================================
Files 0 19 +19
Lines 0 1717 +1717
Branches 0 155 +155
=============================================
+ Hits 0 1374 +1374
- Misses 0 341 +341
- Partials 0 2 +2 |
nazarhussain
commented
Oct 31, 2023
packages/test-utils/src/cli.ts
Outdated
Comment on lines
33
to
41
| await cli | ||
| .parseAsync(parseArgs(args), {}, (err, _argv, output) => { | ||
| if (err) return reject(err); | ||
|
|
||
| resolve(output); | ||
| }); | ||
| resolve(output); | ||
| }) | ||
| .catch(() => { | ||
| // We are suppressing error here as we are throwing from inside the callback | ||
| }); |
There was a problem hiding this comment.
That was a weird double throw case, but never catched with mocha earlier.
nazarhussain
force-pushed
the
nh/prover-tests
branch
2 times, most recently
from
255d1f7 to
f25a8c7
Compare
nazarhussain
force-pushed
the
nh/prover-tests
branch
from
e140387 to
52c26dc
Compare
nazarhussain
commented
Dec 7, 2023
| @@ -213,7 +214,8 @@ const unknownBlockSync = await env.createNodePair({ | |||
| // unknown block sync can work only if the gap is maximum `slotImportTolerance * 2` | |||
| // default value for slotImportTolerance is one epoch, so if gap is more than 2 epoch | |||
| // unknown block sync will not work. So why we have to increase it for tests. | |||
| "sync.slotImportTolerance": headForUnknownBlockSync.response.data.message.slot / 2 + 2, | |||
| // Adding SLOTS_PER_EPOCH will cover the case if the node starts on the last slot of epoch | |||
| "sync.slotImportTolerance": headForUnknownBlockSync.response.data.message.slot / 2 + SLOTS_PER_EPOCH, | |||
There was a problem hiding this comment.
Eph 8/6 0.320[unknown-block-sync-node-beacon_lodestar] �[32minfo�[39m: Lodestar network=dev, version=v1.12.0/fc67c5e, commit=fc67c5e7bef36b868d69a0bc55d4c33068106e85
....
....
Eph 8/6 1.720[unknown-block-sync-node-beacon_lodestar/sync] �[36mverbose�[39m: UnknownBlockSync enabled.
Eph 8/6 1.720[unknown-block-sync-node-beacon_lodestar/sync] �[34mdebug�[39m: RangeSync disabled.
....
....
Eph 9/7 0.003[unknown-block-sync-node-beacon_lodestar/sync] �[32minfo�[39m: Initiating epoch check for sync progress
....
....
Eph 10/0 0.006[unknown-block-sync-node-beacon_lodestar/sync] �[36mverbose�[39m: UnknownBlockSync disabled.
....
....
As from the logs its seen that UnknownBlockSync disabled right after the Initiating epoch check for sync progress because it was open epoch over epoch since it was started when we calculated the epoch slotImportTolerance. So had to increase the tolerance level more.
g11tech
reviewed
Dec 8, 2023
jeluard
reviewed
Dec 8, 2023
nflaig
reviewed
Dec 8, 2023
There was a problem hiding this comment.
This script is effectively useless now, I am getting a bunch of different warnings when running yarn install
> yarn
yarn install v1.22.19
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
warning Pattern ["buffer@^6.0.3"] is trying to unpack in the same destination "/home/nico/.cache/yarn/v6/npm-buffer-polyfill-6.0.3-2ace578459cc8fbe2a70aaa8f52ee63b6a74c6c6-integrity/node_modules/buffer-polyfill" as pattern ["buffer-polyfill@npm:buffer@^6.0.3"]. This could result in non-deterministic behavior, skipping.
[4/5] Linking dependencies...
warning " > vite-plugin-node-polyfills@0.17.0" has unmet peer dependency "vite@^2.0.0 || ^3.0.0 || ^4.0.0 || ^5.0.0".
warning " > vite-plugin-top-level-await@1.3.1" has unmet peer dependency "vite@>=2.8".
warning Workspaces can only be enabled in private projects.
warning Workspaces can only be enabled in private projects.
[5/5] Building fresh packages...
Done in 56.69s.
This PR just hides this change in a massiv diff without properly mentioning it and disregards previous efforts put into getting rid of these warnings. There has been concerns from multiple users and we know this causes bad UX
There was a problem hiding this comment.
It was discussed in detail before modifying the yarn lint script.
There was a problem hiding this comment.
As mentioned in the lint script there was only one warning warning Pattern ["buffer@^6.0.3"] muted. Other warnings were not produced then otherwise would have been catched by the lint script.
There was a problem hiding this comment.
Related to lock diff, verified locally it was produced after rebasing the unstable on target branch. Obviously had no way to detect it locally for me. We may add some check for it on the CI ti detect uncommitted local changes after the build step.
There was a problem hiding this comment.
It was discussed in detail before modifying the yarn lint script.
Last thing I remember was https://discord.com/channels/593655374469660673/605818244036821012/1169747785462054933, this wasn't really resolved at that time, just disregarded
As mentioned in the lint script there was only one warning warning Pattern ["buffer@^6.0.3"] muted
I don't think the script works (at least not locally)
./scripts/assert_no_yarn_warnings.sh
yarn install v1.22.19 [1/5] Validating package.json... [2/5] Resolving packages... success Already up-to-date. Done in 13.11s.
Running grep -qi -e warning --exclude Pattern \[".*"\] is trying to unpack in the same destination
grep: \[".*"\]: No such file or directory
grep: is: No such file or directory
grep: trying: No such file or directory
grep: to: No such file or directory
grep: unpack: No such file or directory
grep: in: No such file or directory
grep: the: No such file or directory
grep: same: No such file or directory
grep: destination: No such file or directory
No warnings in yarn install --check-files
There was a problem hiding this comment.
Related to lock diff, verified locally it was produced after rebasing the unstable on target branch.
Not an issue, we can easily fix that in a follow up PR
This was referenced Dec 8, 2023
|
🎉 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
Use the consistent testing pattern across the packages.
Description
As we started migrating our tests form
mocha + chai + sinontovitest, this PR migrate further the@lodestar/proverpackage.We had to drop the testing support for the electron for short while during this migration.
Steps to test or reproduce