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

feat: Run separate cairo integration test files as a separate Go test #392

Merged
merged 9 commits into from
May 13, 2024

Conversation

xiaolou86
Copy link
Contributor

@xiaolou86 xiaolou86 commented May 7, 2024

Resolves #201

First checking if the environment variable INTEGRATION_TESTS_FILTERS is set in the console.
And if not, then load it from the .env file.

@xiaolou86
Copy link
Contributor Author

@rodrigo-pino @quasilyte @cicr99 Please help me to review. Thank you very much.

@MaksymMalicki
Copy link
Contributor

Hey, I left a comment on the issue. Can you please check it?

@xiaolou86
Copy link
Contributor Author

@MaksymMalicki I have made this improvement. Thanks for your suggestion.

Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

Great, I hadn't thought about having multiple filters. I think is a nice addition. Just leaving some minor comments

integration_tests/.env Show resolved Hide resolved
integration_tests/cairozero_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

It's great as it is. If you want to improve the PR a little more there are two things we can do:

  1. Given the current code it might be nice to refactor into an object Filter which will have two functions:
    • new where it will initialize the filter
    • filter where it filters the code
  2. Add a section to the readme explaining how to use filters

You can do one or two, or none. Just let me know. In case of none, I'll approve immediately.

@xiaolou86
Copy link
Contributor Author

@rodrigo-pino I have refactored into an object Filter. Thank you very much for your suggestion and review.

Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

Great!

@TAdev0
Copy link
Member

TAdev0 commented May 12, 2024

@xiaolou86 there is a linting error:

Error: Error return value of godotenv.Load is not checked (errcheck)

Can you resolve this?

@xiaolou86
Copy link
Contributor Author

Error: Error return value of godotenv.Load is not checked (errcheck)

@TAdev0 Done. Thank you very much.

@rodrigo-pino rodrigo-pino merged commit 17152cb into NethermindEth:main May 13, 2024
4 checks passed
@xiaolou86 xiaolou86 deleted the integration_tests_filter branch May 13, 2024 09:14
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.

Run separate cairo integration test files as a separate Go test
4 participants