Skip to content

Add specs to catch recursive feature-flag requests

Sylvester Chin requested to merge sc1-test-recursive-ff into master

What does this MR do and why?

This MR adds a spec to run Feature.enabled? through Rails.cache by adding use_clean_rails_redis_caching to the example. This catches usage of feature flags in any Redis instrumentation middleware or Gitlab::Redis::Cache/ActiveSupport::Cache::RedisStoreCache patches.

See #385237 for more background

Two complications when testing out the efficacy of this spec:

  1. The line of code preventing it from being captured in gdk rails c has been fixed in MR.
  2. The code causing the recursion has been reverted

This new spec was tested on a commit between its introduction and removal: https://gitlab.com/gitlab-org/gitlab/-/commits/sc1-investigate. The diff is fairly small, containing 1 line of code change and 5 lines of spec changes (same as this MR).

To test it, checkout to git checkout sc1-investigate and run the spec.

➜  gitlab git:(sc1-investigate) ✗ CI=true bundle exec rspec spec/lib/feature_spec.rb:165  -r spec_helper
/Users/sylvesterchin/work/gitlab-development-kit/gitlab/rspec/flaky/suite-report.json doesn't exist
Run options: include {:locations=>{"./spec/lib/feature_spec.rb"=>[165]}}
==> Starting GitLab Elasticsearch Indexer set up...
==> Starting Gitaly set up...

Test environment set up in 1.476678 seconds

1st Try error in ./spec/lib/feature_spec.rb:166:
(Feature).enabled?(:validate_allowed_cross_slot_commands, {:type=>:development})
    expected: 1 time with any arguments
    received: 2 times with arguments: (:validate_allowed_cross_slot_commands, {:type=>:development})

RSpec::Retry: 2nd try ./spec/lib/feature_spec.rb:166
F

Failures:

  1) Feature.enabled? when using redis cache does not make recursive feature-flag calls
     Failure/Error: Feature.enabled?(:validate_allowed_cross_slot_commands, type: :development)

       (Feature).enabled?(:validate_allowed_cross_slot_commands, {:type=>:development})
           expected: 1 time with any arguments
           received: 2 times with arguments: (:validate_allowed_cross_slot_commands, {:type=>:development})
     # ./lib/gitlab/instrumentation/redis_base.rb:82:in `block in validate_allowed_commands?'
     # ./lib/gitlab/null_request_store.rb:34:in `fetch'
     # ./lib/gitlab/safe_request_store.rb:12:in `fetch'
     # ./lib/gitlab/instrumentation/redis_base.rb:81:in `validate_allowed_commands?'
....
Finished in 3.06 seconds (files took 1 minute 0.4 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/lib/feature_spec.rb:166 # Feature.enabled? when using redis cache does not make recursive feature-flag calls

[TEST PROF INFO] Time spent in factories: 00:00.010 (0.03% of total time)

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Sylvester Chin

Merge request reports