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

Add merge GitHub Actions workflow to run tests on Windows and macOS runners #9863

Merged
merged 9 commits into from
Sep 21, 2022

Conversation

laurazard
Copy link
Member

What I did

Add GitHub Actions workflow merge to run unit/e2e tests on Windows and Mac runners, after PRs are merged to v2 and when tags such as v* are pushed.

Removed /rebase action since this is no longer necessary.

(not mandatory) A picture of a cute animal, if possible in relation with what you did

IMG_4645

Fixes error when attempting to run `uname` on Windows, and add `.exe` to built binary on `make` if on Windows

Signed-off-by: Laura Brehm <[email protected]>
@laurazard laurazard requested a review from a team September 20, 2022 15:38
@@ -1,3 +1,6 @@
//go:build !windows
// +build !windows
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does not compile on Windows due to:

cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}

and surrounding logic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, we've dealt with this same thing before in Tilt 🙃

What we can do is make a func SetProcessPgid(cmd *exec.Cmd) in proc_windows.go that's a no-op and proc_unix.go that does the code ⬆️ and then call the helper. Annoying but 🤷

(IIRC Windows always has the desired behavior re: killing process groups)

@@ -29,6 +30,9 @@ import (
)

func TestPause(t *testing.T) {
if _, ok := os.LookupEnv("CI"); ok {
t.Skip("Skipping test on CI... flaky")
Copy link
Member Author

@laurazard laurazard Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was really flaky on CI, so I think it's best to just skip it there for now until we can harden it. Fwiw, it seems to be a performance issue, as it always passes on the M1 mac, but is flaky on the Intel mac and on the Windows runners.

@@ -135,6 +135,9 @@ func TestDownComposefileInParentFolder(t *testing.T) {
}

func TestAttachRestart(t *testing.T) {
if _, ok := os.LookupEnv("CI"); ok {
t.Skip("Skipping test on CI... flaky")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this test, and same regarding always passing on the M1 mac but being flaky on the Intel mac/Windows runner.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof yeah I've been 👀 this test for a while, might take a look tomorrow to see if I can figure out where the race is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(To be clear though, I'm in favor of merging this with the Skip() and deal with cleanup after!)

@@ -18,13 +18,20 @@ VERSION ?= $(shell git describe --match 'v[0-9]*' --dirty='.m' --always --tags)
GO_LDFLAGS ?= -s -w -X ${PKG}/internal.Version=${VERSION}
GO_BUILDTAGS ?= e2e,kube

UNAME_S := $(shell uname -s)
ifeq ($(UNAME_S),Linux)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running $(shell uname -s) on Windows breaks, so we only do it if we're not on Windows

MOBY_DOCKER=/Applications/Docker.app/Contents/Resources/bin/docker
endif
ifeq ($(DETECTED_OS),Windows)
BINARY_EXT=.exe
endif
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make was previously not adding .exe to the docker-compose binary built on Windows 😅 (thanks for looking at that @milas)

func TestBuildSSH(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("Running on Windows. Skipping...")
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split out the SSH tests out so I could disable them on Windows -- they were failing with:

Invalid empty ssh agent socket: Windows OpenSSH agent not available at \\.\pipe\openssh-ssh-agent. Enable the SSH agent service or set SSH_AUTH_SOCK.

We can take a look at that later, maybe set up an SSH agent on the Windows runner and re-enable these

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows does have a built-in OpenSSH Agent nowadays, so it's fairly straightforward to enable in services.msc if we want

Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wahoo 🎉

.github/workflows/merge.yml Outdated Show resolved Hide resolved
.github/workflows/merge.yml Outdated Show resolved Hide resolved
Comment on lines 181 to +184
func TestBuildSecrets(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("skipping test on windows")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly surprised this test is failing on Windows! Should be a fun one to investigate 🙈

Signed-off-by: Laura Brehm <[email protected]>
Makefile Outdated Show resolved Hide resolved
@laurazard laurazard merged commit 94465d5 into v2 Sep 21, 2022
@laurazard laurazard deleted the gha-win-mac-runners branch September 21, 2022 14:39
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