- Notifications
You must be signed in to change notification settings - Fork906
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
c3f6634 to291a7dfCompare9c27aa8 toe321870Compare3bcc4f4 toc8e68dbCompare| 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 toGITHUB_ENV andGITHUB_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
Uh oh!
There was an error while loading.Please reload this page.
| @@ -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
hugodutkaMay 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 to64646edCompare64646ed to2929c79Comparea0e229a intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 alsobrought 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,
mainwill still run tests without it.This PR also:
test-clijob for the same reasonsetup-imdiskaction which is now fully replaced bycoder/setup-ramdisk-actionif: always()condition on thegenjob with aif: ${{ !cancelled() }}to allow the job to be cancelled. Previously the job would run to completion even if the entire workflow was cancelled. Seethe GitHub docs for more details.TestReinitializeAgentsince it does not pass on Windows with Postgres. There's an open issue to fix it:flake:TestReinitializeAgentinternal#642This PR will:
I tested caching by temporarily enabling cache upload on this PR: here'sa run showing cache being used.