From 0961965b7317635fe08e99ce21610d997d2302b4 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 11 Oct 2021 14:09:19 +0200 Subject: [PATCH 1/8] testhelper: Add function to quarantine a test We are having issues with some tests being flaky and blocking deploys because the pipeline on master is failing. This annoying, but often it's not easy to quickly fix the flaky test. This change allows us to put flaky tests in quarantine. The quarantined tests are run in a separate job, but that job is marked to allow to fail. This allows us to make the pipeline green again quickly, but still have the tests running. --- internal/testhelper/testhelper.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 7a52ce3ee6c..6029df86b0e 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -314,3 +314,12 @@ func GenerateCerts(t *testing.T) (string, string) { return certFile.Name(), keyFile.Name() } + +// Quarantine skips a test unless environment variable QUARANTINE is set +func Quarantine(t testing.TB, issueURL string) { + _, ok := os.LookupEnv("QUARANTINE") + if ok { + return + } + t.Skipf("Skipping quarantined test, issue %s", issueURL) +} -- GitLab From 9b705dc538aef15e3e9dafc0c1ddd9d0be4e64ce Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 11 Oct 2021 14:25:05 +0200 Subject: [PATCH 2/8] ci: Remove junit report from artifacts The junit report is mentioned as normal artifact *and* report. This is not needed, so remove it from the regular artifacts. --- .gitlab-ci.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 228fb4e13e0..d5c5ad743c6 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -186,8 +186,6 @@ test: - find . -type d \( -path ./_build -o -path ./ruby \) -prune -o -type d -exec chmod a-w {} \; - make ${TARGET} artifacts: - paths: - - _build/reports/go-tests-report-go-${GO_VERSION}-git-${GIT_VERSION}.xml reports: junit: _build/reports/go-tests-report-go-${GO_VERSION}-git-${GIT_VERSION}.xml parallel: -- GitLab From e2055acfbe6565fee93c57c3853bf314ad74d76c Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 11 Oct 2021 14:33:46 +0200 Subject: [PATCH 3/8] ci: Default image uses default versions The CI yaml file specifies default versions for tools like Ruby and Golang. But the default image still uses older versions of these tools. This change uses the default versions of these tools in the default image. This removes the need the need to specify the image in many jobs. --- .gitlab-ci.yml | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d5c5ad743c6..b8a8adfb181 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -5,7 +5,7 @@ stages: - qa default: - image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.7-golang-1.16-git-2.31 + image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-${RUBY_VERSION}-golang-${GO_VERSION}-git-2.31 tags: - gitlab-org @@ -84,7 +84,6 @@ danger-review: expire_in: 1 week .postgres_template: &postgres_definition - image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-${RUBY_VERSION}-golang-${GO_VERSION}-git-2.31 services: - postgres:${POSTGRES_VERSION} variables: &postgres_variables @@ -133,7 +132,6 @@ proto: build: <<: *cache_definition stage: build - image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-${RUBY_VERSION}-golang-${GO_VERSION}-git-2.31 script: - go version - make all git @@ -156,7 +154,6 @@ build: binaries: <<: *cache_definition stage: build - image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-${RUBY_VERSION}-golang-${GO_VERSION}-git-2.31 only: - tags script: @@ -177,7 +174,6 @@ binaries: test: <<: *test_definition <<: *postgres_definition - image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-${RUBY_VERSION}-golang-${GO_VERSION}-git-2.31 script: - _build/deps/git/install/bin/git version # This command will make all directories except of our build directory and Ruby code unwritable. @@ -208,7 +204,6 @@ test: nightly:git: <<: *test_definition <<: *postgres_definition - image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-${RUBY_VERSION}-golang-${GO_VERSION}-git-2.31 script: - go version - make all ${TARGET} -- GitLab From a97f7e24b67c60b94081634a9903634589ff5566 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 11 Oct 2021 14:43:11 +0200 Subject: [PATCH 4/8] ci: Move junit report artifact to test_template Specify the location of the junit report in the test_template anchor so every job using the anchor has it. --- .gitlab-ci.yml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b8a8adfb181..9677c25e443 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -79,7 +79,9 @@ danger-review: policy: pull artifacts: paths: - - ruby/tmp/gitaly-rspec-test.log + - ruby/tmp/gitaly-rspec-test.log + reports: + junit: _build/reports/go-tests-report-go-${GO_VERSION}-git-${GIT_VERSION}.xml when: on_failure expire_in: 1 week @@ -181,9 +183,6 @@ test: # they should all instead use a temporary directory for runtime data. - find . -type d \( -path ./_build -o -path ./ruby \) -prune -o -type d -exec chmod a-w {} \; - make ${TARGET} - artifacts: - reports: - junit: _build/reports/go-tests-report-go-${GO_VERSION}-git-${GIT_VERSION}.xml parallel: matrix: # These definitions are for the non-default Git versions. We do not want -- GitLab From 52484cf44f23461885e93546194c6efac4fff9ba Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 11 Oct 2021 14:47:39 +0200 Subject: [PATCH 5/8] ci: Add job to run quarantined tests This change adds a jobs that also runs the quarantined tests, but the job is marked to allow failures so it doesn't block deploys when the pipeline on master is red due to flaky tests. --- .gitlab-ci.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 9677c25e443..f18029c321a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -200,6 +200,16 @@ test: - GO_VERSION: "1.17" TARGET: [ test-with-proxies, test-with-praefect, race-go ] +test-quarantine: + <<: *test_definition + <<: *postgres_definition + allow_failure: true + variables: + <<: *postgres_variables + QUARANTINE: 1 + script: + - make test + nightly:git: <<: *test_definition <<: *postgres_definition -- GitLab From 4a3c008651d819bf979f24c88406d5b3eb69ba7d Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 11 Oct 2021 14:48:55 +0200 Subject: [PATCH 6/8] ci: Remove lint job The verify job runs, amongst other things, the lint target in the Makefile. So the job to run `make lint` is redundant, therefore this change removes the lint job. --- .gitlab-ci.yml | 7 ------- 1 file changed, 7 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index f18029c321a..936961a5610 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -313,13 +313,6 @@ backwards_compatibility_test: - git checkout --no-overlay $CI_COMMIT_SHA -- internal/praefect/datastore/migrations - make test-postgres -lint: - stage: test - retry: 2 - script: - - go version - - make lint - objectinfo_fuzz_test: extends: .fuzz_base stage: test -- GitLab From 37bd28c8122ea7656a95cbbec1dbee057645077d Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 11 Oct 2021 14:50:21 +0200 Subject: [PATCH 7/8] datastore: Quarantine test in TestStorageCleanup The test case TestStorageCleanup_AcquireNextStorage is flaky. To stop it from blocking deploys, put it in quarantine. Issue: https://gitlab.com/gitlab-org/gitaly/-/issues/3828 --- internal/praefect/datastore/storage_cleanup_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/praefect/datastore/storage_cleanup_test.go b/internal/praefect/datastore/storage_cleanup_test.go index bb8e66d75bb..f1af0ff5397 100644 --- a/internal/praefect/datastore/storage_cleanup_test.go +++ b/internal/praefect/datastore/storage_cleanup_test.go @@ -146,6 +146,8 @@ func TestStorageCleanup_AcquireNextStorage(t *testing.T) { }) t.Run("acquired for long time triggers update loop", func(t *testing.T) { + testhelper.Quarantine(t, "https://gitlab.com/gitlab-org/gitaly/-/issues/3828") + db.TruncateAll(t) require.NoError(t, storageCleanup.Populate(ctx, "vs", "g1")) start := time.Now().UTC() -- GitLab From bc5814290bddcf1689f3ba0a07ebccd536d82c58 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 11 Oct 2021 14:52:04 +0200 Subject: [PATCH 8/8] WIP: Demonstrate failure in quarantined test This change should not be merged. --- internal/praefect/datastore/storage_cleanup_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/praefect/datastore/storage_cleanup_test.go b/internal/praefect/datastore/storage_cleanup_test.go index f1af0ff5397..53ebd7b1e9e 100644 --- a/internal/praefect/datastore/storage_cleanup_test.go +++ b/internal/praefect/datastore/storage_cleanup_test.go @@ -175,6 +175,12 @@ func TestStorageCleanup_AcquireNextStorage(t *testing.T) { require.Len(t, check3, 1) require.False(t, check3[0].TriggeredAt.Valid) }) + + t.Run("fail in quarantine", func(t *testing.T) { + testhelper.Quarantine(t, "issue-xyz") + + require.True(t, false, "this is supposed to fail in quarantine only") + }) } func TestStorageCleanup_Exists(t *testing.T) { -- GitLab