-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
integration: skip for dockerd #3101
Conversation
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@@ -1621,6 +1621,7 @@ func testLocalSourceWithDiffer(t *testing.T, sb integration.Sandbox, d llb.DiffT | |||
} | |||
|
|||
func testOCILayoutSource(t *testing.T, sb integration.Sandbox) { | |||
integration.SkipIfDockerd(t, sb, "oci exporter") |
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.
So... really thinking out loud here.
The current situation is that;
- If new tests are added, the author of those tests needs to add these skips
- But, we frequently find ones that were missed
- Which means that once we update BuildKit in docker (and CI there), we find out "oops, some tests fail, but that's expected")
- ... then need to add the missing skips in BuildKit
- And try updating vendor (and CI) again
With the work on containerd integration
- some of those skips may no longer be accurate (e.g., multi-arch store)
- The "reason" string is just that.. a string, so we can't (currently) use that one to ignore the skip if those conditions are met.
A. Env-var / option to provide a list of tests to skip
I'm wondering (and I know they're somewhat ugly); would it make sense to have a way to set the list of tests to skip when running the tests, similar to how we did for the docker-py
tests; https://github.com/moby/moby/blob/924edb948c2731df3b77697a8fcc85da3f6eef57/hack/make/test-docker-py#L17-L19
# TODO re-enable test_attach_no_stream after https://github.com/docker/docker-py/issues/2513 is resolved
# TODO re-enable test_create_with_device_cgroup_rules after https://github.com/docker/docker-py/issues/2939 is resolved
: "${PY_TEST_OPTIONS:=--junitxml=${DEST}/junit-report.xml --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_attach_no_stream --deselect=tests/integration/api_container_test.py::CreateContainerTest::test_create_with_device_cgroup_rules}"
- ✅ PRO: allows moby-CI to disable or enable tests once they can be run, without having to do a round of "update buildkit -> revendor -> etc."
- ❌ CON: risk of "forgetting to re-enable tests" (and now we could be skipping tests that actually should be running)
B. Alternative
Make the SkipIfDockerd
a TestRequires
; if we know specific functionality is required, we could more granularly enable tests, once that functionality arrives, e.g.
integration.TestRequires(t, sb, "oci-exporter", "test requires an OCI exporter")
integration.TestRequires(t, sb, "multi-arch", "test requires a multi-arch capable image store")
If we're not able to detect that functionality, it could be handled through an env-var, e.g.
BUILDER_FEATURES=oci-exporter,multi-arch
- ✅ PRO: enable or disable tests more granularly, without having to provide a (potentially long) list of tests to include/exclude.
- ❌ CON: Like
SkipIfDockerd
, theTestRequires
may be missed / forgotten by the test author, so we'd still get into a round of "update buildkit -> revendor -> etc."
C. Options A and B combined
Make sure tests have the right TestRequires
set, but if we missed one of them, we can temporarily use option "A" to skip those tests until it's fixed in BuildKit.
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 need to figure out something for containerd branch as well as none of these tests should be skipped then.
So I think we at least need configuration option to disable these skips completely. If it is easy, it seems fine if we reorganize the skipping so that it takes a test name and therefore can be done externally. Although that raises the question of why is the dockerd worker defined in this repo at all.
Carry changes from #3003 related to missing skip tests for dockerd worker. Also removes concurrency check in dockerd workflow otherwise we can only trigger one run at a time.