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

Fix Windows tests, verify in CI #394

Merged
merged 4 commits into from
Dec 10, 2023
Merged

Conversation

abhinav
Copy link
Contributor

@abhinav abhinav commented Dec 10, 2023

This adds testing against Windows to CI.
Hermit doesn't support Windows,
so the setup for that is done with GitHub Actions.

The following additional fixes were needed:

  • filecontent mapper returned ioutil.ReadAll error as-is.
    This is less informative on Windows when the source is a directory:
    "Invalid function."
  • mapper_windows_test made use of a non-existent assert.NotNil function.
  • mapper_test looked for Unix-specific "no such file or directory" messages.
    To fix this, we now assert that errors match os.ErrNotExist.
    This handles system-specific variations in the error message.

Adds a Windows test run to CI.
Go setup relies on GHA for this
because Hermit doesn't yet support Windows.
If we couldn't read because the source is a directory,
override the original error message.
@@ -12,16 +12,30 @@ jobs:
- name: Checkout code
uses: actions/checkout@v2
- name: Init Hermit
run: ./bin/hermit env -r >> $GITHUB_ENV
run: ./bin/hermit env -r >> "$GITHUB_ENV"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quoting here was suggested by staticcheck.
I can remove it if you'd prefer not to have that in this change.

@@ -1,8 +1,8 @@
module github.com/alecthomas/kong

require (
github.com/alecthomas/assert/v2 v2.1.0
github.com/alecthomas/repr v0.1.0
github.com/alecthomas/assert/v2 v2.4.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The upgrade was required for the assert.IsError function.

@@ -514,15 +515,16 @@ func TestExistingFileMapperDefaultMissing(t *testing.T) {
}
var cli CLI
p := mustNew(t, &cli)
file := "testdata/file.txt"
file := filepath.Join("testdata", "file.txt")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OS-specific filepath separator for string matching later.

@@ -632,8 +638,7 @@ func TestMapperPlaceHolder(t *testing.T) {
assert.Contains(t, b.String(), "--flag=/a/b/c")
}

type testMapperWithPlaceHolder struct {
}
type testMapperWithPlaceHolder struct{}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops. this snuck in from gofumpt.
I can revert this line if you care about the difference.

Copy link
Owner

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@@ -33,11 +33,12 @@ func TestWindowsFileMapper(t *testing.T) {
p := mustNew(t, &cli)
_, err := p.Parse([]string{"testdata\\file.txt"})
assert.NoError(t, err)
assert.NotNil(t, cli.File)
assert.True(t, cli.File != nil, "File should not be nil")
Copy link
Owner

Choose a reason for hiding this comment

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

This is fine, but you should be able to use assert.NotZero() here if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Doh. I completely blanked on that. "Hm, there's no NotNil. Guess that's not a covered use case."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here #396

@alecthomas
Copy link
Owner

Annoying that it takes 1m just to run a test that takes 1s, but that's Windows life for you I guess.

@alecthomas alecthomas merged commit 3263463 into alecthomas:master Dec 10, 2023
3 checks passed
@abhinav
Copy link
Contributor Author

abhinav commented Dec 10, 2023

Annoying that it takes 1m just to run a test that takes 1s, but that's Windows life for you I guess.

Indeed. Windows runners take a bit longer on GitHub. Although following this first run, it should have cached the dependencies so it'll take slightly less. AFAIK, the only way to make that go faster is pay GH money, which makes sense but also not worth it for an open source project.

abhinav added a commit to abhinav/kong that referenced this pull request Dec 10, 2023
@abhinav abhinav deleted the windows branch December 10, 2023 04:46
alecthomas pushed a commit that referenced this pull request Dec 10, 2023
Follow up to #394, specifically [this comment][1].

  [1]: #394 (comment)
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.

2 participants