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.

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

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)
+```

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)

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

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

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

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 }