Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitlab-foss.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMichael Kozono <mkozono@gmail.com>2017-10-31 22:02:18 +0300
committerFrancisco Lopez <fjlopez@gitlab.com>2017-11-17 11:58:18 +0300
commit91628170c7ed6d6f9ad0e6a19c6ca8fc69672b1d (patch)
treec706c48e164aad76041be63eb8977bd610efcaab /spec/requests/rack_attack_global_spec.rb
parent203e3b2d072b3a673fda651dafdacb0e0cd6f1ae (diff)
Remove extra attempts
Get the tests deterministic or bust! Also, explain what the constants are doing and why. And don’t output the log message if it doesn’t apply.
Diffstat (limited to 'spec/requests/rack_attack_global_spec.rb')
-rw-r--r--spec/requests/rack_attack_global_spec.rb29
1 files changed, 22 insertions, 7 deletions
diff --git a/spec/requests/rack_attack_global_spec.rb b/spec/requests/rack_attack_global_spec.rb
index c5f48d3b0cc..7db8264f12b 100644
--- a/spec/requests/rack_attack_global_spec.rb
+++ b/spec/requests/rack_attack_global_spec.rb
@@ -1,7 +1,22 @@
require 'spec_helper'
describe 'Rack Attack global throttles' do
- NUM_TRIES_FOR_REJECTION = 3 # Flaky tests, have not figured out how to fix it
+ # If the tests are being flaky as described below, then this constant
+ # can be set to greater than 1 to make multiple attempts to get a 429.
+ #
+ # In tests exceeding the rate limit within a time period (which we know we
+ # have accomplished because we've made exactly 1 more request than allowed
+ # while time is stopped) we expect a 429. But sometimes we get a 200,
+ # sometimes for more than one request, but eventually we get a 429. This
+ # constant and its usages should be removed if we figure out why this happens.
+ NUM_TRIES_FOR_REJECTION = 1
+
+ # Extra time travel past what should be strictly necessary to ensure the
+ # throttle we are testing is using a cache key where the request count is 0.
+ #
+ # Why add this? Sometimes we get a 429 when we expect a 200. This constant and
+ # its usages should be removed if we figure out why this happens.
+ NEXT_TIME_PERIOD_BUFFER = 0.seconds
let(:settings) { Gitlab::CurrentSettings.current_application_settings }
@@ -26,11 +41,11 @@ describe 'Rack Attack global throttles' do
let(:url_that_requires_authentication) { '/dashboard/snippets' }
let(:api_partial_url) { '/todos' }
- # Make time-dependent tests deterministic
around do |example|
- # Instead of test environment's :null_store
+ # Instead of test environment's :null_store so the throttles can increment
Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
+ # Make time-dependent tests deterministic
Timecop.freeze { example.run }
Rack::Attack.cache.store = Rails.cache
@@ -72,7 +87,7 @@ describe 'Rack Attack global throttles' do
expect_rejection { get(*get_args) }
- Timecop.travel((1.second + period).from_now) do # Add 1 because flaky
+ Timecop.travel((NEXT_TIME_PERIOD_BUFFER + period).from_now) do
requests_per_period.times do
get(*get_args)
expect(response).to have_http_status 200
@@ -152,7 +167,7 @@ describe 'Rack Attack global throttles' do
expect_rejection { get url_that_does_not_require_authentication }
- Timecop.travel((1.second + period).from_now) do # Add 1 because flaky
+ Timecop.travel((NEXT_TIME_PERIOD_BUFFER + period).from_now) do
requests_per_period.times do
get url_that_does_not_require_authentication
expect(response).to have_http_status 200
@@ -315,7 +330,7 @@ describe 'Rack Attack global throttles' do
expect_rejection { get url_that_requires_authentication }
- Timecop.travel((1.second + period).from_now) do # Add 1 because flaky
+ Timecop.travel((NEXT_TIME_PERIOD_BUFFER + period).from_now) do
requests_per_period.times do
get url_that_requires_authentication
expect(response).to have_http_status 200
@@ -389,7 +404,7 @@ describe 'Rack Attack global throttles' do
NUM_TRIES_FOR_REJECTION.times do |i|
yield
break if response.status == 429 # success
- Rails.logger.warn "Flaky test expected HTTP status 429 but got #{response.status}. Will attempt again (#{i + 1}/#{NUM_TRIES_FOR_REJECTION})"
+ Rails.logger.warn "Flaky test expected HTTP status 429 but got #{response.status}. Will attempt again (#{i + 1}/#{NUM_TRIES_FOR_REJECTION})" if i + 1 < NUM_TRIES_FOR_REJECTION
end
expect(response).to have_http_status(429)