NotificationsYou must be signed in to change notification settings
Fork906
Star10k

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

Jump to bottom

Merged

hugodutka merged 6 commits intomainfromhugodutka/test-go-pg-windows-macos-cache

May 22, 2025

Merged

chore: run test-go-pg on macOS and Windows in regular CI#17853

hugodutka merged 6 commits intomainfromhugodutka/test-go-pg-windows-macos-cache

May 22, 2025

Conversation

Copy link

Contributor

hugodutka commentedMay 15, 2025•
edited
Loading

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:

it was flaky
it was slow (took 17 minutes)

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,main will still run tests without it.

This PR also:

removes the nightly gauntlet since all tests now run in regular CI
removes thetest-cli job for the same reason
removes thesetup-imdisk action which is now fully replaced bycoder/setup-ramdisk-action
makes 2 minor changes which could be separate PRs, but I rolled them into this because they were helpful when iterating on it:
- replace theif: 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. Seethe GitHub docs for more details.
- disable the recently addedTestReinitializeAgent since it does not pass on Windows with Postgres. There's an open issue to fix it:flake:TestReinitializeAgent internal#642

This PR will:

unblockRemove the in-memory database #15109
alleviateflake: ImDisk on Windows CLI internal#647

I tested caching by temporarily enabling cache upload on this PR: here'sa run showing cache being used.

github-actionsbot assignedhugodutka

May 15, 2025

hugodutka force-pushed thehugodutka/test-go-pg-windows-macos-cache branch 23 times, most recently fromc3f6634 to291a7dfCompare

May 19, 2025 11:54

hugodutka changed the title~~chore: run test-go-pg on macOS and Windows with test cache~~chore: run test-go-pg on macOS and Windows in regular CI

May 19, 2025

hugodutka force-pushed thehugodutka/test-go-pg-windows-macos-cache branch 5 times, most recently from9c27aa8 toe321870Compare

May 19, 2025 14:07

hugodutka force-pushed thehugodutka/test-go-pg-windows-macos-cache branch 5 times, most recently from3bcc4f4 toc8e68dbCompare

May 19, 2025 17:23

hugodutka marked this pull request as ready for review

May 19, 2025 17:44

hugodutka requested a review fromdeansheather

May 19, 2025 17:44

deansheather reviewed

May 20, 2025

View reviewed changes

.github/actions/setup-go-paths/action.yml

		id:paths
		uses:actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea# v7
		with:
		script:\|

Copy link

Member

deansheatherMay 20, 2025

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

Copy link

ContributorAuthor

hugodutkaMay 22, 2025

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.

.github/workflows/ci.yaml


		- name: Checkout
		uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
		with:
		fetch-depth: 1

		- name: Setup Go Paths
		uses: ./.github/actions/setup-go-paths

Copy link

Member

deansheatherMay 20, 2025

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?

Copy link

ContributorAuthor

hugodutkaMay 22, 2025

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.

.github/workflows/ci.yaml

		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 }}

Copy link

Member

deansheatherMay 20, 2025

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

.github/workflows/ci.yaml OutdatedShow resolvedHide resolved

enterprise/coderd/workspaceagents_test.go Outdated

		@@ -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")

Copy link

Member

deansheatherMay 20, 2025

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

Copy link

ContributorAuthor

hugodutkaMay 22, 2025•
edited
Loading

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

.github/workflows/ci.yaml

Comment on lines +554 to +556

		# 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))"

Copy link

Member

deansheatherMay 20, 2025

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.