Add basic workflow for automatic subcommand testing on Windows. by aznashwan · Pull Request #1393 · containerd/nerdctl

aznashwan · 2022-09-26T21:58:16Z

This PR adapts some existing command tests to run on both Linux and Windows, as well as add a simple GitHub action to run the command tests on Windows 2019/2022 automatically.

aznashwan · 2022-09-26T22:11:13Z

It might be worth discussing whether nerdctl push should also be included in this workflow (and if so, to which public/private registry implementation), but considering the efforts to port BuildKit on Windows are still ongoing it is unlikely to be critical for users at this stage...

The rest of the failing commands currently being tested have clear causes for the most part (image encrypt/decrypt have keyfile encoding issues, pause/unpause/cp are not yet implemented, and rename errors out), and will be addressed in dedicated PRs.

manugupt1 · 2022-09-27T01:13:29Z

+  CONTAINERD_REF: 'main'
+  TEST_CONTAINER_IMAGE: 'k8s.gcr.io/e2e-test-images/busybox:1.29-2'
+
+jobs:


Would it be better to have a test arg like docker.

go test -exec sudo -v ./cmd/nerdctl/... -args -test.target=docker

and if a test is windows compatible it will run automatically

AkihiroSuda · 2022-09-27T02:14:06Z

Why not use the same test suite as Linux?

nerdctl/.cirrus.yml

Line 41 in 70bc5a9

- go test -v ./cmd/...

AkihiroSuda · 2022-10-11T14:49:48Z

@@ -0,0 +1,101 @@
+# This workflow runs the command test suite on GitHub-based Windows Server runners.


Could you explain pros/cons compared to the current Cirrus CI ?

The main pro would be the fact the containerd repo has a workflow using the same GitHub runner images to run the integration tests, so we could crossref any errors we're seeing to any bugs which may have gotten into containerd without having to worry about differences in the runner's OS/patch number as much.

I don't think there are any cons to having both running in parallel, so I could also update the Cirrus config to run the tests on 2022 as well if needed. (currently looks like Cirrus only has images based on 2019 OOB so we'd have to setup a custom 2022 image for it though)

aznashwan · 2022-10-11T14:50:18Z

@AkihiroSuda @manugupt1 thanks a lot for the pointers!
I was initially apprehensive on adapting the existing tests since they seem to have been mostly written with Linux in mind but it makes more sense to unify the *nix/Windows testing as much and as soon as possible!

I've updated this PR in the following ways:

made the workflow setup containerd and just run go test ./cmd/... on the GitHub Windows runners (we'd like to have it tested on both 2019 and 2022 on semi-up-to-date Windows images and containerd release so IMO having a per-PR GitHub workflow is more useful than Cirrus here)
enabled the image encryption/conversion tests on Windows (the commands just worked OOB)
adapted some tests which used to require image building during the test to work around this on Windows until BuildKit is ported too (namely re-tagging or converting the testutil.CommonImage)

There's like a dozen existing tests which are skipped on Windows since they unequivocally require BuildKit as part of the test itself (mostly all of those prefixed with TestBuild*), so I have left those as-is for now.

There's a couple of tests which outright fail and I'll be looking into next:

TestRename yields an internal OS error
TestRenameUpdate fails to start the test container (almost surely related to TestRename)
TestEncryptJWE (tests nerdctl image encrypt --jwe=...) needs a registry running during the test but the current test registry image is Linux-only

Lastly, some points I'd like to bring to your attention now (with separate PRs to work through these later):

some tests which were cross-platform and executed on Unix too have been "deactivated" by giving them a Windows and Linux versions only (TestImagePrune and TestImagesFilter), but I've seen that the test suite isn't even run on FreeBSD in the Cirrus config so no actual "regression" happened.
nerdctl logs will always exec the tail binary when using simple JSON file logging, which is impractical to expect to be present on Windows installs. Afaik docker logs always just prints/streams to stdout so I was wondering whether it's really worth having nerdctl act differently in this case?
how should I go about updating the Windows compatibility for the commands in the README? It looks like the list was generated with DocToc but it looks like any updates are manually committed by contributors, correct?

manugupt1 · 2022-10-14T00:19:44Z

+
+    defaults:
+      run:
+        shell: bash


Should this be bash or PowerShell?

Good catch, the initial version of the workflow contained way more steps and mostly used bash in them but none do any more so I'll be updating the workflow to have PS as the default.

Updated the PR to use PowerShell as the default shell FWIW.

manugupt1 · 2022-10-14T00:24:12Z

 	assert.Equal(t, string(data), testContent)
 }

-func createBuildContext(dockerfile string) (string, error) {


Why remove this?

I had moved it to image_testing_utils.go since it is also called in a couple of non-build-related tests.

manugupt1 · 2022-10-14T00:30:18Z

+	"github.com/containerd/nerdctl/pkg/testutil"
+)
+
+func TestRename(t *testing.T) {


Just curious: Why not have common tests in rename_test.go and platform specific tests in rename__test.go using build tags?

As far as I could tell all the existing tests were laid out using the filename suffixes for build constraints so I strived to keep it that way.

I also needed to paraemtrize the test image since testutils.CommonImage is only defined on Linux/Windows/FreeBSD and that would prevent the tests from compiling on any other future platform we'd like to support.

We could alternatively set a default testutils.CommonImage for all platforms and thus have the TestRename* tests be 100% shared across all platforms.
I'd personally rather we move the CommonImage constants from testutils to here since it's only ever referenced in the ./cmd/nerdctl/... tests anyway...

Signed-off-by: Nashwan Azhari <[email protected]>

aznashwan · 2023-08-06T15:37:38Z

Superseded by #2418.

manugupt1 reviewed Sep 27, 2022

View reviewed changes

AkihiroSuda added area/ci e.g., CI failure platform/Windows/Non-WSL2 Microsoft Windows (non-WSL2) labels Sep 27, 2022

aznashwan force-pushed the windows-acceptance-basic branch from d35f7c2 to 3ce3ad7 Compare October 11, 2022 14:47

AkihiroSuda reviewed Oct 11, 2022

View reviewed changes

aznashwan force-pushed the windows-acceptance-basic branch from 3ce3ad7 to e97def0 Compare October 11, 2022 14:55

manugupt1 reviewed Oct 14, 2022

View reviewed changes

aznashwan added 3 commits October 17, 2022 14:34

Add basic workflow for automatic subcommand testing on Windows.

beb5a25

Signed-off-by: Nashwan Azhari <[email protected]>

Adapt testutils/testregistry for multiple platforms.

c87903e

Signed-off-by: Nashwan Azhari <[email protected]>

Update command tests for Windows.

a2c4cfa

Signed-off-by: Nashwan Azhari <[email protected]>

aznashwan force-pushed the windows-acceptance-basic branch from e97def0 to a2c4cfa Compare October 17, 2022 11:59

aznashwan mentioned this pull request Oct 17, 2022

Use go env to determine GOPATH in Makefile. containerd/containerd#7542

Merged

aznashwan closed this Aug 6, 2023

		@@ -0,0 +1,101 @@
		# This workflow runs the command test suite on GitHub-based Windows Server runners.

Conversation

aznashwan commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aznashwan commented Sep 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aznashwan commented Oct 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aznashwan commented Aug 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aznashwan commented Sep 26, 2022 •

edited

Loading

AkihiroSuda commented Sep 27, 2022 •

edited

Loading