test: support .env.test to reuse env variables across the tests #6408
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
matthewkeil
requested changes
Feb 7, 2024
packages/cli/package.json
Outdated
| "test:sim:endpoints": "LODESTAR_PRESET=minimal node --loader ts-node/esm test/sim/endpoints.test.ts", | ||
| "test:sim:deneb": "LODESTAR_PRESET=minimal node --loader ts-node/esm test/sim/deneb.test.ts", | ||
| "test:sim:backup_eth_provider": "LODESTAR_PRESET=minimal node --loader ts-node/esm test/sim/backup_eth_provider.test.ts", | ||
| "test:sim:multifork": "DOTENV_CONFIG_PATH=../../.env.test LODESTAR_PRESET=minimal node -r dotenv/config --loader ts-node/esm test/sim/multi_fork.test.ts", |
There was a problem hiding this comment.
should LODESTAR_PRESET go into the .env?
There was a problem hiding this comment.
Yes we can add that there as well. Need to check if any other test file depend on other preset.
There was a problem hiding this comment.
Ok. Then let's definitely move that so we are using one pattern (ie and env file)
vitest.base.unit.config.ts
Outdated
| @@ -1,6 +1,6 @@ | |||
| import path from "node:path"; | |||
| import {defineConfig} from "vitest/config"; | |||
| const __dirname = new URL(".", import.meta.url).pathname; | |||
| const currentDir = new URL(".", import.meta.url).pathname; | |||
There was a problem hiding this comment.
I’m partial to leaving this as __dirname. I dont feel strongly about it but it’s sort of the “node way” to name that
There was a problem hiding this comment.
Yes I had that initially as __dirname here and other places as well, but feel like a old pattern now.
There was a problem hiding this comment.
For consistency with the rest of the codebase __dirname is better, but no strong opinion here
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
reviewed
Feb 7, 2024
There was a problem hiding this comment.
It a bit counter intuitive to me to have .env files in source control. I am rather used to having those files to override locally and explicitly not have them tracked by git.
Why can't we just set the default image versions in scripts/run_e2e_env.sh and allow to override them via env variables?
We could consider having dotenv module to override those but seems a bit overkill as there are not many envs and you can just add GETH_DOCKER_IMAGE=... in front of the yarn command to achieve this easily if someone wants to use a different version than the default one.
I am also not in favor of keeping
Not now but if needed we can make this setup in a way that you can override those variables in |
But I like the de-duplication of setting the version which is much better than before Edit: It seems to be convention to have .env.test as part of source control So I guess we are good to go |
nflaig
reviewed
Feb 7, 2024
vitest.base.unit.config.ts
Outdated
| @@ -1,6 +1,6 @@ | |||
| import path from "node:path"; | |||
| import {defineConfig} from "vitest/config"; | |||
| const __dirname = new URL(".", import.meta.url).pathname; | |||
| const currentDir = new URL(".", import.meta.url).pathname; | |||
There was a problem hiding this comment.
For consistency with the rest of the codebase __dirname is better, but no strong opinion here
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6408 +/- ##
============================================
+ Coverage 60.14% 61.72% +1.57%
============================================
Files 407 553 +146
Lines 46512 57856 +11344
Branches 1551 1829 +278
============================================
+ Hits 27975 35711 +7736
- Misses 18505 22108 +3603
- Partials 32 37 +5 |
nflaig
previously approved these changes
Feb 8, 2024
nflaig
reviewed
Feb 8, 2024
nflaig
previously approved these changes
Feb 9, 2024
…r into nh/share-env-variables
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
matthewkeil
requested changes
Feb 10, 2024
There was a problem hiding this comment.
FWIW I am sorry I brought up the LODESTAR_PRESET. Apologies!! I didn’t realize the amount of confusion it would insert because of how we use the two for different tests interchangeably. Thank you for the thoroughness in working through those challenges.
Just the two questions about the inserts and the update for getting the BASH dirname
| "test:unit:minimal": "vitest --run --segfaultRetry 3 --dir test/unit/", | ||
| "test:unit:mainnet": "LODESTAR_PRESET=mainnet vitest --run --dir test/unit-mainnet", | ||
| "test:unit:minimal": "LODESTAR_PRESET=minimal vitest --run --segfaultRetry 3 --dir test/unit/", | ||
| "test:unit:mainnet": "LODESTAR_PRESET=mainnet vitest --run --segfaultRetry 3 --dir test/unit-mainnet", |
There was a problem hiding this comment.
I noticed this segfaultRetry was added. what does that do?
There was a problem hiding this comment.
It was earlier added in minimal preset and missing in mainnet. It just check if tests fails with a segfault error, it tries to rerun it.
| @@ -14,6 +14,7 @@ const {forkConfig} = defineSimTestConfig({ | |||
| BELLATRIX_FORK_EPOCH: bellatrixForkEpoch, | |||
| CAPELLA_FORK_EPOCH: capellaForkEpoch, | |||
| runTillEpoch: Infinity, | |||
| initialNodes: 2, | |||
There was a problem hiding this comment.
i noticed this was added. what does that do?
There was a problem hiding this comment.
It adjust the genesis delay for the e2e notes, without that default is a bit higher value, so tests take more time to wait for capella fork.
scripts/run_e2e_env.sh
Outdated
| @@ -1,18 +1,19 @@ | |||
| #!/bin/bash | |||
|
|
|||
| DIR=$(dirname "$0") | |||
There was a problem hiding this comment.
I’ve seen this done like the below before.
DIR="$(CDPATH= cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"It ensures that the directory is obtained correctly even if the script is executed from a different directory or via a symbolic link
@matthewkeil No problem at all. We had to clean up today or later anyway. |
matthewkeil
previously approved these changes
Feb 10, 2024
There was a problem hiding this comment.
Sweet!! 🎸 I haven't run with the most recent changes but assuming it works. Although I noticed that CI was not happy. Not sure if that is related to these changes though.... A goal to shoot for is all green CI. If I can help I will be glad to lend a hand. Just point me in a direction and ill do my best
nflaig
previously approved these changes
Feb 10, 2024
nazarhussain
dismissed stale reviews from nflaig and matthewkeil
via
6b3522d
matthewkeil
previously approved these changes
Feb 12, 2024
nflaig
reviewed
Feb 12, 2024
nflaig
approved these changes
Feb 12, 2024
|
🎉 This PR is included in v1.16.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
Reuse the env variables.
Description
.env.testSteps to test or reproduce