atelet: add successful RPC boundary tests#469
Conversation
089f487 to
711e3bb
Compare
|
Dmitry Berkovich (@dberkov) ptal, thanks! |
6f40c26 to
02ab98b
Compare
Dmitry Berkovich (dberkov)
left a comment
Dmitry Berkovich (dberkov)
left a comment
There was a problem hiding this comment.
minor comment, could you please fix it.
| // BasePath is both the path of the root shared folder on the host filesystem, | ||
| // and where it is mounted into ateom and atelet containers. It is a variable | ||
| // so tests can redirect filesystem operations to a temporary directory. | ||
| var BasePath = "/var/lib/ateom-gvisor" |
There was a problem hiding this comment.
making mutable global variable might introduce issues. Can it stay const or could you make it Method too, similar to StaticFilesDir()?
There was a problem hiding this comment.
Thanks for the feedback. I kept BasePath constant and added an instance-local path mapper for tests, so they no longer mutate global state.
02ab98b to
9c32828
Compare
9c32828 to
1cffd6f
Compare
There was a problem hiding this comment.
Are we injecting a full fake filesystem? If not, this isn't really a unit test, these methods may interact with other paths than ateom-path.
Ordinarily I'm all for test coverage ... but I'm not sure I understand what sort of bugs we intend to catch with this, while we're making the code more indirect.
I don't think we should "unit" test main(). And I'm not sure integration testing atelet makes much sense (vs e2e with ateom).
EDIT: keep in mind atelet now also does things like mount for OCI layers, which is not at all unit-test like, and I think it will be unnecessarily complex to fake out interfaces like this.
Benjamin Elder (@BenTheElder) Thanks for the feedback. Following #217, the goal was to cover atelet’s request handling and local checkpoint/restore flow. I checked the existing e2e suite and confirmed that it already covers the Run, Checkpoint, and Restore lifecycle with real atelet and ateom. Given that coverage, I agree the additional test seam may not justify the added indirection. |
3 participants
Summary
Testing
go test ./cmd/atelet ./internal/ateompathgo test ./...Closes #217