From a7cd105e34a9f7be6b39f02d5766ac2cbec0e0c8 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Tue, 24 Mar 2020 09:39:11 -0700 Subject: [PATCH 1/3] Fix flaky test TestLimiter --- .../limithandler/concurrency_limiter_test.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/internal/middleware/limithandler/concurrency_limiter_test.go b/internal/middleware/limithandler/concurrency_limiter_test.go index 735ff77c62..8a68f4422a 100644 --- a/internal/middleware/limithandler/concurrency_limiter_test.go +++ b/internal/middleware/limithandler/concurrency_limiter_test.go @@ -73,7 +73,6 @@ func TestLimiter(t *testing.T) { concurrency int maxConcurrency int iterations int - delay time.Duration buckets int wantMaxRange []int wantMonitorCalls bool @@ -83,7 +82,6 @@ func TestLimiter(t *testing.T) { concurrency: 1, maxConcurrency: 1, iterations: 1, - delay: 1 * time.Millisecond, buckets: 1, wantMaxRange: []int{1, 1}, wantMonitorCalls: true, @@ -93,7 +91,6 @@ func TestLimiter(t *testing.T) { concurrency: 100, maxConcurrency: 2, iterations: 10, - delay: 1 * time.Millisecond, buckets: 1, wantMaxRange: []int{2, 3}, wantMonitorCalls: true, @@ -102,7 +99,6 @@ func TestLimiter(t *testing.T) { name: "two-by-two", concurrency: 100, maxConcurrency: 2, - delay: 1000 * time.Nanosecond, iterations: 4, buckets: 2, wantMaxRange: []int{4, 5}, @@ -113,7 +109,6 @@ func TestLimiter(t *testing.T) { concurrency: 10, maxConcurrency: 0, iterations: 200, - delay: 1000 * time.Nanosecond, buckets: 1, wantMaxRange: []int{8, 10}, wantMonitorCalls: false, @@ -125,7 +120,6 @@ func TestLimiter(t *testing.T) { // We use a long delay here to prevent flakiness in CI. If the delay is // too short, the first goroutines to enter the critical section will be // gone before we hit the intended maximum concurrency. - delay: 5 * time.Millisecond, iterations: 40, buckets: 50, wantMaxRange: []int{95, 105}, @@ -135,7 +129,6 @@ func TestLimiter(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { gauge := &counter{} - start := make(chan struct{}) limiter := NewLimiter(tt.maxConcurrency, gauge) wg := sync.WaitGroup{} @@ -165,7 +158,6 @@ func TestLimiter(t *testing.T) { // concurrently. for c := 0; c < tt.concurrency; c++ { go func(counter int) { - <-start for i := 0; i < tt.iterations; i++ { lockKey := strconv.Itoa((i ^ counter) % tt.buckets) @@ -179,15 +171,12 @@ func TestLimiter(t *testing.T) { gauge.down() return nil, nil }) - - time.Sleep(tt.delay) } wg.Done() }(c) } - close(start) wg.Wait() assert.True(t, tt.wantMaxRange[0] <= gauge.max && gauge.max <= tt.wantMaxRange[1], "Expected maximum concurrency to be in the range [%v,%v] but got %v", tt.wantMaxRange[0], tt.wantMaxRange[1], gauge.max) -- GitLab From fda8dd13c1182c05bc8655d40a7c1a0bf88cbf54 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Thu, 26 Mar 2020 17:00:16 -0700 Subject: [PATCH 2/3] Flaky test job allowed to fail --- .gitlab-ci.yml | 33 +++++++++++++++++++ Makefile | 4 +++ _support/Makefile.template | 5 ++- .../limithandler/concurrency_limiter_test.go | 5 ++- internal/testhelper/buildhelper/flaky.go | 22 +++++++++++++ 5 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 internal/testhelper/buildhelper/flaky.go diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 8d5d24bf06..8adfc7e0e9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -77,6 +77,14 @@ danger-review: - make test - echo 'make test-gitaly-remote TODO https://gitlab.com/gitlab-org/gitaly/issues/1706' +.test_flaky_template: &test_flaky_definition + <<: *go_test_definition + script: + - go version + - git version + - make test-go-flaky + allow_failure: true + verify: <<: *ruby_definition stage: test @@ -137,6 +145,31 @@ test:go1.12-git-2.24-ruby-2.6: image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6-golang-1.12-git-2.24 <<: *test_definition +# Flaky tests +test-flaky:go1.14-git-2.21-ruby-2.6: + image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6-golang-1.13-git-2.24 + <<: *test_flaky_definition + +test-flaky:go1.13-git-2.21-ruby-2.6: + image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6-golang-1.13-git-2.21 + <<: *test_flaky_definition + +test-flaky:go1.13-git-2.22-ruby-2.6: + image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6-golang-1.13-git-2.22 + <<: *test_flaky_definition + +test-flaky:go1.12-git-2.22-ruby-2.6: + image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6-golang-1.12-git-2.22 + <<: *test_flaky_definition + +test-flaky:go1.13-git-2.24-ruby-2.6: + image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6-golang-1.13-git-2.24 + <<: *test_flaky_definition + +test-flaky:go1.12-git-2.24-ruby-2.6: + image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6-golang-1.12-git-2.24 + <<: *test_flaky_definition + test:proxy: <<: *test_definition script: diff --git a/Makefile b/Makefile index b839541e0d..80cc6fc764 100644 --- a/Makefile +++ b/Makefile @@ -60,6 +60,10 @@ prepare-tests: prepare-build test: prepare-build cd $(BUILD_DIR) && $(MAKE) $@ +.PHONY: test-go-flaky +test-go-flaky: prepare-build + cd $(BUILD_DIR) && $(MAKE) $@ + .PHONY: test-with-praefect test-with-praefect: prepare-build cd $(BUILD_DIR) && $(MAKE) $@ diff --git a/_support/Makefile.template b/_support/Makefile.template index 60f334e0ef..afc5e6d154 100644 --- a/_support/Makefile.template +++ b/_support/Makefile.template @@ -125,12 +125,15 @@ test: test-go rspec rspec-gitlab-shell test-go: prepare-tests @cd {{ .SourceDir }} && go test -tags "$(BUILD_TAGS)" -count=1 {{ join .AllPackages " " }} # count=1 bypasses go 1.10 test caching +.PHONY: test-go-flaky +test-go-flaky: prepare-tests + @cd {{ .SourceDir }} && ENABLE_FLAKY_TESTS=1 go test -run=TestFlaky -tags "$(BUILD_TAGS)" -count=1 {{ join .AllPackages " " }} # count=1 bypasses go 1.10 test caching + .PHONY: test-with-proxies test-with-proxies: prepare-tests @cd {{ .SourceDir }} &&\ go test -tags "$(BUILD_TAGS)" -count=1 -exec {{ .SourceDir }}/_support/bad-proxies {{ .Pkg }}/internal/rubyserver/ - .PHONY: test-with-praefect test-with-praefect: build prepare-tests @cd {{ .SourceDir }} &&\ diff --git a/internal/middleware/limithandler/concurrency_limiter_test.go b/internal/middleware/limithandler/concurrency_limiter_test.go index 8a68f4422a..b351cec6fc 100644 --- a/internal/middleware/limithandler/concurrency_limiter_test.go +++ b/internal/middleware/limithandler/concurrency_limiter_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "gitlab.com/gitlab-org/gitaly/internal/testhelper/buildhelper" ) type counter struct { @@ -67,7 +68,9 @@ func (c *counter) Exit(ctx context.Context) { c.exit++ } -func TestLimiter(t *testing.T) { +func TestFlakyLimiter(t *testing.T) { + buildhelper.SkipFlakyTest(t) + tests := []struct { name string concurrency int diff --git a/internal/testhelper/buildhelper/flaky.go b/internal/testhelper/buildhelper/flaky.go new file mode 100644 index 0000000000..2a794c5df3 --- /dev/null +++ b/internal/testhelper/buildhelper/flaky.go @@ -0,0 +1,22 @@ +package buildhelper + +import ( + "os" + "strings" + "testing" +) + +// SkipFlakyTest will ensure that the test is skipped if the appropriate +// environment variable is enabled. Additionally, the test name will be asserted +// to ensure it follows the appropriate naming scheme. +func SkipFlakyTest(t *testing.T) { + const testPrefix = "TestFlaky" + if !strings.HasPrefix(t.Name(), testPrefix) { + t.Fatalf("test name %q must begin with %q to be marked flaky", + t.Name(), testPrefix, + ) + } + if os.Getenv("ENABLE_FLAKY_TESTS") != "1" { + t.Skip("flaky test is not enabled") + } +} -- GitLab From 2930daf5c35186ecc6790e712e8fdf97e6b054b8 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Thu, 26 Mar 2020 20:41:21 -0700 Subject: [PATCH 3/3] Changelog for MR 1975 --- changelogs/unreleased/po-fix-flaky-test-limiter-too-high.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/po-fix-flaky-test-limiter-too-high.yml diff --git a/changelogs/unreleased/po-fix-flaky-test-limiter-too-high.yml b/changelogs/unreleased/po-fix-flaky-test-limiter-too-high.yml new file mode 100644 index 0000000000..dbb27bdb82 --- /dev/null +++ b/changelogs/unreleased/po-fix-flaky-test-limiter-too-high.yml @@ -0,0 +1,5 @@ +--- +title: Allow flaky test TestLimiter to fail in CI with warning +merge_request: 1975 +author: +type: other -- GitLab