-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conformance Testing Setup for Shim #134
Conformance Testing Setup for Shim #134
Conversation
5b3abd1
to
9744ac0
Compare
@rylev for visibility |
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.
Oh nice that's great, is the plan to add this to the CI?
@Mossaka indeed the idea is to add this to CI, but I think we'll wait until we're confident that this can run more quickly and reliably than it currently can. The issue is that this requires pushing to an OCI registry, and we're currently using ttl.sh which is probably not a viable strategy. @kate-goldenring What do you think about merging this as is? I'm fine keeping this as a draft PR until we add this to CI (and I can just keep making PRs to your branch), but it also might be easier to just merge and make PRs directly to this repo instead of your fork. |
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.
Great to see this!
Left a few non-blocking comments.
LGTM
Need a rebase |
Signed-off-by: Kate Goldenring <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Kate Goldenring <[email protected]>
2f973d3
to
9c09de2
Compare
For now, I am skipping the TCP tests as they are failing #147 |
Signed-off-by: Kate Goldenring <[email protected]>
9c09de2
to
cecce6c
Compare
conformance-tests/src/main.rs
Outdated
.next() | ||
.expect("expected second arg to be path to ctr binary"); | ||
|
||
'test: for test in conformance_tests::tests_iter(&tests_dir).unwrap() { |
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.
Is the reason you're not using conformance_tests::run_tests
because the tests run in parallel? Should we provide an API that allows the tests to not run in parallel?
That API is preferred since it gives better test output based on libtest-mimic
.
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.
I wasn't aware of the run_tests
API being added. I can change to use that.
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.
Running them in parallel we run into issues concurrently pushing to the local registry
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.
Okay, got them running in parallel. There was an issue with the registry service not coming up -- restarting docker resolved it. Commit c3a0a58 leverages run_test
and updates these tests to configure the port for the Spin app and generate a random hash for the run ID to enable running the tests in parallel.
c3a0a58
to
61b58a1
Compare
…allel - Set unique port for each run - Set unique ctr run ID for each run Signed-off-by: Kate Goldenring <[email protected]>
61b58a1
to
5e4ac59
Compare
Spin has created a set of conformance test to ensure all Spin runtimes conform to baseline requirements/expectations
Status: runs conformance test to successful termination 🎉
This is very draft: