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

ci: github actions test workflow #3365

Merged
merged 1 commit into from
Dec 7, 2021
Merged

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Nov 17, 2021

Follow-up #3320

Switch To GitHub Actions for tests:

  • add test stage and update bake definition
  • create test workflow
    • run tests with bake
    • run tests on macos and windows host runners
  • appveyor and make.ps1 not needed anymore (cc @StefanScherer)

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@crazy-max crazy-max force-pushed the gha-test branch 4 times, most recently from 2fedb62 to c051a91 Compare November 17, 2021 19:24
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2021

Codecov Report

Merging #3365 (4d93717) into master (ba0ace0) will decrease coverage by 1.66%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3365      +/-   ##
==========================================
- Coverage   58.02%   56.35%   -1.67%     
==========================================
  Files         302      304       +2     
  Lines       21761    26833    +5072     
==========================================
+ Hits        12626    15121    +2495     
- Misses       8214    10792    +2578     
+ Partials      921      920       -1     

@crazy-max crazy-max marked this pull request as ready for review November 17, 2021 20:00
@crazy-max crazy-max marked this pull request as draft November 17, 2021 20:02
@crazy-max crazy-max marked this pull request as ready for review November 18, 2021 05:02
@crazy-max crazy-max force-pushed the gha-test branch 2 times, most recently from aaa7b18 to 1802073 Compare November 22, 2021 10:09
@crazy-max

This comment has been minimized.

@crazy-max crazy-max marked this pull request as draft November 22, 2021 13:17
@thaJeztah
Copy link
Member

I'm a bit confused though; why do they differ?

context_test.go:84: assertion failed:
/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/builder-context-test348532470 (contextDir string) !=
/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/builder-context-test348532470 (absContextDir string)

Is it because we create it as /var/folders, but /var is a symlink, so when reading it back, it returns the actual path?

$ ls -la /var
lrwxr-xr-x@ 1 root  wheel  11 Jul  1  2019 /var -> private/var

@thaJeztah
Copy link
Member

So, I guess os.TempDir() returns /tmp, but that's a symlink;

bash-5.0$ ls -la /tmp
lrwxr-xr-x@ 1 root  wheel  11 Jul  1  2019 /tmp -> private/tmp

(also wondering if os.MkdirTemp() returns the wrong path, I'm wondering if that should be considered a bug in Golang, but not sure what would be the least surprising behavior 🤔)

I guess as part of the test, we'd have to resolve the path to the actual location, but it's still odd that one part gives the path of the symbolic linked location, and the other the actual location.

@crazy-max crazy-max force-pushed the gha-test branch 6 times, most recently from 30b3778 to ca6b595 Compare November 22, 2021 21:03
Comment on lines +248 to +249
path, err = filepath.EvalSymlinks(path)
assert.NilError(t, err)
Copy link
Member Author

@crazy-max crazy-max Nov 23, 2021

Choose a reason for hiding this comment

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

Thanks for your review @thaJeztah, I fixed this issue by evaluating symlinks and now tests are ok on MacOS runners.

Comment on lines +48 to +52
name: Prepare git
if: matrix.os == 'windows-latest'
run: |
git config --system core.autocrlf false
git config --system core.eol lf
Copy link
Member Author

@crazy-max crazy-max Nov 23, 2021

Choose a reason for hiding this comment

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

To fix the EOL issues on Windows runners, we need to disable autocrlf. See actions/checkout#135 for more info.

@crazy-max
Copy link
Member Author

There are still tests failing on the Windows runner and from what I see, it was already failing before this PR, so I guess we should fix that in a follow-up and mark the Windows GHA job as "allow-failure" or disable it in the meantime. WDYT @thaJeztah?

@crazy-max crazy-max force-pushed the gha-test branch 2 times, most recently from 8b7c464 to 18a7b4c Compare November 24, 2021 18:30
@crazy-max
Copy link
Member Author

crazy-max commented Nov 24, 2021

I have disabled tests on Windows runner in the meantime. FWIW the last time the tests were ok on AppVeyor was June 24, 2018.

image

@crazy-max crazy-max marked this pull request as ready for review November 24, 2021 18:35
@crazy-max crazy-max force-pushed the gha-test branch 3 times, most recently from d1c4f7f to 1bf2680 Compare November 25, 2021 12:37
Makefile Outdated
Comment on lines 25 to 26
mkdir -p ./coverage
gotestsum -- $(shell go list ./... | grep -vE '/vendor/|/e2e/') -coverprofile=coverage.txt
Copy link
Member

Choose a reason for hiding this comment

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

I see this was changed, but looks like the options are left the same; do we actually use the ./coverage directory now when using this target? Or only if we use the buildx bake approach?

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed moved coverage output to ./build folder.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@thaJeztah
Copy link
Member

rebase was just s/1.16.10/1.16.11/ I'll merge this once CI completes (thanks!)

@thaJeztah thaJeztah merged commit 4db5a4f into docker:master Dec 7, 2021
@crazy-max crazy-max deleted the gha-test branch December 7, 2021 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants