Skip to content
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

feature: support hardhat test --parallel flag #691

Open
cgewecke opened this issue Feb 11, 2022 · 11 comments
Open

feature: support hardhat test --parallel flag #691

cgewecke opened this issue Feb 11, 2022 · 11 comments

Comments

@cgewecke
Copy link
Member

HH is supporting mocha parallelism soon. Would like to make sure the data is aggregated correctly (not sure how this works).

We have an existing recipe for knitting together reports from different jobs if that's necessary (see #662).

cf: hh 2359

@cgewecke cgewecke changed the title Double check output with hardhat test --parallel flag Check output with hardhat test --parallel flag Feb 15, 2022
@fvictorio
Copy link
Collaborator

There is at least one thing that doesn't work when running the coverage task and enabling parallel tests in mocha: the allowUnlimitedContractSize flag is not being set. And I think any config override just doesn't work in parallel mode.

@cgewecke
Copy link
Member Author

From the HH PR referenced above:

In serial mode all the test files share the same instance of the HRE, but in parallel mode this is not always the case. Mocha uses a pool of workers to execute the tests, and each worker starts with its own instance of the HRE. This means that if one test file deploys a contract, then that deployment will exist in some of the other test files and it won't in others

@fvictorio Does each worker getting its own HRE happen automagically in Mocha or is this something you're setting up in a mocha reporter hook somewhere?

@fvictorio
Copy link
Collaborator

It happens because we add something like --require hardhat/register.

If you do node --register hardhat/register script.js, then the script will have a HRE initialized and available. For Hardhat tests in parallel mode, each test file is executed in a similar way. But the main test task (and the ones overriden by solidity-coverage) are in the parent process, and so any change to the HRE is not reflected in the test files.

Does that make sense?

@cgewecke
Copy link
Member Author

@fvictorio Yes makes sense, thanks.

Not sure but suspect that without a provider initialization task hook to override supporting mocha parallel here might be difficult. (If that's too pessimistic lmk).

Another strategy might be:

  • detect when the parallel flag is set in mocha options and unset it
  • print a warning
  • create docs which show recipes for generating coverage in parallel-izable CI environments. More info about that in:

Does this sound reasonable? (Open to anything fwiw)

(register code is here)

@fvictorio
Copy link
Collaborator

Sorry for not responding sooner @cgewecke!

I think the best and easiest solution is to detect that parallel mode is being used and throw an error with instructions? A warning could work too, but maybe it's too easy to miss, and things are not going to work as expected more generally.

@cgewecke
Copy link
Member Author

@fvictorio No worries! Your idea sgtm, will do.

@cgewecke
Copy link
Member Author

With 0.8.6 an error is thrown when parallel is configured for mocha ... the error advises the user to override the option in the mocha config you can add to .solcover.js. There's also a new section in the docs about running coverage in parallel in CI.

@cgewecke cgewecke changed the title Check output with hardhat test --parallel flag feature: support hardhat test --parallel flag Feb 10, 2024
@cgewecke
Copy link
Member Author

cgewecke commented Aug 30, 2024

Looking at this again...I think it might be possible to support the flag using the same strategy used for hardhat-viem, e.g require that the user signal they're running the coverage task using an environment variable.

SOLIDITY_COVERAGE=true npx hardhat coverage --parallel

This allows us to initialize the provider correctly before the task begins and then we'd need to:

  • use mocha root hooks to make sure each provider is coverage enabled? (investigate)
  • dump the results to a cache as each test thread completes? (investigate)
  • merge the results as the task exits.

@fvictorio
Copy link
Collaborator

@cgewecke if that works, then why not just do process.env.SOLIDITY_COVERAGE = true in the definition of the coverage task?

@cgewecke
Copy link
Member Author

cgewecke commented Sep 5, 2024

@fvictorio Sorry, I might be misunderstanding the problem.

I thought if allowUnlimitedSizeContract... etc gets set in Hardhat's extendConfig hook, the provider initialization issues would be solved, because that hook propagates to the children. Is that wrong?

And the environment variable would be used so that coverage-specific extendConfig logic only runs for the coverage task - atm it's not possible to determine which task was invoked in extendConfig from the args passed to the hook.

EDIT

Ah apologies, I've gotten confused about what's visible in the parents/children. You're saying that if I set the process env variable internally in the parent, the children will inherit it?

@cgewecke
Copy link
Member Author

cgewecke commented Sep 6, 2024

@fvictorio

Update: made a sample project to see when all the hooks run and I understand how this can work now. Please ignore questions in previous comment...will try to work up a PR for this tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants