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

Move test utils in the src dir #206

Closed

Conversation

andreivladbrg
Copy link
Contributor

@andreivladbrg andreivladbrg commented Oct 23, 2023

Changelog:

refactor: move test utils in src dir
build: install prb test as npm package
build: remove prb test git module
ci: install node js dependencies in build and test job
chore: update remappings accordingly

As mentioned in my comment here, importing files from the ~test/ path is not possible in the NPM package because this folder doesn't exist.

Inevitably, to make this fully compatible with the idea presented in the discussion linked above, I had to install prb-test as an NPM package.

build: install prb test as npm package
build: remove prb test git module
chore: update remappings accordingly
ci: install node js dependencies in test job
@andreivladbrg
Copy link
Contributor Author

Context: here, StdUtils is imported, assuming a remappings set for forge-std/=lib/forge-std/src/
exists.

Question: Should forge-std also be installed as a node module?
My initial answer was no, because if there are other projects that need these test utils they are also working on a foundry project (with forge-std remapping considered as the default configuration), otherwise, if one is testing in Hardhat, a contract as a util would not be needed, no?

Am I missing something and we should install as NPM package like this "@forge-std": "github:foundry-rs/forge-std#commit_hash"?

@PaulRBerg
Copy link
Owner

Thanks @andreivladbrg, just letting you know that it will take me a while until I find the time to pick this PR up.

Copy link
Owner

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

looks good but could you also please install Forge Std from npm?

package.json Outdated Show resolved Hide resolved
@PaulRBerg
Copy link
Owner

because if there are other projects that need these test utils they are also working on a foundry project (with forge-std remapping considered as the default configuration), otherwise, if one is testing in Hardhat, a contract as a util would not be needed, no?

Foundry should support nested remappings but I agree that it would be worth testing the set-up before merging this PR.

@andreivladbrg
Copy link
Contributor Author

andreivladbrg commented Oct 30, 2023

Addressed everything in my latest commits.

@PaulRBerg
Copy link
Owner

I've changed my mind about this PR.

Instead of moving the test utils in src, we could instead whitelist the test/utils directory to be included in the npm package.

We just need to add test/utils here:

prb-math/package.json

Lines 16 to 19 in abdf0b2

"files": [
"src",
"CHANGELOG.md"
],

test: change path of test utils
@PaulRBerg
Copy link
Owner

Thanks @andreivladbrg for the latest commit, but I would prefer to open a new PR to start with a clean slate!

@andreivladbrg
Copy link
Contributor Author

Clossing in favor of #211

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.

2 participants