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

test: Basic test setup #399

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AndreMiras
Copy link

Make sure the tests can be ran easily.
This is the first of a list of change aiming at improving the test setup. We will try to keep the changes simple and iterate rather than making a giant pull request that would be painful to be reviewed and merged.

Note that the rpcUrls.test.js file was added back to tracking and leverages process.env. Alternatively we could add dotenv as a dev dependency and use (untracked) .env files for RPC URLs.

A lot of tests are still failing but at least some pass. Fixing tests will be addressed in follow up pull requests.

Tested with both Hardhat and Anvil exporting ETH_RPC as:

export ETH_RPC=http://127.0.0.1:8545

In follow up pull requests we want to:

  • fix or skip broken tests
  • run tests from the CI

@AndreMiras
Copy link
Author

@fedorovdg or @Macket would you mind to take a look and let me know if I should edit anything?
This change doesn't cover everything of course, but I have a few follow up change to improve testing in general.
My final goal in fact is to bring up a couple of performance improvement and bring better extensibility of the library, but I would prefer to first improve the tests state and coverage

Make sure the tests can be ran easily.
This is the first of a list of change aiming at improving the test setup.
We will try to keep the changes simple and iterate rather than making a
giant pull request that would be painful to be reviewed and merged.

Note that the `rpcUrls.test.js` file was added back to tracking and
leverages `process.env`. Alternatively we could add dotenv as a dev
dependency and use (untracked) `.env` files for RPC URLs.

A lot of tests are still failing but at least some pass.
Fixing tests will be addressed in follow up pull requests.

Tested with both Hardhat and Anvil exporting `ETH_RPC` as:
```
export ETH_RPC=http://127.0.0.1:8545
```

In follow up pull requests we want to:
- fix or skip broken tests
- run tests from the CI
@AndreMiras AndreMiras force-pushed the feature/basic_test_setup branch from a60fd64 to e11f08a Compare October 13, 2024 09:22
Copy link
Contributor

@DanielSchiavini DanielSchiavini left a comment

Choose a reason for hiding this comment

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

I've tried to get the tests to run in #389
however, even locally, I didn't manage to make them actually pass

@AndreMiras
Copy link
Author

I've tried to get the tests to run in #389 however, even locally, I didn't manage to make them actually pass

Hey @DanielSchiavini,

Thanks for getting back to me and for sharing the draft 🙏

I agree, it's a huge task and I believe we can get there step by step.
I'd be happy to contribute, though I think it's important to tackle this incrementally rather than trying to solve everything in one go.

I also noticed that we're running tests across all possible pools, but I don't think it's necessary for decent library coverage. I'd like to focus on refining that part of the tests. As I mentioned, addressing everything at once could make it hard to move forward.

Once this PR is merged, I can work on getting a smaller subset of tests running and demonstrate it through CI. Then, as more tests are fixed, we can gradually expand coverage and integrate that into the CI process.

Would you mind merging this PR if you think it’s a small step toward achieving the larger goal?

Copy link
Contributor

@DanielSchiavini DanielSchiavini left a comment

Choose a reason for hiding this comment

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

It looks fine but please wait for @Macket, he has a week off

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