Commit 98f5140f authored by Shinya Maeda's avatar Shinya Maeda

Merge branch 'use-memory-flipper' into 'master'

Use a memoized Flipper engine in specs to simplify testing

See merge request gitlab-org/gitlab!33054
parents 2ff42f2c 19b4e263
......@@ -23,6 +23,9 @@ class Foo < ActiveRecord::Base
end
```
Only models that `include FeatureGate` or expose `flipper_id` method can be
used as an actor for `Feature.enabled?`.
Features that are developed and are intended to be merged behind a feature flag
should not include a changelog entry. The entry should either be added in the merge
request removing the feature flag or the merge request where the default value of
......@@ -119,17 +122,30 @@ how to access feature flags in a Vue component.
### Specs
In the test environment `Feature.enabled?` is stubbed to always respond to `true`,
so we make sure behavior under feature flag doesn't go untested in some non-specific
contexts.
Our Flipper engine in the test environment works in a memory mode `Flipper::Adapters::Memory`.
`production` and `development` modes use `Flipper::Adapters::ActiveRecord`.
### `stub_feature_flags: true` (default and preferred)
In this mode Flipper is configured to use `Flipper::Adapters::Memory` and mark all feature
flags to be on-by-default and persisted on a first use. This overwrites the `default_enabled:`
of `Feature.enabled?` and `Feature.disabled?` returning always `true` unless feature flag
is persisted.
Whenever a feature flag is present, make sure to test _both_ states of the
feature flag.
Make sure behavior under feature flag doesn't go untested in some non-specific contexts.
See the
[testing guide](../testing_guide/best_practices.md#feature-flags-in-tests)
for information and examples on how to stub feature flags in tests.
### `stub_feature_flags: false`
This disables a memory-stubbed flipper, and uses `Flipper::Adapters::ActiveRecord`
a mode that is used by `production` and `development`.
You should use this mode only when you really want to tests aspects of Flipper
with how it interacts with `ActiveRecord`.
### Enabling a feature flag (in development)
In the rails console (`rails c`), enter the following command to enable your feature flag
......@@ -143,3 +159,9 @@ Similarly, the following command will disable a feature flag:
```ruby
Feature.disable(:feature_flag_name)
```
You can as well enable feature flag for a given gate:
```ruby
Feature.enable(:feature_flag_name, Project.find_by_full_path("root/my-project"))
```
......@@ -357,6 +357,60 @@ Feature.enabled?(:my_feature2) # => false
Feature.enabled?(:my_feature2, project1) # => true
```
#### `stub_feature_flags` vs `Feature.get/enable`
It is preferred to use `stub_feature_flags` for enabling feature flags
in testing environment. This method provides a simple and well described
interface for a simple use-cases.
However, in some cases a more complex behaviors needs to be tested,
like a feature flag percentage rollouts. This can be achieved using
the `.enable_percentage_of_time` and `.enable_percentage_of_actors`
```ruby
# Good: feature needs to be explicitly disabled, as it is enabled by default if not defined
stub_feature_flags(my_feature: false)
stub_feature_flags(my_feature: true)
stub_feature_flags(my_feature: project)
stub_feature_flags(my_feature: [project, project2])
# Bad
Feature.enable(:my_feature_2)
# Good: enable my_feature for 50% of time
Feature.get(:my_feature_3).enable_percentage_of_time(50)
# Good: enable my_feature for 50% of actors/gates/things
Feature.get(:my_feature_4).enable_percentage_of_actors(50)
```
Each feature flag that has a defined state will be persisted
for test execution time:
```ruby
Feature.persisted_names.include?('my_feature') => true
Feature.persisted_names.include?('my_feature_2') => true
Feature.persisted_names.include?('my_feature_3') => true
Feature.persisted_names.include?('my_feature_4') => true
```
#### Stubbing gate
It is required that a gate that is passed as an argument to `Feature.enabled?`
and `Feature.disabled?` is an object that includes `FeatureGate`.
In specs you can use a `stub_feature_flag_gate` method that allows you to have
quickly your custom gate:
```ruby
gate = stub_feature_flag_gate('CustomActor')
stub_feature_flags(ci_live_trace: gate)
Feature.enabled?(:ci_live_trace) # => false
Feature.enabled?(:ci_live_trace, gate) # => true
```
### Pristine test environments
The code exercised by a single GitLab test may access and modify many items of
......
......@@ -15,19 +15,8 @@ module EE
def experiment_enabled_for_user?
return true unless current_user
# If EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME is not set, we should return
# true which means available for all
return true unless onboarding_sign_up_experiment_set?
::Feature.enabled?(EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME, current_user)
end
def onboarding_sign_up_experiment_set?
::Feature.persisted?(onboarding_sign_up_experiment_feature)
end
def onboarding_sign_up_experiment_feature
::Feature.get(EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME)
::Feature.enabled?(EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME, current_user, default_enabled: true)
end
end
end
......@@ -15,19 +15,8 @@ module EE
private
def experiment_enabled_for_session?
# If EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME is not set, we should show
# reCAPTCHA on the sign_up page
return true unless recaptcha_sign_up_experiment_set?
::Feature.enabled?(EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME, flipper_session)
end
def recaptcha_sign_up_experiment_set?
::Feature.persisted?(recaptcha_sign_up_experiment_feature)
end
def recaptcha_sign_up_experiment_feature
::Feature.get(EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME)
::Feature.enabled?(EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME, flipper_session,
default_enabled: true)
end
end
end
......@@ -49,7 +49,7 @@ module Gitlab
end
def use_vulnerability_cache?
Feature.enabled?(:cache_vulnerability_summary, vulnerable) && !dynamic_filters_included?
Feature.enabled?(:cache_vulnerability_summary) && !dynamic_filters_included?
end
def dynamic_filters_included?
......
......@@ -55,10 +55,9 @@ describe OnboardingExperimentHelper, type: :helper do
context 'and experiment_growth_onboarding has been set' do
it 'checks if feature is enabled for current_user' do
percentage = ::Feature.flipper.actors(50)
::Feature.flipper[described_class::EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME].enable(percentage)
feature = Feature.get(described_class::EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME)
feature.enable_percentage_of_actors(50)
expect(Feature).to receive(:enabled?).with(described_class::EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME, user).and_call_original
expect(helper.allow_access_to_onboarding?).to eq(true)
end
end
......
......@@ -5,6 +5,12 @@ require 'spec_helper'
describe RecaptchaExperimentHelper, type: :helper do
using RSpec::Parameterized::TableSyntax
let(:session) { {} }
before do
allow(helper).to receive(:session) { session }
end
describe '.show_recaptcha_sign_up?' do
context 'when reCAPTCHA is disabled' do
it 'returns false' do
......@@ -28,18 +34,6 @@ describe RecaptchaExperimentHelper, type: :helper do
context 'and experiment_growth_recaptcha has been set' do
let(:feature_name) { described_class::EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME }
before(:all) do
# We need to create a '50%' of actors feature flag before _any_ test
# runs and need to continue to use the same feature throughout the
# test duration.
fifty_percent = ::Feature.flipper.actors(50)
::Feature.flipper[described_class::EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME].enable(fifty_percent)
end
after(:all) do
Feature.flipper.remove(described_class::EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME)
end
where(:flipper_session_id, :expected_result) do
'00687625-667c-480c-ae2a-9bf861ddd909' | true
'b8b78156-f7b8-4bf4-b906-06a899b84ea3' | false
......@@ -48,8 +42,12 @@ describe RecaptchaExperimentHelper, type: :helper do
end
with_them do
before do
# Enable feature to 50% of actors
Feature.get(feature_name).enable_percentage_of_actors(50)
end
it "returns expected_result" do
allow(Feature).to receive(:enabled?).and_call_original # Allow Feature to _really_ work.
allow(helper).to receive(:flipper_session).and_return(FlipperSession.new(flipper_session_id))
expect(helper.show_recaptcha_sign_up?).to eq(expected_result)
......
......@@ -14,12 +14,16 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgres
end
context 'when there is no more MigrateApproverToApprovalRulesInBatch jobs' do
before do
stub_feature_flags(approval_rule: false)
end
it 'enables feature' do
allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(false)
expect(Feature).to receive(:enable).with(:approval_rule)
described_class.new.perform
expect(Feature.enabled?(:approval_rule)).to eq(true)
end
end
end
......@@ -36,7 +36,7 @@ describe Gitlab::ImportExport::Project::TreeSaver do
before_all do
RSpec::Mocks.with_temporary_scope do
allow(Feature).to receive(:enabled?) { true }
stub_all_feature_flags
stub_feature_flags(project_export_as_ndjson: ndjson_enabled)
project.add_maintainer(user)
......
......@@ -142,6 +142,7 @@ describe ApplicationSetting do
# and we want to make sure we're still testing
# should_check_namespace_plan? method through the test-suite (see
# https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/18461#note_69322821).
allow(Rails).to receive_message_chain(:env, :development?).and_return(false)
allow(Rails).to receive_message_chain(:env, :test?).and_return(false)
end
......
......@@ -18,6 +18,8 @@ class Feature
superclass.table_name = 'feature_gates'
end
InvalidFeatureFlagError = Class.new(Exception) # rubocop:disable Lint/InheritException
class << self
delegate :group, to: :flipper
......@@ -32,26 +34,47 @@ class Feature
def persisted_names
return [] unless Gitlab::Database.exists?
Gitlab::SafeRequestStore[:flipper_persisted_names] ||=
begin
# We saw on GitLab.com, this database request was called 2300
# times/s. Let's cache it for a minute to avoid that load.
Gitlab::ProcessMemoryCache.cache_backend.fetch('flipper:persisted_names', expires_in: 1.minute) do
FlipperFeature.feature_names
if Gitlab::Utils.to_boolean(ENV['FF_LEGACY_PERSISTED_NAMES'])
# To be removed:
# This uses a legacy persisted names that are know to work (always)
Gitlab::SafeRequestStore[:flipper_persisted_names] ||=
begin
# We saw on GitLab.com, this database request was called 2300
# times/s. Let's cache it for a minute to avoid that load.
Gitlab::ProcessMemoryCache.cache_backend.fetch('flipper:persisted_names', expires_in: 1.minute) do
FlipperFeature.feature_names
end.to_set
end
end
else
# This loads names of all stored feature flags
# and returns a stable Set in the following order:
# - Memoized: using Gitlab::SafeRequestStore or @flipper
# - L1: using Process cache
# - L2: using Redis cache
# - DB: using a single SQL query
flipper.adapter.features
end
end
def persisted?(feature)
def persisted_name?(feature_name)
# Flipper creates on-memory features when asked for a not-yet-created one.
# If we want to check if a feature has been actually set, we look for it
# on the persisted features list.
persisted_names.include?(feature.name.to_s)
persisted_names.include?(feature_name.to_s)
end
# use `default_enabled: true` to default the flag to being `enabled`
# unless set explicitly. The default is `disabled`
# TODO: remove the `default_enabled:` and read it from the `defintion_yaml`
# check: https://gitlab.com/gitlab-org/gitlab/-/issues/30228
def enabled?(key, thing = nil, default_enabled: false)
if check_feature_flags_definition?
if thing && !thing.respond_to?(:flipper_id)
raise InvalidFeatureFlagError,
"The thing '#{thing.class.name}' for feature flag '#{key}' needs to include `FeatureGate` or implement `flipper_id`"
end
end
# During setup the database does not exist yet. So we haven't stored a value
# for the feature yet and return the default.
return default_enabled unless Gitlab::Database.exists?
......@@ -62,7 +85,7 @@ class Feature
# `persisted?` can potentially generate DB queries and also checks for inclusion
# in an array of feature names (177 at last count), possibly reducing performance by half.
# So we only perform the `persisted` check if `default_enabled: true`
!default_enabled || Feature.persisted?(feature) ? feature.enabled?(thing) : true
!default_enabled || Feature.persisted_name?(feature.name) ? feature.enabled?(thing) : true
end
def disabled?(key, thing = nil, default_enabled: false)
......@@ -87,10 +110,9 @@ class Feature
end
def remove(key)
feature = get(key)
return unless persisted?(feature)
return unless persisted_name?(key)
feature.remove
get(key).remove
end
def flipper
......@@ -101,17 +123,12 @@ class Feature
end
end
def build_flipper_instance
Flipper.new(flipper_adapter).tap { |flip| flip.memoize = true }
def reset
Gitlab::SafeRequestStore.delete(:flipper) if Gitlab::SafeRequestStore.active?
@flipper = nil
end
# This method is called from config/initializers/flipper.rb and can be used
# to register Flipper groups.
# See https://docs.gitlab.com/ee/development/feature_flags.html#feature-groups
def register_feature_groups
end
def flipper_adapter
def build_flipper_instance
active_record_adapter = Flipper::Adapters::ActiveRecord.new(
feature_class: FlipperFeature,
gate_class: FlipperGate)
......@@ -125,10 +142,26 @@ class Feature
# Thread-local L1 cache: use a short timeout since we don't have a
# way to expire this cache all at once
Flipper::Adapters::ActiveSupportCacheStore.new(
flipper_adapter = Flipper::Adapters::ActiveSupportCacheStore.new(
redis_cache_adapter,
l1_cache_backend,
expires_in: 1.minute)
Flipper.new(flipper_adapter).tap do |flip|
flip.memoize = true
end
end
def check_feature_flags_definition?
# We want to check feature flags usage only when
# running in development or test environment
Gitlab.dev_or_test_env?
end
# This method is called from config/initializers/flipper.rb and can be used
# to register Flipper groups.
# See https://docs.gitlab.com/ee/development/feature_flags.html#feature-groups
def register_feature_groups
end
def l1_cache_backend
......
......@@ -5,8 +5,7 @@ module Gitlab
module RuggedImpl
module UseRugged
def use_rugged?(repo, feature_key)
feature = Feature.get(feature_key)
return feature.enabled? if Feature.persisted?(feature)
return Feature.enabled?(feature_key) if Feature.persisted_name?(feature_key)
# Disable Rugged auto-detect(can_use_disk?) when Puma threads>1
# https://gitlab.com/gitlab-org/gitlab/issues/119326
......
......@@ -25,7 +25,7 @@ describe SourcegraphDecorator do
end
before do
Feature.get(:sourcegraph).enable(feature_enabled)
stub_feature_flags(sourcegraph: feature_enabled)
stub_application_setting(sourcegraph_url: sourcegraph_url, sourcegraph_enabled: sourcegraph_enabled, sourcegraph_public_only: sourcegraph_public_only)
......
......@@ -135,25 +135,6 @@ describe Types::BaseField do
it 'returns true if the feature is enabled' do
expect(field.visible?(context)).to eq(true)
end
context 'falsey feature_flag values' do
using RSpec::Parameterized::TableSyntax
where(:flag, :feature_value, :visible) do
'' | false | true
'' | true | true
nil | false | true
nil | true | true
end
with_them do
it 'returns the correct value' do
stub_feature_flags(flag => feature_value)
expect(field.visible?(context)).to eq(visible)
end
end
end
end
end
end
......
......@@ -3,6 +3,12 @@
require 'spec_helper'
describe RecaptchaExperimentHelper, type: :helper do
let(:session) { {} }
before do
allow(helper).to receive(:session) { session }
end
describe '.show_recaptcha_sign_up?' do
context 'when reCAPTCHA is disabled' do
it 'returns false' do
......
......@@ -3,12 +3,14 @@
require 'spec_helper'
describe Banzai::Pipeline::DescriptionPipeline do
let_it_be(:project) { create(:project) }
def parse(html)
# When we pass HTML to Redcarpet, it gets wrapped in `p` tags...
# ...except when we pass it pre-wrapped text. Rabble rabble.
unwrap = !html.start_with?('<p ')
output = described_class.to_html(html, project: spy)
output = described_class.to_html(html, project: project)
output.gsub!(%r{\A<p dir="auto">(.*)</p>(.*)\z}, '\1\2') if unwrap
......
......@@ -3,6 +3,11 @@
require 'spec_helper'
describe Banzai::Pipeline::WikiPipeline do
let_it_be(:namespace) { create(:namespace, name: "wiki_link_ns") }
let_it_be(:project) { create(:project, :public, name: "wiki_link_project", namespace: namespace) }
let_it_be(:wiki) { ProjectWiki.new(project, double(:user)) }
let_it_be(:page) { build(:wiki_page, wiki: wiki, title: 'nested/twice/start-page') }
describe 'TableOfContents' do
it 'replaces the tag with the TableOfContentsFilter result' do
markdown = <<-MD.strip_heredoc
......@@ -13,7 +18,7 @@ describe Banzai::Pipeline::WikiPipeline do
Foo
MD
result = described_class.call(markdown, project: spy, wiki: spy)
result = described_class.call(markdown, project: project, wiki: wiki)
aggregate_failures do
expect(result[:output].text).not_to include '[['
......@@ -31,7 +36,7 @@ describe Banzai::Pipeline::WikiPipeline do
Foo
MD
output = described_class.to_html(markdown, project: spy, wiki: spy)
output = described_class.to_html(markdown, project: project, wiki: wiki)
expect(output).to include('[[<em>toc</em>]]')
end
......@@ -44,7 +49,7 @@ describe Banzai::Pipeline::WikiPipeline do
Foo
MD
output = described_class.to_html(markdown, project: spy, wiki: spy)
output = described_class.to_html(markdown, project: project, wiki: wiki)
aggregate_failures do
expect(output).not_to include('<ul>')
......@@ -54,11 +59,6 @@ describe Banzai::Pipeline::WikiPipeline do
end
describe "Links" do
let(:namespace) { create(:namespace, name: "wiki_link_ns") }
let(:project) { create(:project, :public, name: "wiki_link_project", namespace: namespace) }
let(:wiki) { ProjectWiki.new(project, double(:user)) }
let(:page) { build(:wiki_page, wiki: wiki, title: 'nested/twice/start-page') }
{ 'when GitLab is hosted at a root URL' => '',
'when GitLab is hosted at a relative URL' => '/nested/relative/gitlab' }.each do |test_name, relative_url_root|
context test_name do
......@@ -261,11 +261,6 @@ describe Banzai::Pipeline::WikiPipeline do
end
describe 'videos and audio' do
let_it_be(:namespace) { create(:namespace, name: "wiki_link_ns") }
let_it_be(:project) { create(:project, :public, name: "wiki_link_project", namespace: namespace) }
let_it_be(:wiki) { ProjectWiki.new(project, double(:user)) }
let_it_be(:page) { build(:wiki_page, wiki: wiki, title: 'nested/twice/start-page') }
it 'generates video html structure' do
markdown = "![video_file](video_file_name.mp4)"
output = described_class.to_html(markdown, project: project, wiki: wiki, page_slug: page.slug)
......
......@@ -5,9 +5,12 @@ require 'spec_helper'
describe Constraints::FeatureConstrainer do
describe '#matches' do
it 'calls Feature.enabled? with the correct arguments' do
expect(Feature).to receive(:enabled?).with(:feature_name, "an object", default_enabled: true)
gate = stub_feature_flag_gate("an object")
described_class.new(:feature_name, "an object", default_enabled: true).matches?(double('request'))
expect(Feature).to receive(:enabled?)
.with(:feature_name, gate, default_enabled: true)
described_class.new(:feature_name, gate, default_enabled: true).matches?(double('request'))
end
end
end
......@@ -25,7 +25,7 @@ describe Feature::Gitaly do
describe ".server_feature_flags" do
before do
allow(Feature).to receive(:persisted_names).and_return(%w[gitaly_mep_mep foo])
stub_feature_flags(gitaly_mep_mep: true, foo: true)
end
subject { described_class.server_feature_flags }
......
......@@ -2,12 +2,10 @@
require 'spec_helper'
describe Feature do
describe Feature, stub_feature_flags: false do
before do
# We mock all calls to .enabled? to return true in order to force all
# specs to run the feature flag gated behavior, but here we need a clean
# behavior from the class
allow(described_class).to receive(:enabled?).and_call_original
# reset Flipper AR-engine
Feature.reset
end
describe '.get' do
......@@ -23,67 +21,106 @@ describe Feature do
end
describe '.persisted_names' do
it 'returns the names of the persisted features' do
Feature::FlipperFeature.create!(key: 'foo')
context 'when FF_LEGACY_PERSISTED_NAMES=false' do
before do
stub_env('FF_LEGACY_PERSISTED_NAMES', 'false')
end
expect(described_class.persisted_names).to eq(%w[foo])
end
it 'returns the names of the persisted features' do
Feature.enable('foo')
expect(described_class.persisted_names).to contain_exactly('foo')
end
it 'returns an empty Array when no features are presisted' do
expect(described_class.persisted_names).to be_empty
end
it 'caches the feature names when request store is active',
:request_store, :use_clean_rails_memory_store_caching do
Feature.enable('foo')
it 'returns an empty Array when no features are presisted' do
expect(described_class.persisted_names).to be_empty
expect(Gitlab::ProcessMemoryCache.cache_backend)
.to receive(:fetch)
.once
.with('flipper/v1/features', expires_in: 1.minute)
.and_call_original
2.times do
expect(described_class.persisted_names).to contain_exactly('foo')
end
end
end
it 'caches the feature names when request store is active',
context 'when FF_LEGACY_PERSISTED_NAMES=true' do
before do
stub_env('FF_LEGACY_PERSISTED_NAMES', 'true')
end
it 'returns the names of the persisted features' do
Feature.enable('foo')
expect(described_class.persisted_names).to contain_exactly('foo')
end
it 'returns an empty Array when no features are presisted' do
expect(described_class.persisted_names).to be_empty
end
it 'caches the feature names when request store is active',
:request_store, :use_clean_rails_memory_store_caching do
Feature::FlipperFeature.create!(key: 'foo')
Feature.enable('foo')
expect(Feature::FlipperFeature)
.to receive(:feature_names)
.once
.and_call_original
expect(Gitlab::ProcessMemoryCache.cache_backend)
.to receive(:fetch)
.once
.with('flipper:persisted_names', expires_in: 1.minute)
.and_call_original
expect(Gitlab::ProcessMemoryCache.cache_backend)
.to receive(:fetch)
.once
.with('flipper:persisted_names', expires_in: 1.minute)
.and_call_original
2.times do
expect(described_class.persisted_names).to contain_exactly('foo')
end
end
end
2.times do
expect(described_class.persisted_names).to eq(%w[foo])
it 'fetches all flags once in a single query', :request_store do
Feature.enable('foo1')
Feature.enable('foo2')
queries = ActiveRecord::QueryRecorder.new(skip_cached: false) do
expect(described_class.persisted_names).to contain_exactly('foo1', 'foo2')
RequestStore.clear!
expect(described_class.persisted_names).to contain_exactly('foo1', 'foo2')
end
expect(queries.count).to eq(1)
end
end
describe '.persisted?' do
describe '.persisted_name?' do
context 'when the feature is persisted' do
it 'returns true when feature name is a string' do
Feature::FlipperFeature.create!(key: 'foo')
feature = double(:feature, name: 'foo')
Feature.enable('foo')
expect(described_class.persisted?(feature)).to eq(true)
expect(described_class.persisted_name?('foo')).to eq(true)
end
it 'returns true when feature name is a symbol' do
Feature::FlipperFeature.create!(key: 'foo')
Feature.enable('foo')
feature = double(:feature, name: :foo)
expect(described_class.persisted?(feature)).to eq(true)
expect(described_class.persisted_name?(:foo)).to eq(true)
end
end
context 'when the feature is not persisted' do
it 'returns false when feature name is a string' do
feature = double(:feature, name: 'foo')
expect(described_class.persisted?(feature)).to eq(false)
expect(described_class.persisted_name?('foo')).to eq(false)
end
it 'returns false when feature name is a symbol' do
feature = double(:feature, name: :bar)
expect(described_class.persisted?(feature)).to eq(false)
expect(described_class.persisted_name?(:bar)).to eq(false)
end
end
end
......@@ -100,10 +137,6 @@ describe Feature do
end
describe '.flipper' do
before do
described_class.instance_variable_set(:@flipper, nil)
end
context 'when request store is inactive' do
it 'memoizes the Flipper instance' do
expect(Flipper).to receive(:new).once.and_call_original
......@@ -216,10 +249,8 @@ describe Feature do
end
context 'with an individual actor' do
CustomActor = Struct.new(:flipper_id)
let(:actor) { CustomActor.new(flipper_id: 'CustomActor:5') }
let(:another_actor) { CustomActor.new(flipper_id: 'CustomActor:10') }
let(:actor) { stub_feature_flag_gate('CustomActor:5') }
let(:another_actor) { stub_feature_flag_gate('CustomActor:10') }
before do
described_class.enable(:enabled_feature_flag, actor)
......@@ -237,6 +268,17 @@ describe Feature do
expect(described_class.enabled?(:enabled_feature_flag)).to be_falsey
end
end
context 'with invalid actor' do
let(:actor) { double('invalid actor') }
context 'when is dev_or_test_env' do
it 'does raise exception' do
expect { described_class.enabled?(:enabled_feature_flag, actor) }
.to raise_error /needs to include `FeatureGate` or implement `flipper_id`/
end
end
end
end
describe '.disable?' do
......
......@@ -54,6 +54,10 @@ describe Gitlab::BackgroundMigration::MergeRequestAssigneesMigrationProgressChec
end
context 'when there are no scheduled, or retrying or dead' do
before do
stub_feature_flags(multiple_merge_request_assignees: false)
end
it 'enables feature' do
allow(Gitlab::BackgroundMigration).to receive(:exists?)
.with('PopulateMergeRequestAssigneesTable')
......@@ -67,9 +71,9 @@ describe Gitlab::BackgroundMigration::MergeRequestAssigneesMigrationProgressChec
.with('PopulateMergeRequestAssigneesTable')
.and_return(false)
expect(Feature).to receive(:enable).with(:multiple_merge_request_assignees)
described_class.new.perform
expect(Feature.enabled?(:multiple_merge_request_assignees)).to eq(true)
end
end
......
......@@ -5,19 +5,13 @@ require 'spec_helper'
describe Gitlab::Chat, :use_clean_rails_memory_store_caching do
describe '.available?' do
it 'returns true when the chatops feature is available' do
allow(Feature)
.to receive(:enabled?)
.with(:chatops, default_enabled: true)
.and_return(true)
stub_feature_flags(chatops: true)
expect(described_class).to be_available
end
it 'returns false when the chatops feature is not available' do
allow(Feature)
.to receive(:enabled?)
.with(:chatops, default_enabled: true)
.and_return(false)
stub_feature_flags(chatops: false)
expect(described_class).not_to be_available
end
......
......@@ -11,7 +11,7 @@ describe Gitlab::Experimentation do
}
})
allow(Feature).to receive(:get).with(:test_experiment_experiment_percentage).and_return double(percentage_of_time_value: enabled_percentage)
Feature.get(:test_experiment_experiment_percentage).enable_percentage_of_time(enabled_percentage)
end
let(:environment) { Rails.env.test? }
......
......@@ -49,10 +49,6 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
end
context 'when feature flag is not persisted' do
before do
allow(Feature).to receive(:persisted?).with(feature_flag).and_return(false)
end
context 'when running puma with multiple threads' do
before do
allow(subject).to receive(:running_puma_with_multiple_threads?).and_return(true)
......@@ -97,18 +93,15 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
end
context 'when feature flag is persisted' do
before do
allow(Feature).to receive(:persisted?).with(feature_flag).and_return(true)
end
it 'returns false when the feature flag is off' do
allow(feature_flag).to receive(:enabled?).and_return(false)
Feature.disable(feature_flag_name)
expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey
end
it "returns true when feature flag is on" do
allow(feature_flag).to receive(:enabled?).and_return(true)
Feature.enable(feature_flag_name)
allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(false)
expect(subject.use_rugged?(repository, feature_flag_name)).to be true
......
......@@ -12,21 +12,19 @@ describe Gitlab::GonHelper do
describe '#push_frontend_feature_flag' do
it 'pushes a feature flag to the frontend' do
gon = instance_double('gon')
thing = stub_feature_flag_gate('thing')
stub_feature_flags(my_feature_flag: thing)
allow(helper)
.to receive(:gon)
.and_return(gon)
expect(Feature)
.to receive(:enabled?)
.with(:my_feature_flag, 10)
.and_return(true)
expect(gon)
.to receive(:push)
.with({ features: { 'myFeatureFlag' => true } }, true)
helper.push_frontend_feature_flag(:my_feature_flag, 10)
helper.push_frontend_feature_flag(:my_feature_flag, thing)
end
end
......
......@@ -26,7 +26,7 @@ describe Gitlab::ImportExport::Project::TreeRestorer do
@project = create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project')
@shared = @project.import_export_shared
allow(Feature).to receive(:enabled?) { true }
stub_all_feature_flags
stub_feature_flags(project_import_ndjson: ndjson_enabled)
setup_import_export_config('complex')
......
......@@ -29,7 +29,7 @@ describe Gitlab::ImportExport::Project::TreeSaver do
before_all do
RSpec::Mocks.with_temporary_scope do
allow(Feature).to receive(:enabled?) { true }
stub_all_feature_flags
stub_feature_flags(project_export_as_ndjson: ndjson_enabled)
project.add_maintainer(user)
......
......@@ -7,7 +7,7 @@ describe Gitlab::Sourcegraph do
let(:feature_scope) { true }
before do
Feature.enable(:sourcegraph, feature_scope)
stub_feature_flags(sourcegraph: feature_scope)
end
describe '.feature_conditional?' do
......
......@@ -27,14 +27,13 @@ describe Gitlab::Tracking do
expect(subject.snowplow_options(nil)).to match(expected_fields)
end
it 'enables features using feature flags' do
stub_feature_flags(additional_snowplow_tracking: :__group__)
addition_feature_fields = {
it 'when feature flag is disabled' do
stub_feature_flags(additional_snowplow_tracking: false)
expect(subject.snowplow_options(nil)).to include(
formTracking: false,
linkClickTracking: false
}
expect(subject.snowplow_options(:_group_)).to include(addition_feature_fields)
)
end
end
......
......@@ -7,7 +7,7 @@ describe Gitlab::WikiPages::FrontMatterParser do
let(:content) { 'This is the content' }
let(:end_divider) { '---' }
let(:gate) { double('Gate') }
let(:gate) { stub_feature_flag_gate('Gate') }
let(:with_front_matter) do
<<~MD
......
......@@ -4261,7 +4261,6 @@ describe Project do
describe '#auto_devops_enabled?' do
before do
allow(Feature).to receive(:enabled?).and_call_original
Feature.get(:force_autodevops_on_by_default).enable_percentage_of_actors(0)
end
......@@ -4464,7 +4463,6 @@ describe Project do
let_it_be(:project, reload: true) { create(:project) }
before do
allow(Feature).to receive(:enabled?).and_call_original
Feature.get(:force_autodevops_on_by_default).enable_percentage_of_actors(0)
end
......
......@@ -2,11 +2,12 @@
require 'spec_helper'
describe API::Features do
describe API::Features, stub_feature_flags: false do
let_it_be(:user) { create(:user) }
let_it_be(:admin) { create(:admin) }
before do
Feature.reset
Flipper.unregister_groups
Flipper.register(:perf_team) do |actor|
actor.respond_to?(:admin) && actor.admin?
......
......@@ -134,7 +134,8 @@ describe API::Files do
context 'when PATs are used' do
it_behaves_like 'repository files' do
let(:token) { create(:personal_access_token, scopes: ['read_repository'], user: user) }
let(:current_user) { { personal_access_token: token } }
let(:current_user) { user }
let(:api_user) { { personal_access_token: token } }
end
end
......@@ -153,15 +154,17 @@ describe API::Files do
describe "GET /projects/:id/repository/files/:file_path" do
shared_examples_for 'repository files' do
let(:api_user) { current_user }
it 'returns 400 for invalid file path' do
get api(route(rouge_file_path), current_user), params: params
get api(route(rouge_file_path), api_user), params: params
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq(invalid_file_message)
end
it 'returns file attributes as json' do
get api(route(file_path), current_user), params: params
get api(route(file_path), api_user), params: params
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['file_path']).to eq(CGI.unescape(file_path))
......@@ -174,7 +177,7 @@ describe API::Files do
it 'returns json when file has txt extension' do
file_path = "bar%2Fbranch-test.txt"
get api(route(file_path), current_user), params: params
get api(route(file_path), api_user), params: params
expect(response).to have_gitlab_http_status(:ok)
expect(response.content_type).to eq('application/json')
......@@ -185,7 +188,7 @@ describe API::Files do
file_path = "files%2Fjs%2Fcommit%2Ejs%2Ecoffee"
params[:ref] = "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9"
get api(route(file_path), current_user), params: params
get api(route(file_path), api_user), params: params
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['file_name']).to eq('commit.js.coffee')
......@@ -197,7 +200,7 @@ describe API::Files do
url = route(file_path) + "/raw"
expect(Gitlab::Workhorse).to receive(:send_git_blob)
get api(url, current_user), params: params
get api(url, api_user), params: params
expect(response).to have_gitlab_http_status(:ok)
expect(headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
......@@ -206,7 +209,7 @@ describe API::Files do
it 'returns blame file info' do
url = route(file_path) + '/blame'
get api(url, current_user), params: params
get api(url, api_user), params: params
expect(response).to have_gitlab_http_status(:ok)
end
......@@ -214,7 +217,7 @@ describe API::Files do
it 'sets inline content disposition by default' do
url = route(file_path) + "/raw"
get api(url, current_user), params: params
get api(url, api_user), params: params
expect(headers['Content-Disposition']).to eq(%q(inline; filename="popen.rb"; filename*=UTF-8''popen.rb))
end
......@@ -229,7 +232,7 @@ describe API::Files do
let(:params) { { ref: 'master' } }
it_behaves_like '404 response' do
let(:request) { get api(route('app%2Fmodels%2Fapplication%2Erb'), current_user), params: params }
let(:request) { get api(route('app%2Fmodels%2Fapplication%2Erb'), api_user), params: params }
let(:message) { '404 File Not Found' }
end
end
......@@ -238,7 +241,7 @@ describe API::Files do
include_context 'disabled repository'
it_behaves_like '403 response' do
let(:request) { get api(route(file_path), current_user), params: params }
let(:request) { get api(route(file_path), api_user), params: params }
end
end
end
......@@ -253,7 +256,8 @@ describe API::Files do
context 'when PATs are used' do
it_behaves_like 'repository files' do
let(:token) { create(:personal_access_token, scopes: ['read_repository'], user: user) }
let(:current_user) { { personal_access_token: token } }
let(:current_user) { user }
let(:api_user) { { personal_access_token: token } }
end
end
......
......@@ -396,7 +396,7 @@ describe API::Internal::Base do
context "git pull" do
before do
allow(Feature).to receive(:persisted_names).and_return(%w[gitaly_mep_mep])
stub_feature_flags(gitaly_mep_mep: true)
end
it "has the correct payload" do
......
......@@ -284,7 +284,7 @@ describe API::Repositories do
context "when hotlinking detection is enabled" do
before do
Feature.enable(:repository_archive_hotlinking_interception)
stub_feature_flags(repository_archive_hotlinking_interception: true)
end
it_behaves_like "hotlink interceptor" do
......
......@@ -44,7 +44,7 @@ describe Namespaces::CheckStorageSizeService, '#execute' do
end
it 'errors when feature flag is activated for the current namespace' do
stub_feature_flags(namespace_storage_limit: namespace )
stub_feature_flags(namespace_storage_limit: namespace)
expect(response).to be_error
expect(response.message).to be_present
......
......@@ -172,37 +172,35 @@ RSpec.configure do |config|
end
config.before do |example|
# Enable all features by default for testing
allow(Feature).to receive(:enabled?) { true }
enable_rugged = example.metadata[:enable_rugged].present?
if example.metadata.fetch(:stub_feature_flags, true)
# Enable all features by default for testing
stub_all_feature_flags
# The following can be removed when we remove the staged rollout strategy
# and we can just enable it using instance wide settings
# (ie. ApplicationSetting#auto_devops_enabled)
stub_feature_flags(force_autodevops_on_by_default: false)
# The following can be removed once Vue Issuable Sidebar
# is feature-complete and can be made default in place
# of older sidebar.
# See https://gitlab.com/groups/gitlab-org/-/epics/1863
stub_feature_flags(vue_issuable_sidebar: false)
stub_feature_flags(vue_issuable_epic_sidebar: false)
enable_rugged = example.metadata[:enable_rugged].present?
# Disable Rugged features by default
Gitlab::Git::RuggedImpl::Repository::FEATURE_FLAGS.each do |flag|
stub_feature_flags(flag => enable_rugged)
end
# Disable Rugged features by default
Gitlab::Git::RuggedImpl::Repository::FEATURE_FLAGS.each do |flag|
stub_feature_flags(flag => enable_rugged)
allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(enable_rugged)
end
allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(enable_rugged)
# The following can be removed when we remove the staged rollout strategy
# and we can just enable it using instance wide settings
# (ie. ApplicationSetting#auto_devops_enabled)
stub_feature_flags(force_autodevops_on_by_default: false)
# Enable Marginalia feature for all specs in the test suite.
allow(Gitlab::Marginalia).to receive(:cached_feature_enabled?).and_return(true)
# The following can be removed once Vue Issuable Sidebar
# is feature-complete and can be made default in place
# of older sidebar.
# See https://gitlab.com/groups/gitlab-org/-/epics/1863
stub_feature_flags(vue_issuable_sidebar: false)
stub_feature_flags(vue_issuable_epic_sidebar: false)
allow(Feature).to receive(:enabled?)
.with(/\Apromo_\w+\z/, default_enabled: false)
.and_return(false)
# Stub these calls due to being expensive operations
# It can be reenabled for specific tests via:
#
......
# frozen_string_literal: true
module StubFeatureFlags
class StubFeatureGate
attr_reader :flipper_id
def initialize(flipper_id)
@flipper_id = flipper_id
end
end
def stub_all_feature_flags
adapter = Flipper::Adapters::Memory.new
flipper = Flipper.new(adapter)
allow(Feature).to receive(:flipper).and_return(flipper)
# All new requested flags are enabled by default
allow(Feature).to receive(:enabled?).and_wrap_original do |m, *args|
feature_flag = m.call(*args)
# If feature flag is not persisted we mark the feature flag as enabled
# We do `m.call` as we want to validate the execution of method arguments
# and a feature flag state if it is not persisted
unless Feature.persisted_name?(args.first)
# TODO: this is hack to support `promo_feature_available?`
# We enable all feature flags by default unless they are `promo_`
# Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/218667
feature_flag = true unless args.first.to_s.start_with?('promo_')
end
feature_flag
end
end
# Stub Feature flags with `flag_name: true/false`
#
# @param [Hash] features where key is feature name and value is boolean whether enabled or not.
......@@ -14,23 +46,29 @@ module StubFeatureFlags
# Enable `ci_live_trace` feature flag only on the specified projects.
def stub_feature_flags(features)
features.each do |feature_name, actors|
allow(Feature).to receive(:enabled?).with(feature_name, any_args).and_return(false)
allow(Feature).to receive(:enabled?).with(feature_name.to_s, any_args).and_return(false)
# Remove feature flag overwrite
feature = Feature.get(feature_name)
feature.remove
Array(actors).each do |actor|
raise ArgumentError, "actor cannot be Hash" if actor.is_a?(Hash)
case actor
when false, true
allow(Feature).to receive(:enabled?).with(feature_name, any_args).and_return(actor)
allow(Feature).to receive(:enabled?).with(feature_name.to_s, any_args).and_return(actor)
when nil, ActiveRecord::Base, Symbol, RSpec::Mocks::Double
allow(Feature).to receive(:enabled?).with(feature_name, actor, any_args).and_return(true)
allow(Feature).to receive(:enabled?).with(feature_name.to_s, actor, any_args).and_return(true)
# Control a state of feature flag
if actor == true || actor.nil? || actor.respond_to?(:flipper_id)
feature.enable(actor)
elsif actor == false
feature.disable
else
raise ArgumentError, "#stub_feature_flags accepts only `nil`, `true`, `false`, `ActiveRecord::Base` or `Symbol` as actors"
raise ArgumentError, "#stub_feature_flags accepts only `nil`, `bool`, an object responding to `#flipper_id` or including `FeatureGate`."
end
end
end
end
def stub_feature_flag_gate(object)
return if object.nil?
return object if object.is_a?(FeatureGate)
StubFeatureGate.new(object)
end
end
......@@ -3,16 +3,7 @@
require 'spec_helper'
describe StubFeatureFlags do
before do
# reset stub introduced by `stub_feature_flags`
allow(Feature).to receive(:enabled?).and_call_original
end
context 'if not stubbed' do
it 'features are disabled by default' do
expect(Feature.enabled?(:test_feature)).to eq(false)
end
end
let(:feature_name) { :test_feature }
describe '#stub_feature_flags' do
using RSpec::Parameterized::TableSyntax
......@@ -30,7 +21,7 @@ describe StubFeatureFlags do
with_them do
before do
stub_feature_flags(feature_name => feature_actors)
stub_feature_flags(feature_name => actor(feature_actors))
end
it { expect(Feature.enabled?(feature_name)).to eq(expected_result) }
......@@ -62,15 +53,15 @@ describe StubFeatureFlags do
with_them do
before do
stub_feature_flags(feature_name => feature_actors)
stub_feature_flags(feature_name => actor(feature_actors))
end
it { expect(Feature.enabled?(feature_name, tested_actor)).to eq(expected_result) }
it { expect(Feature.disabled?(feature_name, tested_actor)).not_to eq(expected_result) }
it { expect(Feature.enabled?(feature_name, actor(tested_actor))).to eq(expected_result) }
it { expect(Feature.disabled?(feature_name, actor(tested_actor))).not_to eq(expected_result) }
context 'default_enabled does not impact feature state' do
it { expect(Feature.enabled?(feature_name, tested_actor, default_enabled: true)).to eq(expected_result) }
it { expect(Feature.disabled?(feature_name, tested_actor, default_enabled: true)).not_to eq(expected_result) }
it { expect(Feature.enabled?(feature_name, actor(tested_actor), default_enabled: true)).to eq(expected_result) }
it { expect(Feature.disabled?(feature_name, actor(tested_actor), default_enabled: true)).not_to eq(expected_result) }
end
end
end
......@@ -82,7 +73,7 @@ describe StubFeatureFlags do
end
with_them do
subject { stub_feature_flags(feature_name => feature_actors) }
subject { stub_feature_flags(feature_name => actor(feature_actors)) }
it { expect { subject }.to raise_error(ArgumentError, /accepts only/) }
end
......@@ -90,11 +81,11 @@ describe StubFeatureFlags do
context 'does not raise error' do
where(:feature_actors) do
[true, false, nil, :symbol, double, User.new]
[true, false, nil, stub_feature_flag_gate(100), User.new]
end
with_them do
subject { stub_feature_flags(feature_name => feature_actors) }
subject { stub_feature_flags(feature_name => actor(feature_actors)) }
it { expect { subject }.not_to raise_error }
end
......@@ -103,28 +94,39 @@ describe StubFeatureFlags do
it 'subsquent run changes state' do
# enable FF only on A
stub_feature_flags(test_feature: %i[A])
expect(Feature.enabled?(:test_feature)).to eq(false)
expect(Feature.enabled?(:test_feature, :A)).to eq(true)
expect(Feature.enabled?(:test_feature, :B)).to eq(false)
stub_feature_flags({ feature_name => actor(%i[A]) })
expect(Feature.enabled?(feature_name)).to eq(false)
expect(Feature.enabled?(feature_name, actor(:A))).to eq(true)
expect(Feature.enabled?(feature_name, actor(:B))).to eq(false)
# enable FF only on B
stub_feature_flags(test_feature: %i[B])
expect(Feature.enabled?(:test_feature)).to eq(false)
expect(Feature.enabled?(:test_feature, :A)).to eq(false)
expect(Feature.enabled?(:test_feature, :B)).to eq(true)
stub_feature_flags({ feature_name => actor(%i[B]) })
expect(Feature.enabled?(feature_name)).to eq(false)
expect(Feature.enabled?(feature_name, actor(:A))).to eq(false)
expect(Feature.enabled?(feature_name, actor(:B))).to eq(true)
# enable FF on all
stub_feature_flags(test_feature: true)
expect(Feature.enabled?(:test_feature)).to eq(true)
expect(Feature.enabled?(:test_feature, :A)).to eq(true)
expect(Feature.enabled?(:test_feature, :B)).to eq(true)
stub_feature_flags({ feature_name => true })
expect(Feature.enabled?(feature_name)).to eq(true)
expect(Feature.enabled?(feature_name, actor(:A))).to eq(true)
expect(Feature.enabled?(feature_name, actor(:B))).to eq(true)
# disable FF on all
stub_feature_flags(test_feature: false)
expect(Feature.enabled?(:test_feature)).to eq(false)
expect(Feature.enabled?(:test_feature, :A)).to eq(false)
expect(Feature.enabled?(:test_feature, :B)).to eq(false)
stub_feature_flags({ feature_name => false })
expect(Feature.enabled?(feature_name)).to eq(false)
expect(Feature.enabled?(feature_name, actor(:A))).to eq(false)
expect(Feature.enabled?(feature_name, actor(:B))).to eq(false)
end
end
def actor(actor)
case actor
when Array
actor.map(&method(:actor))
when Symbol # convert to flipper compatible object
stub_feature_flag_gate(actor)
else
actor
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment