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 support for running tests and coverage display. #371

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Willyham
Copy link

This change adds the ability for the extension to:

  • Run tests 'on save'
  • Run coverage 'on save'
  • Interpret test results and display them in an output window
  • Interpret coverage data and display it as decorations in the editor.

It is currently built only to support the output format of forge.
New features are not enabled by default (requires configuration).

Configuration options:
image

This results in tests being run on every save, and displayed like so:

image

Coverage bars are written to the gutters for Solidity files:

image

Coverage is recomputed every time a Solidity file is saved, if enabled in the settings.

Performance: works very well for smaller repos. Larger repos can take multiple seconds for the coverage bars to show, and there is no loading indicator. However, I have benchmarked this and only a few milliseconds are taken by the extension, the time is taken by forge, so there's not much we can do there.

@Willyham
Copy link
Author

Hey @juanfranblanco would love to get your thoughts on this. It currently works as is but it's quite a large change so would like to get any feedback before doing any final changes :)

@juanfranblanco
Copy link
Owner

@Willyham very nice, and yes big change, we could have had a chat before hand ;), but this is the chat!. This could be added as such for the time being but that might break stuff for users in the future, so it will be "experimental". Overall after a quick look is great, but it will be ideal to have a forge configuration section (which includes test and coverage), then have a forge as an option for both coverage and test, and finally a command to run test and coverage manually instead on save, so users could just run it on demand if they think is too slow (just based on your comments).

@Willyham
Copy link
Author

I am more than happy to rework all of this if needed. I've been using it for my own projects, and just decided to clean it up and try and contribute it back, so I appreciate that there might be things which need changing.

There is already a command which users can use to trigger running the tests. It'll only run automatically if they enable the 'test on save' boolean option. Same for coverage (though there's no command to invoke manually, but I'll add that).

As you point out the configuration probably needs some more thinking. I wasn't sure how you'd like to handle different 'environments' e.g. forge/hardhat etc. Ideally it would be nice to automatically detect the 'environment' by looking for the presence of files (like foundry.toml) and then setting this configuration directly. So, for example we can have:

Environment/Mode: [forge/hardhat/...]
forge.testCommand: `forge test ...`
forge.coverageCommand: `...`
hardhat.testCommand: `...`

This maybe this is a little redundant and inflexible? Should we just have a mode, and single testCommand and then pick automatic defaults?

@juanfranblanco
Copy link
Owner

juanfranblanco commented Oct 31, 2022

Yeah, i always thought that we should have a common project file that all settings could be put in place for different tooling etc.

This maybe this is a little redundant and inflexible? Should we just have a mode, and single testCommand and then pick automatic defaults?

Hmm, yes you are right, having default values might make it easier, and these can be overidden if required. The default values, can be set on the description for users to override or validate, best of both worlds.

An option for "other" might be needed, which will require the input of the user.

@Willyham
Copy link
Author

Sounds good! So then I would propose that we have:

solidity.developmentEnvironment: [forge, hardhat] // as a dropdown with just these values (for now).

And the existing:

solidity.test.command
solidity.test.coverageCommand
solidity.test.runOnSave
solidity.test.coverOnSave

Then, when the extension loads we check:

  • Can we detect the dev environment via presence of some config files?
  • If forge is detected, and the above settings are blank, we set the workspace setting to default values for the given mode.
  • When invoking the command manually, if no config is set then we prompt the user to set it (for now, we could improve this too).
  • Do nothing as it stands for hardhat, apart from detect the environment.

Does that sound ok?

@juanfranblanco
Copy link
Owner

@Willyham sounds great!

@Willyham
Copy link
Author

Willyham commented Nov 1, 2022

If it makes sense, I'll probably do the environment + detection part as a separate PR, and then follow with this one to try and keep the size down a little

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

Successfully merging this pull request may close these issues.

None yet

2 participants