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

Add --parallel, --bail and --grep params to the test task #2359

Merged
merged 26 commits into from
Feb 23, 2022

Conversation

fvictorio
Copy link
Member

@fvictorio fvictorio commented Feb 9, 2022

Add support for the --parallel, --bail and --grep flags in the test task.

The first commit is just the mocha upgrades, the second one adds the --parallel flag. Notice that this flag also adds hardhat/register to Mocha's required files. This makes parallel tests mostly work out-of-the-box, but there might be a risk here that I'm not seeing.

Pending stuff:

  • Check if hardhat-gas-reporter and solidity-coverage work on parallel mode. Document how to make them work if not, or just document the limitation if making them work is too much effort.
  • Run tests in parallel mode in the e2e tests
  • Add --bail and --grep flags
  • Document this mode, its limitations, and tips.
  • Add changeset.

@changeset-bot
Copy link

changeset-bot bot commented Feb 9, 2022

🦋 Changeset detected

Latest commit: 7a6d522

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
hardhat Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@fvictorio
Copy link
Member Author

solidity-coverage works fine because it has its own coverage task.

hardhat-gas-reporter doesn't work with parallel tests. I created an issue about it, but we should also document that and also be explicit about this in the release notes.

@fvictorio fvictorio marked this pull request as ready for review February 15, 2022 13:27
@fvictorio fvictorio changed the title Add --parallel flag to test task Add --parallel, --bail and --grep params to the test task Feb 15, 2022
docs/guides/waffle-testing.md Outdated Show resolved Hide resolved
docs/guides/waffle-testing.md Outdated Show resolved Hide resolved
docs/guides/waffle-testing.md Outdated Show resolved Hide resolved
const mochaConfig: MochaOptions = {
...config.mocha,
bail,
grep,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should/do we allow these to be configured in config.mocha as well? If so, this would overwrite them (i.e. if they were set in config.mocha but not passed as params to the subtask).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I hadn't noticed that, thanks for pointing it out. I actually think that would be the correct behavior though? Normally CLI flags override what's in the config (for example, that's what Prettier does by default). But it's good to know that this is the case. I will add a comment explaining that this is intentional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait I see what you mean. For the grep flag this works correctly, but bail has a default value, and so it will always override whatever is in the config. I'll fix this, thank you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! I was also tricked by this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -125,6 +125,23 @@ expect(await greeter.greet()).to.equal("Hola, mundo!");

We can modify the state of a contract in the same way we read from it. Calling `setGreeting` will set a new greeting message. After the `Promise` is resolved, we perform another assertion to verify that the greeting change took effect.

### Running tests in parallel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this to its own guide, to make it more visible.

Also, this is not a waffle thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@alcuadrado
Copy link
Member

As discussed offline, this PR looks great, but we should test that the params actually enable those features. @fvictorio suggested using a fixture project for that.

fvictorio and others added 3 commits February 18, 2022 11:57
Co-authored-by: F. Eugene Aumson <feuGeneA@users.noreply.github.com>
Co-authored-by: F. Eugene Aumson <feuGeneA@users.noreply.github.com>
@fvictorio
Copy link
Member Author

@alcuadrado this is ready for review.

@fvictorio
Copy link
Member Author

Blocked by this. The obvious solution is to update our stack traces to not have circular references, but there might be other approaches.

@fvictorio
Copy link
Member Author

We could do something like this.

SolidityError instances cannot be JSON.stringified because they contain circular references. This causes an issue with parallel tests, because mocha tries to serialize errors thrown by tests.

This commit changes the SourceReference interface to prevent these circular references.
Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LFG!

@fvictorio fvictorio merged commit 43dd1d5 into master Feb 23, 2022
@fvictorio fvictorio deleted the parallel-tests branch February 23, 2022 16:10
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants