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

integration: skip for dockerd #3101

Merged
merged 4 commits into from
Sep 12, 2022
Merged

Conversation

crazy-max
Copy link
Member

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.

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")
Copy link
Member

@thaJeztah thaJeztah Sep 10, 2022

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, the TestRequires 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.

Copy link
Member

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.

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.

3 participants