Skip to content

integration: skip for dockerd#3101

Merged
tonistiigi merged 4 commits into
moby:masterfrom
crazy-max:skip-dockerd-tests
Sep 12, 2022
Merged

integration: skip for dockerd#3101
tonistiigi merged 4 commits into
moby:masterfrom
crazy-max:skip-dockerd-tests

Conversation

@crazy-max

Copy link
Copy Markdown
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.

Comment thread client/client_test.go
}

func testOCILayoutSource(t *testing.T, sb integration.Sandbox) {
integration.SkipIfDockerd(t, sb, "oci exporter")

@thaJeztah thaJeztah Sep 10, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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