-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
consolidate integration tests into itests
package; create test kit; cleanup
#6311
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks quite a bit better than the current state. Couple of notes:
- Some tests seem to be failing with bad rpc multiaddr -> https://app.circleci.com/pipelines/github/filecoin-project/lotus/14429/workflows/fbcf76f9-6f5f-4960-b916-8afbe7eaf7c3/jobs/150402
- Other tests seem to be failing likely due to mixing mock and non-mock nodes -> https://app.circleci.com/pipelines/github/filecoin-project/lotus/14429/workflows/fbcf76f9-6f5f-4960-b916-8afbe7eaf7c3/jobs/150417
- We may need to update pointers to tests in Circle config -> https://github.com/filecoin-project/lotus/blob/master/.circleci/config.yml#L712-L747 (or not?)
This is ready for final review and merge @magik6k 🎉 The merge was quite a pain, but finally done. Not sure if the |
Actually: 🟢🟢🟢🟢🟢 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment, not blocking merge on it as it looks like currently we're not including this package outside of tests, but it should be addressed a followup PR
if err != nil { | ||
panic(fmt.Sprintf("failed to set BELLMAN_NO_GPU env variable: %s", err)) | ||
} | ||
build.InsecurePoStValidation = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do something which makes sure we never include this package by accident in non-test builds, setting this variable in real lotus could be pretty catastrophic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(e.g. a build-time check like https://github.com/filecoin-project/lotus/blob/master/api/api_test.go#L28)
Integration tests in Lotus are pretty coarse-grained. They exercise the whole Lotus stack top to bottom... i.e. they act like end-to-end tests.
They are quite a labyrinth too, as they are scattered between packages, and there's quite a bit of indirection. Folks that wrote new test cases ended up reinventing the wheel (mining loops, builders, etc.) because existing reusable code was hard to discover.
This PR aims to bring some initial order to the chaos.
This PR:
itests
package.itests/kit
) package.