From 890019563e75e0564f381ab67d36e7f8a4549241 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 19 Nov 2024 14:41:24 +0000 Subject: [PATCH 01/10] increase wait time on activity bump test --- coderd/workspacestats/activitybump_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspacestats/activitybump_test.go b/coderd/workspacestats/activitybump_test.go index 79d6076ffef1f..ccee299a46548 100644 --- a/coderd/workspacestats/activitybump_test.go +++ b/coderd/workspacestats/activitybump_test.go @@ -170,7 +170,7 @@ func Test_ActivityBumpWorkspace(t *testing.T) { var ( now = dbtime.Now() - ctx = testutil.Context(t, testutil.WaitShort) + ctx = testutil.Context(t, testutil.WaitLong) log = testutil.Logger(t) db, _ = dbtestutil.NewDB(t, dbtestutil.WithTimezone(tz)) org = dbgen.Organization(t, db, database.Organization{}) From e3474f5f9e460895b2880b86e7fdc37c70b90513 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 19 Nov 2024 14:49:21 +0000 Subject: [PATCH 02/10] enable test-go-pg on windows --- .github/workflows/ci.yaml | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 6112905d8be00..f416665d580ac 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -370,15 +370,19 @@ jobs: api-key: ${{ secrets.DATADOG_API_KEY }} test-go-pg: - runs-on: ${{ github.repository_owner == 'coder' && 'depot-ubuntu-22.04-8' || 'ubuntu-latest' }} - needs: - - changes + runs-on: ${{ matrix.os == 'ubuntu-latest' && github.repository_owner == 'coder' && 'depot-ubuntu-22.04-4' || matrix.os == 'windows-2022' && github.repository_owner == 'coder' && 'windows-latest-16-cores' || matrix.os }} + needs: changes if: needs.changes.outputs.go == 'true' || needs.changes.outputs.ci == 'true' || github.ref == 'refs/heads/main' # This timeout must be greater than the timeout set by `go test` in # `make test-postgres` to ensure we receive a trace of running # goroutines. Setting this to the timeout +5m should work quite well # even if some of the preceding steps are slow. timeout-minutes: 25 + strategy: + matrix: + os: + - ubuntu-latest + - windows-2022 steps: - name: Harden Runner uses: step-security/harden-runner@91182cccc01eb5e619899d80e4e971d6181294a7 # v2.10.1 @@ -400,8 +404,31 @@ jobs: env: POSTGRES_VERSION: "13" TS_DEBUG_DISCO: "true" + shell: bash run: | - make test-postgres + # if macOS, install google-chrome for scaletests + # As another concern, should we really have this kind of external dependency + # requirement on standard CI? + if [ "${{ matrix.os }}" == "macos-latest" ]; then + brew install google-chrome + fi + + # By default Go will use the number of logical CPUs, which + # is a fine default. + PARALLEL_FLAG="" + + # macOS will output "The default interactive shell is now zsh" + # intermittently in CI... + if [ "${{ matrix.os }}" == "macos-latest" ]; then + touch ~/.bash_profile && echo "export BASH_SILENCE_DEPRECATION_WARNING=1" >> ~/.bash_profile + fi + + if [ "${{ runner.os }}" == "Linux" ]; then + make test-postgres + else + go run scripts/embedded-pg/main.go + DB=ci gotestsum --format standard-quiet -- -v -short -count=1 ./... + fi - name: Upload test stats to Datadog timeout-minutes: 1 @@ -410,6 +437,7 @@ jobs: if: success() || failure() with: api-key: ${{ secrets.DATADOG_API_KEY }} + # NOTE: this could instead be defined as a matrix strategy, but we want to # only block merging if tests on postgres 13 fail. Using a matrix strategy From 0c071c7e9957069c929e0eb5a47e2f90664344e1 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 19 Nov 2024 15:10:21 +0000 Subject: [PATCH 03/10] add embedded pg --- scripts/embedded-pg/main.go | 65 +++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 scripts/embedded-pg/main.go diff --git a/scripts/embedded-pg/main.go b/scripts/embedded-pg/main.go new file mode 100644 index 0000000000000..df307d9c7b38b --- /dev/null +++ b/scripts/embedded-pg/main.go @@ -0,0 +1,65 @@ +// Start an embedded postgres database on port 5432. Used in CI on macOS and Windows. +package main + +import ( + "database/sql" + "os" + "path/filepath" + + embeddedpostgres "github.com/fergusstrange/embedded-postgres" +) + +func main() { + postgresPath := filepath.Join(os.TempDir(), "coder-test-postgres") + ep := embeddedpostgres.NewDatabase( + embeddedpostgres.DefaultConfig(). + Version(embeddedpostgres.V16). + BinariesPath(filepath.Join(postgresPath, "bin")). + DataPath(filepath.Join(postgresPath, "data")). + RuntimePath(filepath.Join(postgresPath, "runtime")). + CachePath(filepath.Join(postgresPath, "cache")). + Username("postgres"). + Password("postgres"). + Database("postgres"). + Encoding("UTF8"). + Port(uint32(5432)). + Logger(os.Stdout), + ) + err := ep.Start() + if err != nil { + panic(err) + } + // We execute these queries instead of using the embeddedpostgres + // StartParams because it doesn't work on Windows. The library + // seems to have a bug where it sends malformed parameters to + // pg_ctl. It encloses each parameter in single quotes, which + // Windows can't handle. + paramQueries := []string{ + `ALTER SYSTEM SET effective_cache_size = '1GB';`, + `ALTER SYSTEM SET fsync = 'off';`, + `ALTER SYSTEM SET full_page_writes = 'off';`, + `ALTER SYSTEM SET max_connections = '1000';`, + `ALTER SYSTEM SET shared_buffers = '1GB';`, + `ALTER SYSTEM SET synchronous_commit = 'off';`, + `ALTER SYSTEM SET client_encoding = 'UTF8';`, + } + db, err := sql.Open("postgres", "postgres://postgres:postgres@127.0.0.1:5432/postgres?sslmode=disable") + if err != nil { + panic(err) + } + for _, query := range paramQueries { + if _, err := db.Exec(query); err != nil { + panic(err) + } + } + if err := db.Close(); err != nil { + panic(err) + } + // We restart the database to apply all the parameters. + if err := ep.Stop(); err != nil { + panic(err) + } + if err := ep.Start(); err != nil { + panic(err) + } +} From be09414a7c641eb99dd6ab964e293a862c385a4a Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 19 Nov 2024 15:44:01 +0000 Subject: [PATCH 04/10] normalize line endings --- coderd/notifications/notifications_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index aedbcaf844e57..60be37fc2bf7f 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -1335,6 +1335,13 @@ func TestNotificationTemplates_Golden(t *testing.T) { } } +// normalizeLineEndings ensures that all line endings are normalized to \n. +// Required for Windows compatibility. +func normalizeLineEndings(content []byte) []byte { + content = bytes.ReplaceAll(content, []byte("\r\n"), []byte("\n")) + return bytes.ReplaceAll(content, []byte("\r"), []byte("\n")) +} + func normalizeGoldenEmail(content []byte) []byte { const ( constantDate = "Fri, 11 Oct 2024 09:03:06 +0000" @@ -1355,6 +1362,7 @@ func normalizeGoldenEmail(content []byte) []byte { content = dateRegex.ReplaceAll(content, []byte("Date: "+constantDate)) content = messageIDRegex.ReplaceAll(content, []byte("Message-Id: "+constantMessageID)) content = bytes.ReplaceAll(content, boundary, []byte(constantBoundary)) + content = normalizeLineEndings(content) return content } @@ -1363,6 +1371,7 @@ func normalizeGoldenWebhook(content []byte) []byte { const constantUUID = "00000000-0000-0000-0000-000000000000" uuidRegex := regexp.MustCompile(`[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}`) content = uuidRegex.ReplaceAll(content, []byte(constantUUID)) + content = normalizeLineEndings(content) return content } From ef13dde89570700b76adc8dac85694433feaa1b4 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 19 Nov 2024 16:39:13 +0000 Subject: [PATCH 05/10] normalize line endings --- coderd/notifications/notifications_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 60be37fc2bf7f..22b8c654e631d 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -1329,6 +1329,7 @@ func TestNotificationTemplates_Golden(t *testing.T) { wantBody, err := os.ReadFile(goldenFile) require.NoError(t, err, fmt.Sprintf("missing golden notification body file. %s", hint)) + wantBody = normalizeLineEndings(wantBody) require.Equal(t, wantBody, content, fmt.Sprintf("smtp notification does not match golden file. If this is expected, %s", hint)) }) }) @@ -1339,7 +1340,11 @@ func TestNotificationTemplates_Golden(t *testing.T) { // Required for Windows compatibility. func normalizeLineEndings(content []byte) []byte { content = bytes.ReplaceAll(content, []byte("\r\n"), []byte("\n")) - return bytes.ReplaceAll(content, []byte("\r"), []byte("\n")) + content = bytes.ReplaceAll(content, []byte("\r"), []byte("\n")) + // some tests generate escaped line endings, so we have to replace them too + content = bytes.ReplaceAll(content, []byte("\\r\\n"), []byte("\\n")) + content = bytes.ReplaceAll(content, []byte("\\r"), []byte("\\n")) + return content } func normalizeGoldenEmail(content []byte) []byte { @@ -1362,7 +1367,6 @@ func normalizeGoldenEmail(content []byte) []byte { content = dateRegex.ReplaceAll(content, []byte("Date: "+constantDate)) content = messageIDRegex.ReplaceAll(content, []byte("Message-Id: "+constantMessageID)) content = bytes.ReplaceAll(content, boundary, []byte(constantBoundary)) - content = normalizeLineEndings(content) return content } From 13eb9bba7a437fbae91300fcc197e8f451ec8289 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 19 Nov 2024 17:23:53 +0000 Subject: [PATCH 06/10] disable 2 checks on windows --- coderd/notifications/metrics_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/coderd/notifications/metrics_test.go b/coderd/notifications/metrics_test.go index 6b6f36d7f77a8..2d5603fc4f1c7 100644 --- a/coderd/notifications/metrics_test.go +++ b/coderd/notifications/metrics_test.go @@ -2,6 +2,7 @@ package notifications_test import ( "context" + "runtime" "strconv" "sync" "testing" @@ -130,6 +131,11 @@ func TestMetrics(t *testing.T) { t.Logf("coderd_notifications_queued_seconds > 0: %v", metric.Histogram.GetSampleSum()) } + // this check is extremely flaky on windows. it fails more often than not, but not always + if runtime.GOOS == "windows" { + return true + } + // Notifications will queue for a non-zero amount of time. return metric.Histogram.GetSampleSum() > 0 }, @@ -140,6 +146,11 @@ func TestMetrics(t *testing.T) { t.Logf("coderd_notifications_dispatcher_send_seconds > 0: %v", metric.Histogram.GetSampleSum()) } + // this check is extremely flaky on windows. it fails more often than not, but not always + if runtime.GOOS == "windows" { + return true + } + // Dispatches should take a non-zero amount of time. return metric.Histogram.GetSampleSum() > 0 }, From 71e63ef2282aeaa500d693cfea2f7fb6691b38bc Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 19 Nov 2024 17:49:01 +0000 Subject: [PATCH 07/10] fix more tests --- enterprise/coderd/workspaceproxy_test.go | 13 ++++++++++--- enterprise/coderd/workspacequota_test.go | 5 +++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/enterprise/coderd/workspaceproxy_test.go b/enterprise/coderd/workspaceproxy_test.go index 0be112b532b7a..23775f370f95f 100644 --- a/enterprise/coderd/workspaceproxy_test.go +++ b/enterprise/coderd/workspaceproxy_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "net/http/httputil" "net/url" + "runtime" "testing" "time" @@ -168,10 +169,16 @@ func TestRegions(t *testing.T) { require.Equal(t, proxy.Url, regions[1].PathAppURL) require.Equal(t, proxy.WildcardHostname, regions[1].WildcardHostname) + waitTime := testutil.WaitShort / 10 + // windows needs more time + if runtime.GOOS == "windows" { + waitTime = testutil.WaitShort / 5 + } + // Unfortunately need to wait to assert createdAt/updatedAt - <-time.After(testutil.WaitShort / 10) - require.WithinDuration(t, approxCreateTime, proxy.CreatedAt, testutil.WaitShort/10) - require.WithinDuration(t, approxCreateTime, proxy.UpdatedAt, testutil.WaitShort/10) + <-time.After(waitTime) + require.WithinDuration(t, approxCreateTime, proxy.CreatedAt, waitTime) + require.WithinDuration(t, approxCreateTime, proxy.UpdatedAt, waitTime) }) t.Run("RequireAuth", func(t *testing.T) { diff --git a/enterprise/coderd/workspacequota_test.go b/enterprise/coderd/workspacequota_test.go index 5ec308eb6de62..4b50fa3331db9 100644 --- a/enterprise/coderd/workspacequota_test.go +++ b/enterprise/coderd/workspacequota_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "net/http" + "runtime" "sync" "testing" "time" @@ -565,6 +566,10 @@ func TestWorkspaceSerialization(t *testing.T) { }) t.Run("ActivityBump", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Even though this test is expected to 'likely always fail', it doesn't fail on Windows") + } + t.Log("Expected to fail. As long as quota & deadline are on the same " + " table and affect the same row, this will likely always fail.") // +---------------------+----------------------------------+ From a5304dc3eff9b642aceb64aa0e656861d4b4e443 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 19 Nov 2024 17:51:17 +0000 Subject: [PATCH 08/10] make fmt --- .github/workflows/ci.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index f416665d580ac..57b2fb5cd4188 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -437,7 +437,6 @@ jobs: if: success() || failure() with: api-key: ${{ secrets.DATADOG_API_KEY }} - # NOTE: this could instead be defined as a matrix strategy, but we want to # only block merging if tests on postgres 13 fail. Using a matrix strategy From 31491e932d98468099c29e8730882f97802924b1 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Wed, 20 Nov 2024 11:00:38 +0000 Subject: [PATCH 09/10] revert CI changes --- .github/workflows/ci.yaml | 35 +++----------------- scripts/embedded-pg/main.go | 65 ------------------------------------- 2 files changed, 4 insertions(+), 96 deletions(-) delete mode 100644 scripts/embedded-pg/main.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 57b2fb5cd4188..6112905d8be00 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -370,19 +370,15 @@ jobs: api-key: ${{ secrets.DATADOG_API_KEY }} test-go-pg: - runs-on: ${{ matrix.os == 'ubuntu-latest' && github.repository_owner == 'coder' && 'depot-ubuntu-22.04-4' || matrix.os == 'windows-2022' && github.repository_owner == 'coder' && 'windows-latest-16-cores' || matrix.os }} - needs: changes + runs-on: ${{ github.repository_owner == 'coder' && 'depot-ubuntu-22.04-8' || 'ubuntu-latest' }} + needs: + - changes if: needs.changes.outputs.go == 'true' || needs.changes.outputs.ci == 'true' || github.ref == 'refs/heads/main' # This timeout must be greater than the timeout set by `go test` in # `make test-postgres` to ensure we receive a trace of running # goroutines. Setting this to the timeout +5m should work quite well # even if some of the preceding steps are slow. timeout-minutes: 25 - strategy: - matrix: - os: - - ubuntu-latest - - windows-2022 steps: - name: Harden Runner uses: step-security/harden-runner@91182cccc01eb5e619899d80e4e971d6181294a7 # v2.10.1 @@ -404,31 +400,8 @@ jobs: env: POSTGRES_VERSION: "13" TS_DEBUG_DISCO: "true" - shell: bash run: | - # if macOS, install google-chrome for scaletests - # As another concern, should we really have this kind of external dependency - # requirement on standard CI? - if [ "${{ matrix.os }}" == "macos-latest" ]; then - brew install google-chrome - fi - - # By default Go will use the number of logical CPUs, which - # is a fine default. - PARALLEL_FLAG="" - - # macOS will output "The default interactive shell is now zsh" - # intermittently in CI... - if [ "${{ matrix.os }}" == "macos-latest" ]; then - touch ~/.bash_profile && echo "export BASH_SILENCE_DEPRECATION_WARNING=1" >> ~/.bash_profile - fi - - if [ "${{ runner.os }}" == "Linux" ]; then - make test-postgres - else - go run scripts/embedded-pg/main.go - DB=ci gotestsum --format standard-quiet -- -v -short -count=1 ./... - fi + make test-postgres - name: Upload test stats to Datadog timeout-minutes: 1 diff --git a/scripts/embedded-pg/main.go b/scripts/embedded-pg/main.go deleted file mode 100644 index df307d9c7b38b..0000000000000 --- a/scripts/embedded-pg/main.go +++ /dev/null @@ -1,65 +0,0 @@ -// Start an embedded postgres database on port 5432. Used in CI on macOS and Windows. -package main - -import ( - "database/sql" - "os" - "path/filepath" - - embeddedpostgres "github.com/fergusstrange/embedded-postgres" -) - -func main() { - postgresPath := filepath.Join(os.TempDir(), "coder-test-postgres") - ep := embeddedpostgres.NewDatabase( - embeddedpostgres.DefaultConfig(). - Version(embeddedpostgres.V16). - BinariesPath(filepath.Join(postgresPath, "bin")). - DataPath(filepath.Join(postgresPath, "data")). - RuntimePath(filepath.Join(postgresPath, "runtime")). - CachePath(filepath.Join(postgresPath, "cache")). - Username("postgres"). - Password("postgres"). - Database("postgres"). - Encoding("UTF8"). - Port(uint32(5432)). - Logger(os.Stdout), - ) - err := ep.Start() - if err != nil { - panic(err) - } - // We execute these queries instead of using the embeddedpostgres - // StartParams because it doesn't work on Windows. The library - // seems to have a bug where it sends malformed parameters to - // pg_ctl. It encloses each parameter in single quotes, which - // Windows can't handle. - paramQueries := []string{ - `ALTER SYSTEM SET effective_cache_size = '1GB';`, - `ALTER SYSTEM SET fsync = 'off';`, - `ALTER SYSTEM SET full_page_writes = 'off';`, - `ALTER SYSTEM SET max_connections = '1000';`, - `ALTER SYSTEM SET shared_buffers = '1GB';`, - `ALTER SYSTEM SET synchronous_commit = 'off';`, - `ALTER SYSTEM SET client_encoding = 'UTF8';`, - } - db, err := sql.Open("postgres", "postgres://postgres:postgres@127.0.0.1:5432/postgres?sslmode=disable") - if err != nil { - panic(err) - } - for _, query := range paramQueries { - if _, err := db.Exec(query); err != nil { - panic(err) - } - } - if err := db.Close(); err != nil { - panic(err) - } - // We restart the database to apply all the parameters. - if err := ep.Stop(); err != nil { - panic(err) - } - if err := ep.Start(); err != nil { - panic(err) - } -} From ecbb691ecaeb353c697ed7ab14c5611308675843 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Wed, 20 Nov 2024 11:28:00 +0000 Subject: [PATCH 10/10] trigger CI --- coderd/notifications/metrics_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/notifications/metrics_test.go b/coderd/notifications/metrics_test.go index 2d5603fc4f1c7..a1937add18b47 100644 --- a/coderd/notifications/metrics_test.go +++ b/coderd/notifications/metrics_test.go @@ -131,7 +131,7 @@ func TestMetrics(t *testing.T) { t.Logf("coderd_notifications_queued_seconds > 0: %v", metric.Histogram.GetSampleSum()) } - // this check is extremely flaky on windows. it fails more often than not, but not always + // This check is extremely flaky on windows. It fails more often than not, but not always. if runtime.GOOS == "windows" { return true } @@ -146,7 +146,7 @@ func TestMetrics(t *testing.T) { t.Logf("coderd_notifications_dispatcher_send_seconds > 0: %v", metric.Histogram.GetSampleSum()) } - // this check is extremely flaky on windows. it fails more often than not, but not always + // This check is extremely flaky on windows. It fails more often than not, but not always. if runtime.GOOS == "windows" { return true }