Commit 19b4e263 authored by Kamil Trzciński's avatar Kamil Trzciński

Use a memoized Flipper engine in tests

This makes us to use a memoized Flipper engine
that simply always works.

We also change a implementation of `persisted_names`
to use a `Flipper` instead of requesting features
manually.

By default a memoized engine is used, which
can be overwritten with `stub_feature_flags: false`.

The memoized engine requires us to use object that
are `FeatureGate-inherited`. We ensure
that this holds true, always.

This MR also updates all tests to adhere to this requirement.
parent f619fc5b
...@@ -23,6 +23,9 @@ class Foo < ActiveRecord::Base ...@@ -23,6 +23,9 @@ class Foo < ActiveRecord::Base
end 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 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 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 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. ...@@ -119,17 +122,30 @@ how to access feature flags in a Vue component.
### Specs ### Specs
In the test environment `Feature.enabled?` is stubbed to always respond to `true`, Our Flipper engine in the test environment works in a memory mode `Flipper::Adapters::Memory`.
so we make sure behavior under feature flag doesn't go untested in some non-specific `production` and `development` modes use `Flipper::Adapters::ActiveRecord`.
contexts.
### `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 Make sure behavior under feature flag doesn't go untested in some non-specific contexts.
feature flag.
See the See the
[testing guide](../testing_guide/best_practices.md#feature-flags-in-tests) [testing guide](../testing_guide/best_practices.md#feature-flags-in-tests)
for information and examples on how to stub 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) ### Enabling a feature flag (in development)
In the rails console (`rails c`), enter the following command to enable your feature flag 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: ...@@ -143,3 +159,9 @@ Similarly, the following command will disable a feature flag:
```ruby ```ruby
Feature.disable(:feature_flag_name) 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 ...@@ -357,6 +357,60 @@ Feature.enabled?(:my_feature2) # => false
Feature.enabled?(:my_feature2, project1) # => true 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 ### Pristine test environments
The code exercised by a single GitLab test may access and modify many items of The code exercised by a single GitLab test may access and modify many items of
......
...@@ -15,19 +15,8 @@ module EE ...@@ -15,19 +15,8 @@ module EE
def experiment_enabled_for_user? def experiment_enabled_for_user?
return true unless current_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) ::Feature.enabled?(EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME, current_user, default_enabled: true)
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)
end end
end end
end end
...@@ -15,19 +15,8 @@ module EE ...@@ -15,19 +15,8 @@ module EE
private private
def experiment_enabled_for_session? def experiment_enabled_for_session?
# If EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME is not set, we should show ::Feature.enabled?(EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME, flipper_session,
# reCAPTCHA on the sign_up page default_enabled: true)
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)
end end
end end
end end
...@@ -49,7 +49,7 @@ module Gitlab ...@@ -49,7 +49,7 @@ module Gitlab
end end
def use_vulnerability_cache? def use_vulnerability_cache?
Feature.enabled?(:cache_vulnerability_summary, vulnerable) && !dynamic_filters_included? Feature.enabled?(:cache_vulnerability_summary) && !dynamic_filters_included?
end end
def dynamic_filters_included? def dynamic_filters_included?
......
...@@ -55,10 +55,9 @@ describe OnboardingExperimentHelper, type: :helper do ...@@ -55,10 +55,9 @@ describe OnboardingExperimentHelper, type: :helper do
context 'and experiment_growth_onboarding has been set' do context 'and experiment_growth_onboarding has been set' do
it 'checks if feature is enabled for current_user' do it 'checks if feature is enabled for current_user' do
percentage = ::Feature.flipper.actors(50) feature = Feature.get(described_class::EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME)
::Feature.flipper[described_class::EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME].enable(percentage) 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) expect(helper.allow_access_to_onboarding?).to eq(true)
end end
end end
......
...@@ -5,6 +5,12 @@ require 'spec_helper' ...@@ -5,6 +5,12 @@ require 'spec_helper'
describe RecaptchaExperimentHelper, type: :helper do describe RecaptchaExperimentHelper, type: :helper do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
let(:session) { {} }
before do
allow(helper).to receive(:session) { session }
end
describe '.show_recaptcha_sign_up?' do describe '.show_recaptcha_sign_up?' do
context 'when reCAPTCHA is disabled' do context 'when reCAPTCHA is disabled' do
it 'returns false' do it 'returns false' do
...@@ -28,18 +34,6 @@ describe RecaptchaExperimentHelper, type: :helper do ...@@ -28,18 +34,6 @@ describe RecaptchaExperimentHelper, type: :helper do
context 'and experiment_growth_recaptcha has been set' do context 'and experiment_growth_recaptcha has been set' do
let(:feature_name) { described_class::EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME } 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 where(:flipper_session_id, :expected_result) do
'00687625-667c-480c-ae2a-9bf861ddd909' | true '00687625-667c-480c-ae2a-9bf861ddd909' | true
'b8b78156-f7b8-4bf4-b906-06a899b84ea3' | false 'b8b78156-f7b8-4bf4-b906-06a899b84ea3' | false
...@@ -48,8 +42,12 @@ describe RecaptchaExperimentHelper, type: :helper do ...@@ -48,8 +42,12 @@ describe RecaptchaExperimentHelper, type: :helper do
end end
with_them do 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 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)) allow(helper).to receive(:flipper_session).and_return(FlipperSession.new(flipper_session_id))
expect(helper.show_recaptcha_sign_up?).to eq(expected_result) expect(helper.show_recaptcha_sign_up?).to eq(expected_result)
......
...@@ -14,12 +14,16 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgres ...@@ -14,12 +14,16 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgres
end end
context 'when there is no more MigrateApproverToApprovalRulesInBatch jobs' do context 'when there is no more MigrateApproverToApprovalRulesInBatch jobs' do
before do
stub_feature_flags(approval_rule: false)
end
it 'enables feature' do it 'enables feature' do
allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(false) allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(false)
expect(Feature).to receive(:enable).with(:approval_rule)
described_class.new.perform described_class.new.perform
expect(Feature.enabled?(:approval_rule)).to eq(true)
end end
end end
end end
...@@ -36,7 +36,7 @@ describe Gitlab::ImportExport::Project::TreeSaver do ...@@ -36,7 +36,7 @@ describe Gitlab::ImportExport::Project::TreeSaver do
before_all do before_all do
RSpec::Mocks.with_temporary_scope 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) stub_feature_flags(project_export_as_ndjson: ndjson_enabled)
project.add_maintainer(user) project.add_maintainer(user)
......
...@@ -142,6 +142,7 @@ describe ApplicationSetting do ...@@ -142,6 +142,7 @@ describe ApplicationSetting do
# and we want to make sure we're still testing # and we want to make sure we're still testing
# should_check_namespace_plan? method through the test-suite (see # should_check_namespace_plan? method through the test-suite (see
# https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/18461#note_69322821). # 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) allow(Rails).to receive_message_chain(:env, :test?).and_return(false)
end end
......
...@@ -18,6 +18,8 @@ class Feature ...@@ -18,6 +18,8 @@ class Feature
superclass.table_name = 'feature_gates' superclass.table_name = 'feature_gates'
end end
InvalidFeatureFlagError = Class.new(Exception) # rubocop:disable Lint/InheritException
class << self class << self
delegate :group, to: :flipper delegate :group, to: :flipper
...@@ -32,26 +34,47 @@ class Feature ...@@ -32,26 +34,47 @@ class Feature
def persisted_names def persisted_names
return [] unless Gitlab::Database.exists? return [] unless Gitlab::Database.exists?
Gitlab::SafeRequestStore[:flipper_persisted_names] ||= if Gitlab::Utils.to_boolean(ENV['FF_LEGACY_PERSISTED_NAMES'])
begin # To be removed:
# We saw on GitLab.com, this database request was called 2300 # This uses a legacy persisted names that are know to work (always)
# times/s. Let's cache it for a minute to avoid that load. Gitlab::SafeRequestStore[:flipper_persisted_names] ||=
Gitlab::ProcessMemoryCache.cache_backend.fetch('flipper:persisted_names', expires_in: 1.minute) do begin
FlipperFeature.feature_names # 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
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 end
def persisted?(feature) def persisted_name?(feature_name)
# Flipper creates on-memory features when asked for a not-yet-created one. # 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 # If we want to check if a feature has been actually set, we look for it
# on the persisted features list. # on the persisted features list.
persisted_names.include?(feature.name.to_s) persisted_names.include?(feature_name.to_s)
end end
# use `default_enabled: true` to default the flag to being `enabled` # use `default_enabled: true` to default the flag to being `enabled`
# unless set explicitly. The default is `disabled` # 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) 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 # During setup the database does not exist yet. So we haven't stored a value
# for the feature yet and return the default. # for the feature yet and return the default.
return default_enabled unless Gitlab::Database.exists? return default_enabled unless Gitlab::Database.exists?
...@@ -62,7 +85,7 @@ class Feature ...@@ -62,7 +85,7 @@ class Feature
# `persisted?` can potentially generate DB queries and also checks for inclusion # `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. # 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` # 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 end
def disabled?(key, thing = nil, default_enabled: false) def disabled?(key, thing = nil, default_enabled: false)
...@@ -87,10 +110,9 @@ class Feature ...@@ -87,10 +110,9 @@ class Feature
end end
def remove(key) def remove(key)
feature = get(key) return unless persisted_name?(key)
return unless persisted?(feature)
feature.remove get(key).remove
end end
def flipper def flipper
...@@ -101,17 +123,12 @@ class Feature ...@@ -101,17 +123,12 @@ class Feature
end end
end end
def build_flipper_instance def reset
Flipper.new(flipper_adapter).tap { |flip| flip.memoize = true } Gitlab::SafeRequestStore.delete(:flipper) if Gitlab::SafeRequestStore.active?
@flipper = nil
end end
# This method is called from config/initializers/flipper.rb and can be used def build_flipper_instance
# to register Flipper groups.
# See https://docs.gitlab.com/ee/development/feature_flags.html#feature-groups
def register_feature_groups
end
def flipper_adapter
active_record_adapter = Flipper::Adapters::ActiveRecord.new( active_record_adapter = Flipper::Adapters::ActiveRecord.new(
feature_class: FlipperFeature, feature_class: FlipperFeature,
gate_class: FlipperGate) gate_class: FlipperGate)
...@@ -125,10 +142,26 @@ class Feature ...@@ -125,10 +142,26 @@ class Feature
# Thread-local L1 cache: use a short timeout since we don't have a # Thread-local L1 cache: use a short timeout since we don't have a
# way to expire this cache all at once # way to expire this cache all at once
Flipper::Adapters::ActiveSupportCacheStore.new( flipper_adapter = Flipper::Adapters::ActiveSupportCacheStore.new(
redis_cache_adapter, redis_cache_adapter,
l1_cache_backend, l1_cache_backend,
expires_in: 1.minute) 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 end
def l1_cache_backend def l1_cache_backend
......
...@@ -5,8 +5,7 @@ module Gitlab ...@@ -5,8 +5,7 @@ module Gitlab
module RuggedImpl module RuggedImpl
module UseRugged module UseRugged
def use_rugged?(repo, feature_key) def use_rugged?(repo, feature_key)
feature = Feature.get(feature_key) return Feature.enabled?(feature_key) if Feature.persisted_name?(feature_key)
return feature.enabled? if Feature.persisted?(feature)
# Disable Rugged auto-detect(can_use_disk?) when Puma threads>1 # Disable Rugged auto-detect(can_use_disk?) when Puma threads>1
# https://gitlab.com/gitlab-org/gitlab/issues/119326 # https://gitlab.com/gitlab-org/gitlab/issues/119326
......
...@@ -25,7 +25,7 @@ describe SourcegraphDecorator do ...@@ -25,7 +25,7 @@ describe SourcegraphDecorator do
end end
before do 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) 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 ...@@ -135,25 +135,6 @@ describe Types::BaseField do
it 'returns true if the feature is enabled' do it 'returns true if the feature is enabled' do
expect(field.visible?(context)).to eq(true) expect(field.visible?(context)).to eq(true)
end 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 end
end end
......
...@@ -3,6 +3,12 @@ ...@@ -3,6 +3,12 @@
require 'spec_helper' require 'spec_helper'
describe RecaptchaExperimentHelper, type: :helper do describe RecaptchaExperimentHelper, type: :helper do
let(:session) { {} }
before do
allow(helper).to receive(:session) { session }
end
describe '.show_recaptcha_sign_up?' do describe '.show_recaptcha_sign_up?' do
context 'when reCAPTCHA is disabled' do context 'when reCAPTCHA is disabled' do
it 'returns false' do it 'returns false' do
......
...@@ -3,12 +3,14 @@ ...@@ -3,12 +3,14 @@
require 'spec_helper' require 'spec_helper'
describe Banzai::Pipeline::DescriptionPipeline do describe Banzai::Pipeline::DescriptionPipeline do
let_it_be(:project) { create(:project) }
def parse(html) def parse(html)
# When we pass HTML to Redcarpet, it gets wrapped in `p` tags... # When we pass HTML to Redcarpet, it gets wrapped in `p` tags...
# ...except when we pass it pre-wrapped text. Rabble rabble. # ...except when we pass it pre-wrapped text. Rabble rabble.
unwrap = !html.start_with?('<p ') 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 output.gsub!(%r{\A<p dir="auto">(.*)</p>(.*)\z}, '\1\2') if unwrap
......
...@@ -3,6 +3,11 @@ ...@@ -3,6 +3,11 @@
require 'spec_helper' require 'spec_helper'
describe Banzai::Pipeline::WikiPipeline do 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 describe 'TableOfContents' do
it 'replaces the tag with the TableOfContentsFilter result' do it 'replaces the tag with the TableOfContentsFilter result' do
markdown = <<-MD.strip_heredoc markdown = <<-MD.strip_heredoc
...@@ -13,7 +18,7 @@ describe Banzai::Pipeline::WikiPipeline do ...@@ -13,7 +18,7 @@ describe Banzai::Pipeline::WikiPipeline do
Foo Foo
MD MD
result = described_class.call(markdown, project: spy, wiki: spy) result = described_class.call(markdown, project: project, wiki: wiki)
aggregate_failures do aggregate_failures do
expect(result[:output].text).not_to include '[[' expect(result[:output].text).not_to include '[['
...@@ -31,7 +36,7 @@ describe Banzai::Pipeline::WikiPipeline do ...@@ -31,7 +36,7 @@ describe Banzai::Pipeline::WikiPipeline do
Foo Foo
MD 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>]]') expect(output).to include('[[<em>toc</em>]]')
end end
...@@ -44,7 +49,7 @@ describe Banzai::Pipeline::WikiPipeline do ...@@ -44,7 +49,7 @@ describe Banzai::Pipeline::WikiPipeline do
Foo Foo
MD MD
output = described_class.to_html(markdown, project: spy, wiki: spy) output = described_class.to_html(markdown, project: project, wiki: wiki)
aggregate_failures do aggregate_failures do
expect(output).not_to include('<ul>') expect(output).not_to include('<ul>')
...@@ -54,11 +59,6 @@ describe Banzai::Pipeline::WikiPipeline do ...@@ -54,11 +59,6 @@ describe Banzai::Pipeline::WikiPipeline do
end end
describe "Links" do 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 root URL' => '',
'when GitLab is hosted at a relative URL' => '/nested/relative/gitlab' }.each do |test_name, relative_url_root| 'when GitLab is hosted at a relative URL' => '/nested/relative/gitlab' }.each do |test_name, relative_url_root|
context test_name do context test_name do
...@@ -261,11 +261,6 @@ describe Banzai::Pipeline::WikiPipeline do ...@@ -261,11 +261,6 @@ describe Banzai::Pipeline::WikiPipeline do
end end
describe 'videos and audio' do 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 it 'generates video html structure' do
markdown = "![video_file](video_file_name.mp4)" markdown = "![video_file](video_file_name.mp4)"
output = described_class.to_html(markdown, project: project, wiki: wiki, page_slug: page.slug) output = described_class.to_html(markdown, project: project, wiki: wiki, page_slug: page.slug)
......
...@@ -5,9 +5,12 @@ require 'spec_helper' ...@@ -5,9 +5,12 @@ require 'spec_helper'
describe Constraints::FeatureConstrainer do describe Constraints::FeatureConstrainer do
describe '#matches' do describe '#matches' do
it 'calls Feature.enabled? with the correct arguments' 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 end
end end
...@@ -25,7 +25,7 @@ describe Feature::Gitaly do ...@@ -25,7 +25,7 @@ describe Feature::Gitaly do
describe ".server_feature_flags" do describe ".server_feature_flags" do
before 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 end
subject { described_class.server_feature_flags } subject { described_class.server_feature_flags }
......
...@@ -2,12 +2,10 @@ ...@@ -2,12 +2,10 @@
require 'spec_helper' require 'spec_helper'
describe Feature do describe Feature, stub_feature_flags: false do
before do before do
# We mock all calls to .enabled? to return true in order to force all # reset Flipper AR-engine
# specs to run the feature flag gated behavior, but here we need a clean Feature.reset
# behavior from the class
allow(described_class).to receive(:enabled?).and_call_original
end end
describe '.get' do describe '.get' do
...@@ -23,67 +21,106 @@ describe Feature do ...@@ -23,67 +21,106 @@ describe Feature do
end end
describe '.persisted_names' do describe '.persisted_names' do
it 'returns the names of the persisted features' do context 'when FF_LEGACY_PERSISTED_NAMES=false' do
Feature::FlipperFeature.create!(key: 'foo') before do
stub_env('FF_LEGACY_PERSISTED_NAMES', 'false')
end
expect(described_class.persisted_names).to eq(%w[foo]) it 'returns the names of the persisted features' do
end 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(Gitlab::ProcessMemoryCache.cache_backend)
expect(described_class.persisted_names).to be_empty .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 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 :request_store, :use_clean_rails_memory_store_caching do
Feature::FlipperFeature.create!(key: 'foo') Feature.enable('foo')
expect(Feature::FlipperFeature) expect(Gitlab::ProcessMemoryCache.cache_backend)
.to receive(:feature_names) .to receive(:fetch)
.once .once
.and_call_original .with('flipper:persisted_names', expires_in: 1.minute)
.and_call_original
expect(Gitlab::ProcessMemoryCache.cache_backend) 2.times do
.to receive(:fetch) expect(described_class.persisted_names).to contain_exactly('foo')
.once end
.with('flipper:persisted_names', expires_in: 1.minute) end
.and_call_original end
2.times do it 'fetches all flags once in a single query', :request_store do
expect(described_class.persisted_names).to eq(%w[foo]) 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 end
expect(queries.count).to eq(1)
end end
end end
describe '.persisted?' do describe '.persisted_name?' do
context 'when the feature is persisted' do context 'when the feature is persisted' do
it 'returns true when feature name is a string' do it 'returns true when feature name is a string' 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
it 'returns true when feature name is a symbol' do 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_name?(:foo)).to eq(true)
expect(described_class.persisted?(feature)).to eq(true)
end end
end end
context 'when the feature is not persisted' do context 'when the feature is not persisted' do
it 'returns false when feature name is a string' do it 'returns false when feature name is a string' do
feature = double(:feature, name: 'foo') expect(described_class.persisted_name?('foo')).to eq(false)
expect(described_class.persisted?(feature)).to eq(false)
end end
it 'returns false when feature name is a symbol' do it 'returns false when feature name is a symbol' do
feature = double(:feature, name: :bar) expect(described_class.persisted_name?(:bar)).to eq(false)
expect(described_class.persisted?(feature)).to eq(false)
end end
end end
end end
...@@ -100,10 +137,6 @@ describe Feature do ...@@ -100,10 +137,6 @@ describe Feature do
end end
describe '.flipper' do describe '.flipper' do
before do
described_class.instance_variable_set(:@flipper, nil)
end
context 'when request store is inactive' do context 'when request store is inactive' do
it 'memoizes the Flipper instance' do it 'memoizes the Flipper instance' do
expect(Flipper).to receive(:new).once.and_call_original expect(Flipper).to receive(:new).once.and_call_original
...@@ -216,10 +249,8 @@ describe Feature do ...@@ -216,10 +249,8 @@ describe Feature do
end end
context 'with an individual actor' do context 'with an individual actor' do
CustomActor = Struct.new(:flipper_id) let(:actor) { stub_feature_flag_gate('CustomActor:5') }
let(:another_actor) { stub_feature_flag_gate('CustomActor:10') }
let(:actor) { CustomActor.new(flipper_id: 'CustomActor:5') }
let(:another_actor) { CustomActor.new(flipper_id: 'CustomActor:10') }
before do before do
described_class.enable(:enabled_feature_flag, actor) described_class.enable(:enabled_feature_flag, actor)
...@@ -237,6 +268,17 @@ describe Feature do ...@@ -237,6 +268,17 @@ describe Feature do
expect(described_class.enabled?(:enabled_feature_flag)).to be_falsey expect(described_class.enabled?(:enabled_feature_flag)).to be_falsey
end end
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 end
describe '.disable?' do describe '.disable?' do
......
...@@ -54,6 +54,10 @@ describe Gitlab::BackgroundMigration::MergeRequestAssigneesMigrationProgressChec ...@@ -54,6 +54,10 @@ describe Gitlab::BackgroundMigration::MergeRequestAssigneesMigrationProgressChec
end end
context 'when there are no scheduled, or retrying or dead' do 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 it 'enables feature' do
allow(Gitlab::BackgroundMigration).to receive(:exists?) allow(Gitlab::BackgroundMigration).to receive(:exists?)
.with('PopulateMergeRequestAssigneesTable') .with('PopulateMergeRequestAssigneesTable')
...@@ -67,9 +71,9 @@ describe Gitlab::BackgroundMigration::MergeRequestAssigneesMigrationProgressChec ...@@ -67,9 +71,9 @@ describe Gitlab::BackgroundMigration::MergeRequestAssigneesMigrationProgressChec
.with('PopulateMergeRequestAssigneesTable') .with('PopulateMergeRequestAssigneesTable')
.and_return(false) .and_return(false)
expect(Feature).to receive(:enable).with(:multiple_merge_request_assignees)
described_class.new.perform described_class.new.perform
expect(Feature.enabled?(:multiple_merge_request_assignees)).to eq(true)
end end
end end
......
...@@ -5,19 +5,13 @@ require 'spec_helper' ...@@ -5,19 +5,13 @@ require 'spec_helper'
describe Gitlab::Chat, :use_clean_rails_memory_store_caching do describe Gitlab::Chat, :use_clean_rails_memory_store_caching do
describe '.available?' do describe '.available?' do
it 'returns true when the chatops feature is available' do it 'returns true when the chatops feature is available' do
allow(Feature) stub_feature_flags(chatops: true)
.to receive(:enabled?)
.with(:chatops, default_enabled: true)
.and_return(true)
expect(described_class).to be_available expect(described_class).to be_available
end end
it 'returns false when the chatops feature is not available' do it 'returns false when the chatops feature is not available' do
allow(Feature) stub_feature_flags(chatops: false)
.to receive(:enabled?)
.with(:chatops, default_enabled: true)
.and_return(false)
expect(described_class).not_to be_available expect(described_class).not_to be_available
end end
......
...@@ -11,7 +11,7 @@ describe Gitlab::Experimentation do ...@@ -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 end
let(:environment) { Rails.env.test? } let(:environment) { Rails.env.test? }
......
...@@ -49,10 +49,6 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do ...@@ -49,10 +49,6 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
end end
context 'when feature flag is not persisted' do 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 context 'when running puma with multiple threads' do
before do before do
allow(subject).to receive(:running_puma_with_multiple_threads?).and_return(true) allow(subject).to receive(:running_puma_with_multiple_threads?).and_return(true)
...@@ -97,18 +93,15 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do ...@@ -97,18 +93,15 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
end end
context 'when feature flag is persisted' do 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 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 expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey
end end
it "returns true when feature flag is on" do 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) allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(false)
expect(subject.use_rugged?(repository, feature_flag_name)).to be true expect(subject.use_rugged?(repository, feature_flag_name)).to be true
......
...@@ -12,21 +12,19 @@ describe Gitlab::GonHelper do ...@@ -12,21 +12,19 @@ describe Gitlab::GonHelper do
describe '#push_frontend_feature_flag' do describe '#push_frontend_feature_flag' do
it 'pushes a feature flag to the frontend' do it 'pushes a feature flag to the frontend' do
gon = instance_double('gon') gon = instance_double('gon')
thing = stub_feature_flag_gate('thing')
stub_feature_flags(my_feature_flag: thing)
allow(helper) allow(helper)
.to receive(:gon) .to receive(:gon)
.and_return(gon) .and_return(gon)
expect(Feature)
.to receive(:enabled?)
.with(:my_feature_flag, 10)
.and_return(true)
expect(gon) expect(gon)
.to receive(:push) .to receive(:push)
.with({ features: { 'myFeatureFlag' => true } }, true) .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
end end
......
...@@ -26,7 +26,7 @@ describe Gitlab::ImportExport::Project::TreeRestorer do ...@@ -26,7 +26,7 @@ describe Gitlab::ImportExport::Project::TreeRestorer do
@project = create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project') @project = create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project')
@shared = @project.import_export_shared @shared = @project.import_export_shared
allow(Feature).to receive(:enabled?) { true } stub_all_feature_flags
stub_feature_flags(project_import_ndjson: ndjson_enabled) stub_feature_flags(project_import_ndjson: ndjson_enabled)
setup_import_export_config('complex') setup_import_export_config('complex')
......
...@@ -29,7 +29,7 @@ describe Gitlab::ImportExport::Project::TreeSaver do ...@@ -29,7 +29,7 @@ describe Gitlab::ImportExport::Project::TreeSaver do
before_all do before_all do
RSpec::Mocks.with_temporary_scope 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) stub_feature_flags(project_export_as_ndjson: ndjson_enabled)
project.add_maintainer(user) project.add_maintainer(user)
......
...@@ -7,7 +7,7 @@ describe Gitlab::Sourcegraph do ...@@ -7,7 +7,7 @@ describe Gitlab::Sourcegraph do
let(:feature_scope) { true } let(:feature_scope) { true }
before do before do
Feature.enable(:sourcegraph, feature_scope) stub_feature_flags(sourcegraph: feature_scope)
end end
describe '.feature_conditional?' do describe '.feature_conditional?' do
......
...@@ -27,14 +27,13 @@ describe Gitlab::Tracking do ...@@ -27,14 +27,13 @@ describe Gitlab::Tracking do
expect(subject.snowplow_options(nil)).to match(expected_fields) expect(subject.snowplow_options(nil)).to match(expected_fields)
end end
it 'enables features using feature flags' do it 'when feature flag is disabled' do
stub_feature_flags(additional_snowplow_tracking: :__group__) stub_feature_flags(additional_snowplow_tracking: false)
addition_feature_fields = {
expect(subject.snowplow_options(nil)).to include(
formTracking: false, formTracking: false,
linkClickTracking: false linkClickTracking: false
} )
expect(subject.snowplow_options(:_group_)).to include(addition_feature_fields)
end end
end end
......
...@@ -7,7 +7,7 @@ describe Gitlab::WikiPages::FrontMatterParser do ...@@ -7,7 +7,7 @@ describe Gitlab::WikiPages::FrontMatterParser do
let(:content) { 'This is the content' } let(:content) { 'This is the content' }
let(:end_divider) { '---' } let(:end_divider) { '---' }
let(:gate) { double('Gate') } let(:gate) { stub_feature_flag_gate('Gate') }
let(:with_front_matter) do let(:with_front_matter) do
<<~MD <<~MD
......
...@@ -4261,7 +4261,6 @@ describe Project do ...@@ -4261,7 +4261,6 @@ describe Project do
describe '#auto_devops_enabled?' do describe '#auto_devops_enabled?' do
before do before do
allow(Feature).to receive(:enabled?).and_call_original
Feature.get(:force_autodevops_on_by_default).enable_percentage_of_actors(0) Feature.get(:force_autodevops_on_by_default).enable_percentage_of_actors(0)
end end
...@@ -4464,7 +4463,6 @@ describe Project do ...@@ -4464,7 +4463,6 @@ describe Project do
let_it_be(:project, reload: true) { create(:project) } let_it_be(:project, reload: true) { create(:project) }
before do before do
allow(Feature).to receive(:enabled?).and_call_original
Feature.get(:force_autodevops_on_by_default).enable_percentage_of_actors(0) Feature.get(:force_autodevops_on_by_default).enable_percentage_of_actors(0)
end end
......
...@@ -2,11 +2,12 @@ ...@@ -2,11 +2,12 @@
require 'spec_helper' require 'spec_helper'
describe API::Features do describe API::Features, stub_feature_flags: false do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:admin) { create(:admin) } let_it_be(:admin) { create(:admin) }
before do before do
Feature.reset
Flipper.unregister_groups Flipper.unregister_groups
Flipper.register(:perf_team) do |actor| Flipper.register(:perf_team) do |actor|
actor.respond_to?(:admin) && actor.admin? actor.respond_to?(:admin) && actor.admin?
......
...@@ -134,7 +134,8 @@ describe API::Files do ...@@ -134,7 +134,8 @@ describe API::Files do
context 'when PATs are used' do context 'when PATs are used' do
it_behaves_like 'repository files' do it_behaves_like 'repository files' do
let(:token) { create(:personal_access_token, scopes: ['read_repository'], user: user) } 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
end end
...@@ -153,15 +154,17 @@ describe API::Files do ...@@ -153,15 +154,17 @@ describe API::Files do
describe "GET /projects/:id/repository/files/:file_path" do describe "GET /projects/:id/repository/files/:file_path" do
shared_examples_for 'repository files' do shared_examples_for 'repository files' do
let(:api_user) { current_user }
it 'returns 400 for invalid file path' do 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(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq(invalid_file_message) expect(json_response['error']).to eq(invalid_file_message)
end end
it 'returns file attributes as json' do 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(response).to have_gitlab_http_status(:ok)
expect(json_response['file_path']).to eq(CGI.unescape(file_path)) expect(json_response['file_path']).to eq(CGI.unescape(file_path))
...@@ -174,7 +177,7 @@ describe API::Files do ...@@ -174,7 +177,7 @@ describe API::Files do
it 'returns json when file has txt extension' do it 'returns json when file has txt extension' do
file_path = "bar%2Fbranch-test.txt" 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).to have_gitlab_http_status(:ok)
expect(response.content_type).to eq('application/json') expect(response.content_type).to eq('application/json')
...@@ -185,7 +188,7 @@ describe API::Files do ...@@ -185,7 +188,7 @@ describe API::Files do
file_path = "files%2Fjs%2Fcommit%2Ejs%2Ecoffee" file_path = "files%2Fjs%2Fcommit%2Ejs%2Ecoffee"
params[:ref] = "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9" 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(response).to have_gitlab_http_status(:ok)
expect(json_response['file_name']).to eq('commit.js.coffee') expect(json_response['file_name']).to eq('commit.js.coffee')
...@@ -197,7 +200,7 @@ describe API::Files do ...@@ -197,7 +200,7 @@ describe API::Files do
url = route(file_path) + "/raw" url = route(file_path) + "/raw"
expect(Gitlab::Workhorse).to receive(:send_git_blob) 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(response).to have_gitlab_http_status(:ok)
expect(headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" expect(headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
...@@ -206,7 +209,7 @@ describe API::Files do ...@@ -206,7 +209,7 @@ describe API::Files do
it 'returns blame file info' do it 'returns blame file info' do
url = route(file_path) + '/blame' 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) expect(response).to have_gitlab_http_status(:ok)
end end
...@@ -214,7 +217,7 @@ describe API::Files do ...@@ -214,7 +217,7 @@ describe API::Files do
it 'sets inline content disposition by default' do it 'sets inline content disposition by default' do
url = route(file_path) + "/raw" 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)) expect(headers['Content-Disposition']).to eq(%q(inline; filename="popen.rb"; filename*=UTF-8''popen.rb))
end end
...@@ -229,7 +232,7 @@ describe API::Files do ...@@ -229,7 +232,7 @@ describe API::Files do
let(:params) { { ref: 'master' } } let(:params) { { ref: 'master' } }
it_behaves_like '404 response' do 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' } let(:message) { '404 File Not Found' }
end end
end end
...@@ -238,7 +241,7 @@ describe API::Files do ...@@ -238,7 +241,7 @@ describe API::Files do
include_context 'disabled repository' include_context 'disabled repository'
it_behaves_like '403 response' do 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 end
end end
...@@ -253,7 +256,8 @@ describe API::Files do ...@@ -253,7 +256,8 @@ describe API::Files do
context 'when PATs are used' do context 'when PATs are used' do
it_behaves_like 'repository files' do it_behaves_like 'repository files' do
let(:token) { create(:personal_access_token, scopes: ['read_repository'], user: user) } 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
end end
......
...@@ -396,7 +396,7 @@ describe API::Internal::Base do ...@@ -396,7 +396,7 @@ describe API::Internal::Base do
context "git pull" do context "git pull" do
before do before do
allow(Feature).to receive(:persisted_names).and_return(%w[gitaly_mep_mep]) stub_feature_flags(gitaly_mep_mep: true)
end end
it "has the correct payload" do it "has the correct payload" do
......
...@@ -284,7 +284,7 @@ describe API::Repositories do ...@@ -284,7 +284,7 @@ describe API::Repositories do
context "when hotlinking detection is enabled" do context "when hotlinking detection is enabled" do
before do before do
Feature.enable(:repository_archive_hotlinking_interception) stub_feature_flags(repository_archive_hotlinking_interception: true)
end end
it_behaves_like "hotlink interceptor" do it_behaves_like "hotlink interceptor" do
......
...@@ -44,7 +44,7 @@ describe Namespaces::CheckStorageSizeService, '#execute' do ...@@ -44,7 +44,7 @@ describe Namespaces::CheckStorageSizeService, '#execute' do
end end
it 'errors when feature flag is activated for the current namespace' do 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).to be_error
expect(response.message).to be_present expect(response.message).to be_present
......
...@@ -172,37 +172,35 @@ RSpec.configure do |config| ...@@ -172,37 +172,35 @@ RSpec.configure do |config|
end end
config.before do |example| config.before do |example|
# Enable all features by default for testing if example.metadata.fetch(:stub_feature_flags, true)
allow(Feature).to receive(:enabled?) { true } # Enable all features by default for testing
stub_all_feature_flags
enable_rugged = example.metadata[:enable_rugged].present?
# 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 allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(enable_rugged)
Gitlab::Git::RuggedImpl::Repository::FEATURE_FLAGS.each do |flag|
stub_feature_flags(flag => enable_rugged)
end 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. # Enable Marginalia feature for all specs in the test suite.
allow(Gitlab::Marginalia).to receive(:cached_feature_enabled?).and_return(true) 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 # Stub these calls due to being expensive operations
# It can be reenabled for specific tests via: # It can be reenabled for specific tests via:
# #
......
# frozen_string_literal: true # frozen_string_literal: true
module StubFeatureFlags 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` # Stub Feature flags with `flag_name: true/false`
# #
# @param [Hash] features where key is feature name and value is boolean whether enabled or not. # @param [Hash] features where key is feature name and value is boolean whether enabled or not.
...@@ -14,23 +46,29 @@ module StubFeatureFlags ...@@ -14,23 +46,29 @@ module StubFeatureFlags
# Enable `ci_live_trace` feature flag only on the specified projects. # Enable `ci_live_trace` feature flag only on the specified projects.
def stub_feature_flags(features) def stub_feature_flags(features)
features.each do |feature_name, actors| features.each do |feature_name, actors|
allow(Feature).to receive(:enabled?).with(feature_name, any_args).and_return(false) # Remove feature flag overwrite
allow(Feature).to receive(:enabled?).with(feature_name.to_s, any_args).and_return(false) feature = Feature.get(feature_name)
feature.remove
Array(actors).each do |actor| Array(actors).each do |actor|
raise ArgumentError, "actor cannot be Hash" if actor.is_a?(Hash) raise ArgumentError, "actor cannot be Hash" if actor.is_a?(Hash)
case actor # Control a state of feature flag
when false, true if actor == true || actor.nil? || actor.respond_to?(:flipper_id)
allow(Feature).to receive(:enabled?).with(feature_name, any_args).and_return(actor) feature.enable(actor)
allow(Feature).to receive(:enabled?).with(feature_name.to_s, any_args).and_return(actor) elsif actor == false
when nil, ActiveRecord::Base, Symbol, RSpec::Mocks::Double feature.disable
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)
else 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
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 end
...@@ -3,16 +3,7 @@ ...@@ -3,16 +3,7 @@
require 'spec_helper' require 'spec_helper'
describe StubFeatureFlags do describe StubFeatureFlags do
before do let(:feature_name) { :test_feature }
# 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
describe '#stub_feature_flags' do describe '#stub_feature_flags' do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
...@@ -30,7 +21,7 @@ describe StubFeatureFlags do ...@@ -30,7 +21,7 @@ describe StubFeatureFlags do
with_them do with_them do
before do before do
stub_feature_flags(feature_name => feature_actors) stub_feature_flags(feature_name => actor(feature_actors))
end end
it { expect(Feature.enabled?(feature_name)).to eq(expected_result) } it { expect(Feature.enabled?(feature_name)).to eq(expected_result) }
...@@ -62,15 +53,15 @@ describe StubFeatureFlags do ...@@ -62,15 +53,15 @@ describe StubFeatureFlags do
with_them do with_them do
before do before do
stub_feature_flags(feature_name => feature_actors) stub_feature_flags(feature_name => actor(feature_actors))
end end
it { expect(Feature.enabled?(feature_name, tested_actor)).to eq(expected_result) } it { expect(Feature.enabled?(feature_name, actor(tested_actor))).to eq(expected_result) }
it { expect(Feature.disabled?(feature_name, tested_actor)).not_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 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.enabled?(feature_name, actor(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.disabled?(feature_name, actor(tested_actor), default_enabled: true)).not_to eq(expected_result) }
end end
end end
end end
...@@ -82,7 +73,7 @@ describe StubFeatureFlags do ...@@ -82,7 +73,7 @@ describe StubFeatureFlags do
end end
with_them do 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/) } it { expect { subject }.to raise_error(ArgumentError, /accepts only/) }
end end
...@@ -90,11 +81,11 @@ describe StubFeatureFlags do ...@@ -90,11 +81,11 @@ describe StubFeatureFlags do
context 'does not raise error' do context 'does not raise error' do
where(:feature_actors) do where(:feature_actors) do
[true, false, nil, :symbol, double, User.new] [true, false, nil, stub_feature_flag_gate(100), User.new]
end end
with_them do 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 } it { expect { subject }.not_to raise_error }
end end
...@@ -103,28 +94,39 @@ describe StubFeatureFlags do ...@@ -103,28 +94,39 @@ describe StubFeatureFlags do
it 'subsquent run changes state' do it 'subsquent run changes state' do
# enable FF only on A # enable FF only on A
stub_feature_flags(test_feature: %i[A]) stub_feature_flags({ feature_name => actor(%i[A]) })
expect(Feature.enabled?(:test_feature)).to eq(false) expect(Feature.enabled?(feature_name)).to eq(false)
expect(Feature.enabled?(:test_feature, :A)).to eq(true) expect(Feature.enabled?(feature_name, actor(:A))).to eq(true)
expect(Feature.enabled?(:test_feature, :B)).to eq(false) expect(Feature.enabled?(feature_name, actor(:B))).to eq(false)
# enable FF only on B # enable FF only on B
stub_feature_flags(test_feature: %i[B]) stub_feature_flags({ feature_name => actor(%i[B]) })
expect(Feature.enabled?(:test_feature)).to eq(false) expect(Feature.enabled?(feature_name)).to eq(false)
expect(Feature.enabled?(:test_feature, :A)).to eq(false) expect(Feature.enabled?(feature_name, actor(:A))).to eq(false)
expect(Feature.enabled?(:test_feature, :B)).to eq(true) expect(Feature.enabled?(feature_name, actor(:B))).to eq(true)
# enable FF on all # enable FF on all
stub_feature_flags(test_feature: true) stub_feature_flags({ feature_name => true })
expect(Feature.enabled?(:test_feature)).to eq(true) expect(Feature.enabled?(feature_name)).to eq(true)
expect(Feature.enabled?(:test_feature, :A)).to eq(true) expect(Feature.enabled?(feature_name, actor(:A))).to eq(true)
expect(Feature.enabled?(:test_feature, :B)).to eq(true) expect(Feature.enabled?(feature_name, actor(:B))).to eq(true)
# disable FF on all # disable FF on all
stub_feature_flags(test_feature: false) stub_feature_flags({ feature_name => false })
expect(Feature.enabled?(:test_feature)).to eq(false) expect(Feature.enabled?(feature_name)).to eq(false)
expect(Feature.enabled?(:test_feature, :A)).to eq(false) expect(Feature.enabled?(feature_name, actor(:A))).to eq(false)
expect(Feature.enabled?(:test_feature, :B)).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 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