From 61c35b6ca1840507f5da3aa4d951e4273e612d66 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Mon, 20 Aug 2018 20:52:56 +0200 Subject: [PATCH] Fixed `stub_feature_flag behavior` for `disabled?` flags. Previous code would not work with `disabled?` because that method would send two parameters (second always `nil`) which we are not mocking. Instead of mock yet another state, I decide to fix it where it belongs. --- .../50345-hashed-storage-feature-flag.yml | 5 ++ doc/development/feature_flags.md | 12 ++++ lib/feature.rb | 3 +- spec/lib/feature_spec.rb | 66 +++++++++++++++++++ spec/models/project_spec.rb | 2 + spec/services/projects/update_service_spec.rb | 3 +- spec/support/helpers/stub_feature_flags.rb | 3 + 7 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/50345-hashed-storage-feature-flag.yml diff --git a/changelogs/unreleased/50345-hashed-storage-feature-flag.yml b/changelogs/unreleased/50345-hashed-storage-feature-flag.yml new file mode 100644 index 00000000000..4c5182b843b --- /dev/null +++ b/changelogs/unreleased/50345-hashed-storage-feature-flag.yml @@ -0,0 +1,5 @@ +--- +title: Feature flag to disable Hashed Storage migration when renaming a repository +merge_request: 21291 +author: +type: added diff --git a/doc/development/feature_flags.md b/doc/development/feature_flags.md index 09ea8c05be6..702caacc74f 100644 --- a/doc/development/feature_flags.md +++ b/doc/development/feature_flags.md @@ -57,3 +57,15 @@ end Features that are developed and are intended to be merged behind a feature flag should not include a changelog entry. The entry should be added in the merge request removing the feature flags. + +### 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. + +If you need to test the feature flag in a different state, you need to stub it with: + +```ruby +stub_feature_flags(my_feature_flag: false) +``` diff --git a/lib/feature.rb b/lib/feature.rb index 09c5ef3ad94..24dbcb32fc0 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -47,7 +47,8 @@ class Feature end def disabled?(key, thing = nil) - !enabled?(key, thing) + # we need to make different method calls to make it easy to mock / define expectations in test mode + thing.nil? ? !enabled?(key) : !enabled?(key, thing) end def enable(key, thing = true) diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index f313e675654..8bb5a843484 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -1,6 +1,13 @@ require 'spec_helper' describe Feature 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 + end + describe '.get' do let(:feature) { double(:feature) } let(:key) { 'my_feature' } @@ -106,4 +113,63 @@ describe Feature do it_behaves_like 'a memoized Flipper instance' end end + + describe '.enabled?' do + it 'returns false for undefined feature' do + expect(described_class.enabled?(:some_random_feature_flag)).to be_falsey + end + + it 'returns false for existing disabled feature in the database' do + described_class.disable(:disabled_feature_flag) + + expect(described_class.enabled?(:disabled_feature_flag)).to be_falsey + end + + it 'returns true for existing enabled feature in the database' do + described_class.enable(:enabled_feature_flag) + + expect(described_class.enabled?(:enabled_feature_flag)).to be_truthy + 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') } + + before do + described_class.enable(:enabled_feature_flag, actor) + end + + it 'returns true when same actor is informed' do + expect(described_class.enabled?(:enabled_feature_flag, actor)).to be_truthy + end + + it 'returns false when different actor is informed' do + expect(described_class.enabled?(:enabled_feature_flag, another_actor)).to be_falsey + end + + it 'returns false when no actor is informed' do + expect(described_class.enabled?(:enabled_feature_flag)).to be_falsey + end + end + end + + describe '.disable?' do + it 'returns true for undefined feature' do + expect(described_class.disabled?(:some_random_feature_flag)).to be_truthy + end + + it 'returns true for existing disabled feature in the database' do + described_class.disable(:disabled_feature_flag) + + expect(described_class.disabled?(:disabled_feature_flag)).to be_truthy + end + + it 'returns false for existing enabled feature in the database' do + described_class.enable(:enabled_feature_flag) + + expect(described_class.disabled?(:enabled_feature_flag)).to be_falsey + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 8cb706b54b0..6f82a83ddd2 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2989,6 +2989,7 @@ describe Project do # call. This makes testing a bit easier. allow(project).to receive(:gitlab_shell).and_return(gitlab_shell) allow(project).to receive(:previous_changes).and_return('path' => ['foo']) + stub_feature_flags(disable_hashed_storage_upgrade: false) end it 'renames a repository' do @@ -3160,6 +3161,7 @@ describe Project do # call. This makes testing a bit easier. allow(project).to receive(:gitlab_shell).and_return(gitlab_shell) allow(project).to receive(:previous_changes).and_return('path' => ['foo']) + stub_feature_flags(disable_hashed_storage_upgrade: false) end context 'migration to hashed storage' do diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 2409be80cec..bc8bda1000e 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -262,6 +262,7 @@ describe Projects::UpdateService do context 'when hashed storage is enabled' do before do stub_application_setting(hashed_storage_enabled: true) + stub_feature_flags(disable_hashed_storage_upgrade: false) end it 'migrates project to a hashed storage instead of renaming the repo to another legacy name' do @@ -275,7 +276,7 @@ describe Projects::UpdateService do context 'when disable_hashed_storage_upgrade feature flag is enabled' do before do - expect(Feature).to receive(:enabled?).with(:disable_hashed_storage_upgrade) { true } + stub_feature_flags(disable_hashed_storage_upgrade: true) end it 'renames the project without upgrading it' do diff --git a/spec/support/helpers/stub_feature_flags.rb b/spec/support/helpers/stub_feature_flags.rb index b96338bf548..c54a871b157 100644 --- a/spec/support/helpers/stub_feature_flags.rb +++ b/spec/support/helpers/stub_feature_flags.rb @@ -1,4 +1,7 @@ module StubFeatureFlags + # Stub Feature flags with `flag_name: true/false` + # + # @param [Hash] features where key is feature name and value is boolean whether enabled or not def stub_feature_flags(features) features.each do |feature_name, enabled| allow(Feature).to receive(:enabled?).with(feature_name) { enabled }