-
Notifications
You must be signed in to change notification settings - Fork 889
chore: run test-go-pg on macOS and Windows in regular CI #17853
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
Conversation
c3f6634
to
291a7df
Compare
9c27aa8
to
e321870
Compare
3bcc4f4
to
c8e68db
Compare
id: paths | ||
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7 | ||
with: | ||
script: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind using the script here, but you can accomplish the same with bash by echoing to GITHUB_ENV
and GITHUB_OUTPUT
: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to use the script to handle setting multi-line env variables. With bash, you have to be very careful with whitespace and indentation. It's error-prone.
|
||
- name: Checkout | ||
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
with: | ||
fetch-depth: 1 | ||
|
||
- name: Setup Go Paths | ||
uses: ./.github/actions/setup-go-paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we were only doing this on Windows, should we do the same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step is now used to standardize the Go cache location on disk so the "download cache step" knows where to put it. Without it, it's somewhat complicated to figure out the paths so it works cross-OS and if Go isn't installed.
runs-on: ${{ matrix.os == 'ubuntu-latest' && github.repository_owner == 'coder' && 'depot-ubuntu-22.04-8' || matrix.os }} | ||
# make sure to adjust NUM_PARALLEL_PACKAGES and NUM_PARALLEL_TESTS below | ||
# when changing runner sizes | ||
runs-on: ${{ matrix.os == 'ubuntu-latest' && github.repository_owner == 'coder' && 'depot-ubuntu-22.04-8' || matrix.os && matrix.os == 'macos-latest' && github.repository_owner == 'coder' && 'depot-macos-latest' || matrix.os == 'windows-2022' && github.repository_owner == 'coder' && 'depot-windows-2022-16' || matrix.os }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is an abomination lmfao
@@ -83,6 +84,10 @@ func TestBlockNonBrowser(t *testing.T) { | |||
func TestReinitializeAgent(t *testing.T) { | |||
t.Parallel() | |||
|
|||
if runtime.GOOS == "windows" { | |||
t.Skip("this test doesn't pass on Windows with Postgres") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please open an internal ticket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the skip has already been merged separately: #17968, so I'm removing it from this PR
# terraform gets installed in a random directory, so we need to normalize | ||
# the path to the terraform binary or a bunch of cached tests will be | ||
# invalidated. See scripts/normalize_path.sh for more details. | ||
normalize_path_with_symlinks "$RUNNER_TEMP/sym" "$(dirname $(which terraform))" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it would be better if we changed the test to use system terraform if it's a suitable version, then we can just install it before starting the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean we should use the terraform download function that's built into Coder?
c8e68db
to
64646ed
Compare
64646ed
to
2929c79
Compare
This PR starts running test-go-pg on macOS and Windows in regular CI. Previously this suite was only run in the nightly gauntlet for 2 reasons:
We've since stabilized the flakiness by switching to depot runners, using ram disks, optimizing the number of tests run in parallel, and automatically re-running failing tests. We've also brought down the time to run the suite to 9 minutes. Additionally, this PR allows test-go-pg to use cache from previous runs, which speeds it up further. The cache is only used on PRs,
main
will still run tests without it.This PR also:
test-cli
job for the same reasonsetup-imdisk
action which is now fully replaced by coder/setup-ramdisk-actionif: always()
condition on thegen
job with aif: ${{ !cancelled() }}
to allow the job to be cancelled. Previously the job would run to completion even if the entire workflow was cancelled. See the GitHub docs for more details.TestReinitializeAgent
since it does not pass on Windows with Postgres. There's an open issue to fix it: flake:TestReinitializeAgent
internal#642This PR will:
I tested caching by temporarily enabling cache upload on this PR: here's a run showing cache being used.