Automatic merge of gitlab-org/gitlab-ce master

changelogs/unreleased/sh-rugged-commit-tree-entry.yml

+---
+title: Bring back Rugged implementation of commit_tree_entry
+merge_request: 25896
+author:
+type: other

lib/gitlab/git/commit.rb

@@ -314,11 +314,16 @@ module Gitlab
       def tree_entry(path)
         return unless path.present?
 
+        commit_tree_entry(path)
+      end
+
+      def commit_tree_entry(path)
         # We're only interested in metadata, so limit actual data to 1 byte
         # since Gitaly doesn't support "send no data" option.
         entry = @repository.gitaly_commit_client.tree_entry(id, path, 1)
         return unless entry
 
+        # To be compatible with the rugged format
         entry = entry.to_h
         entry.delete(:data)
         entry[:name] = File.basename(path)

lib/gitlab/git/rugged_impl/commit.rb

@@ -43,6 +43,30 @@ module Gitlab
           end
         end
 
+        override :commit_tree_entry
+        def commit_tree_entry(path)
+          if Feature.enabled?(:rugged_commit_tree_entry)
+            rugged_tree_entry(path)
+          else
+            super
+          end
+        end
+
+        # Is this the same as Blob.find_entry_by_path ?
+        def rugged_tree_entry(path)
+          rugged_commit.tree.path(path)
+        rescue Rugged::TreeError
+          nil
+        end
+
+        def rugged_commit
+          @rugged_commit ||= if raw_commit.is_a?(Rugged::Commit)
+                               raw_commit
+                             else
+                               @repository.rev_parse_target(id)
+                             end
+        end
+
         def init_from_rugged(commit)
           author = commit.author
           committer = commit.committer

lib/gitlab/git/rugged_impl/repository.rb

@@ -12,7 +12,7 @@ module Gitlab
       module Repository
         extend ::Gitlab::Utils::Override
 
-        FEATURE_FLAGS = %i(rugged_find_commit rugged_tree_entries rugged_tree_entry rugged_commit_is_ancestor).freeze
+        FEATURE_FLAGS = %i(rugged_find_commit rugged_tree_entries rugged_tree_entry rugged_commit_is_ancestor rugged_commit_tree_entry).freeze
 
         def alternate_object_directories
           relative_object_directories.map { |d| File.join(path, d) }

qa/qa.rb

@@ -342,6 +342,10 @@ module QA
   module Specs
     autoload :Config, 'qa/specs/config'
     autoload :Runner, 'qa/specs/runner'
+
+    module Helpers
+      autoload :Quarantine, 'qa/specs/helpers/quarantine'
+    end
   end
 
   ##

qa/qa/specs/helpers/quarantine.rb

+# frozen_string_literal: true
+
+require 'rspec/core'
+
+module QA::Specs::Helpers
+  module Quarantine
+    include RSpec::Core::Pending
+
+    extend self
+
+    def configure_rspec
+      RSpec.configure do |config|
+        config.before(:context, :quarantine) do
+          Quarantine.skip_or_run_quarantined_contexts(config.inclusion_filter.rules, self.class)
+        end
+
+        config.before do |example|
+          Quarantine.skip_or_run_quarantined_tests_or_contexts(config.inclusion_filter.rules, example)
+        end
+      end
+    end
+
+    # Skip tests in quarantine unless we explicitly focus on them.
+    def skip_or_run_quarantined_tests_or_contexts(filters, example)
+      if filters.key?(:quarantine)
+        included_filters = filters_other_than_quarantine(filters)
+
+        # If :quarantine is focused, skip the test/context unless its metadata
+        # includes quarantine and any other filters
+        # E.g., Suppose a test is tagged :smoke and :quarantine, and another is tagged
+        # :ldap and :quarantine. If we wanted to run just quarantined smoke tests
+        # using `--tag quarantine --tag smoke`, without this check we'd end up
+        # running that ldap test as well because of the :quarantine metadata.
+        # We could use an exclusion filter, but this way the test report will list
+        # the quarantined tests when they're not run so that we're aware of them
+        skip("Only running tests tagged with :quarantine and any of #{included_filters.keys}") if should_skip_when_focused?(example.metadata, included_filters)
+      else
+        skip('In quarantine') if example.metadata.key?(:quarantine)
+      end
+    end
+
+    # Skip the entire context if a context is quarantined. This avoids running
+    # before blocks unnecessarily.
+    def skip_or_run_quarantined_contexts(filters, example)
+      return unless example.metadata.key?(:quarantine)
+
+      skip_or_run_quarantined_tests_or_contexts(filters, example)
+    end
+
+    def filters_other_than_quarantine(filter)
+      filter.reject { |key, _| key == :quarantine }
+    end
+
+    # Checks if a test or context should be skipped.
+    #
+    # Returns true if
+    # - the metadata does not includes the :quarantine tag
+    # or if
+    # - the metadata includes the :quarantine tag
+    # - and the filter includes other tags that aren't in the metadata
+    def should_skip_when_focused?(metadata, included_filters)
+      return true unless metadata.key?(:quarantine)
+      return false if included_filters.empty?
+
+      (metadata.keys & included_filters.keys).empty?
+    end
+  end
+end

qa/spec/spec_helper.rb

@@ -6,16 +6,10 @@ require 'rspec/retry'
 end
 
 RSpec.configure do |config|
-  config.before(:context) do
-    if self.class.metadata.keys.include?(:quarantine)
-      skip_or_run_quarantined_tests(self.class.metadata.keys, config.inclusion_filter.rules.keys)
-    end
-  end
+  QA::Specs::Helpers::Quarantine.configure_rspec
 
   config.before do |example|
     QA::Runtime::Logger.debug("Starting test: #{example.full_description}") if QA::Runtime::Env.debug?
-
-    skip_or_run_quarantined_tests(example.metadata.keys, config.inclusion_filter.rules.keys)
   end
 
   config.expect_with :rspec do |expectations|
@@ -44,42 +38,3 @@ RSpec.configure do |config|
     example.run_with_retry retry: retry_times
   end
 end
-
-# Skip tests in quarantine unless we explicitly focus on them.
-# Skip the entire context if a context is tagged. This avoids running before
-# blocks unnecessarily.
-# If quarantine is focussed, skip tests/contexts that have other metadata
-# unless they're also focussed. This lets us run quarantined tests in a
-# particular category without running tests in other categories.
-# E.g., if a test is tagged 'smoke' and 'quarantine', and another is tagged
-# 'ldap' and 'quarantine', if we wanted to run just quarantined smoke tests
-# using `--tag quarantine --tag smoke`, without this check we'd end up
-# running that ldap test as well.
-# We could use an exclusion filter, but this way the test report will list
-# the quarantined tests when they're not run so that we're aware of them
-def skip_or_run_quarantined_tests(metadata_keys, filter_keys)
-  included_filters = filters_other_than_quarantine(filter_keys)
-
-  if filter_keys.include?(:quarantine)
-    skip("Only running tests tagged with :quarantine and any of #{included_filters}") unless quarantine_and_optional_other_tag?(metadata_keys, included_filters)
-  else
-    skip('In quarantine') if metadata_keys.include?(:quarantine)
-  end
-end
-
-def filters_other_than_quarantine(filter_keys)
-  filter_keys.reject { |key| key == :quarantine }
-end
-
-# Checks if a test has the 'quarantine' tag and other tags in the inclusion filter.
-#
-# Returns true if
-# - the metadata includes the quarantine tag
-#   - and the metadata and inclusion filter both have any other tag
-#   - or no other tags are in the inclusion filter
-def quarantine_and_optional_other_tag?(metadata_keys, included_filters)
-  return false unless metadata_keys.include? :quarantine
-  return true if included_filters.empty?
-
-  included_filters.any? { |key| metadata_keys.include? key }
-end

qa/spec/specs/helpers/quarantine_spec.rb

+# frozen_string_literal: true
+
+require 'rspec/core/sandbox'
+
+# We need a reporter for internal tests that's different from the reporter for
+# external tests otherwise the results will be mixed up. We don't care about
+# most reporting, but we do want to know if a test fails
+class RaiseOnFailuresReporter < RSpec::Core::NullReporter
+  def self.example_failed(example)
+    raise example.exception
+  end
+end
+
+# We use an example group wrapper to prevent the state of internal tests
+# expanding into the global state
+# See: https://github.com/rspec/rspec-core/issues/2603
+def describe_successfully(*args, &describe_body)
+  example_group    = RSpec.describe(*args, &describe_body)
+  ran_successfully = example_group.run RaiseOnFailuresReporter
+  expect(ran_successfully).to eq true
+  example_group
+end
+
+RSpec.configure do |c|
+  c.around do |ex|
+    RSpec::Core::Sandbox.sandboxed do |config|
+      # If there is an example-within-an-example, we want to make sure the inner example
+      # does not get a reference to the outer example (the real spec) if it calls
+      # something like `pending`
+      config.before(:context) { RSpec.current_example = nil }
+
+      config.color_mode = :off
+
+      # Load airborne again to avoid "undefined method `match_expected_default?'" errors
+      # that happen because a hook calls a method added via a custom RSpec setting
+      # that is removed when the RSpec configuration is sandboxed.
+      # If this needs to be changed (e.g., to load other libraries as well), see
+      # this discussion for alternative solutions:
+      # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25223#note_143392053
+      load 'airborne.rb'
+
+      ex.run
+    end
+  end
+end
+
+describe QA::Specs::Helpers::Quarantine do
+  describe '.skip_or_run_quarantined_contexts' do
+    context 'with no tag focused' do
+      before do
+        described_class.configure_rspec
+      end
+
+      it 'skips before hooks of quarantined contexts' do
+        executed_hooks = []
+
+        group = describe_successfully('quarantine', :quarantine) do
+          before(:all) do
+            executed_hooks << :before_all
+          end
+          before do
+            executed_hooks << :before
+          end
+          example {}
+        end
+
+        expect(executed_hooks).to eq []
+        expect(group.descendant_filtered_examples.first.execution_result.status).to eq(:pending)
+        expect(group.descendant_filtered_examples.first.execution_result.pending_message)
+          .to eq('In quarantine')
+      end
+
+      it 'executes before hooks of non-quarantined contexts' do
+        executed_hooks = []
+
+        group = describe_successfully do
+          before(:all) do
+            executed_hooks << :before_all
+          end
+          before do
+            executed_hooks << :before
+          end
+          example {}
+        end
+
+        expect(executed_hooks).to eq [:before_all, :before]
+        expect(group.descendant_filtered_examples.first.execution_result.status).to eq(:passed)
+      end
+    end
+
+    context 'with :quarantine focused' do
+      before do
+        described_class.configure_rspec
+        RSpec.configure do |c|
+          c.filter_run :quarantine
+        end
+      end
+
+      it 'executes before hooks of quarantined contexts' do
+        executed_hooks = []
+
+        group = describe_successfully('quarantine', :quarantine) do
+          before(:all) do
+            executed_hooks << :before_all
+          end
+          before do
+            executed_hooks << :before
+          end
+          example {}
+        end
+
+        expect(executed_hooks).to eq [:before_all, :before]
+        expect(group.descendant_filtered_examples.first.execution_result.status).to eq(:passed)
+      end
+
+      it 'skips before hooks of non-quarantined contexts' do
+        executed_hooks = []
+
+        group = describe_successfully do
+          before(:all) do
+            executed_hooks << :before_all
+          end
+          before do
+            executed_hooks << :before
+          end
+          example {}
+        end
+
+        expect(executed_hooks).to eq []
+        expect(group.descendant_filtered_examples.first).to be_nil
+      end
+    end
+  end
+
+  describe '.skip_or_run_quarantined_tests' do
+    context 'with no tag focused' do
+      before do
+        described_class.configure_rspec
+      end
+
+      it 'skips quarantined tests' do
+        group = describe_successfully do
+          it('is pending', :quarantine) {}
+        end
+
+        expect(group.examples.first.execution_result.status).to eq(:pending)
+        expect(group.examples.first.execution_result.pending_message)
+          .to eq('In quarantine')
+      end
+
+      it 'executes non-quarantined tests' do
+        group = describe_successfully do
+          example {}
+        end
+
+        expect(group.examples.first.execution_result.status).to eq(:passed)
+      end
+    end
+
+    context 'with :quarantine focused' do
+      before do
+        described_class.configure_rspec
+        RSpec.configure do |c|
+          c.filter_run :quarantine
+        end
+      end
+
+      it 'executes quarantined tests' do
+        group = describe_successfully do
+          it('passes', :quarantine) {}
+        end
+
+        expect(group.examples.first.execution_result.status).to eq(:passed)
+      end
+
+      it 'ignores non-quarantined tests' do
+        group = describe_successfully do
+          example {}
+        end
+
+        expect(group.examples.first.execution_result.status).to be_nil
+      end
+    end
+
+    context 'with a non-quarantine tag focused' do
+      before do
+        described_class.configure_rspec
+        RSpec.configure do |c|
+          c.filter_run :foo
+        end
+      end
+
+      it 'ignores non-quarantined non-focused tests' do
+        group = describe_successfully do
+          example {}
+        end
+
+        expect(group.examples.first.execution_result.status).to be_nil
+      end
+
+      it 'executes non-quarantined focused tests' do
+        group = describe_successfully do
+          it('passes', :foo) {}
+        end
+
+        expect(group.examples.first.execution_result.status).to be(:passed)
+      end
+
+      it 'ignores quarantined tests' do
+        group = describe_successfully do
+          it('is ignored', :quarantine) {}
+        end
+
+        expect(group.examples.first.execution_result.status).to be_nil
+      end
+
+      it 'skips quarantined focused tests' do
+        group = describe_successfully do
+          it('is pending', :quarantine, :foo) {}
+        end
+
+        expect(group.examples.first.execution_result.status).to be(:pending)
+        expect(group.examples.first.execution_result.pending_message)
+          .to eq('In quarantine')
+      end
+    end
+
+    context 'with :quarantine and non-quarantine tags focused' do
+      before do
+        described_class.configure_rspec
+        RSpec.configure do |c|
+          c.filter_run :foo, :bar, :quarantine
+        end
+      end
+
+      it 'ignores non-quarantined non-focused tests' do
+        group = describe_successfully do
+          example {}
+        end
+
+        expect(group.examples.first.execution_result.status).to be_nil
+      end
+
+      it 'skips non-quarantined focused tests' do
+        group = describe_successfully do
+          it('is pending', :foo) {}
+        end
+
+        expect(group.examples.first.execution_result.status).to be(:pending)
+        expect(group.examples.first.execution_result.pending_message)
+          .to eq('Only running tests tagged with :quarantine and any of [:bar, :foo]')
+      end
+
+      it 'skips quarantined non-focused tests' do
+        group = describe_successfully do
+          it('is pending', :quarantine) {}
+        end
+
+        expect(group.examples.first.execution_result.status).to be(:pending)
+      end
+
+      it 'executes quarantined focused tests' do
+        group = describe_successfully do
+          it('passes', :quarantine, :foo) {}
+        end
+
+        expect(group.examples.first.execution_result.status).to be(:passed)
+      end
+    end
+  end
+end

spec/models/commit_spec.rb

@@ -542,7 +542,7 @@ eos
     end
   end
 
-  describe '#uri_type' do
+  shared_examples '#uri_type' do
     it 'returns the URI type at the given path' do
       expect(commit.uri_type('files/html')).to be(:tree)
       expect(commit.uri_type('files/images/logo-black.png')).to be(:raw)
@@ -561,6 +561,20 @@ eos
     end
   end
 
+  describe '#uri_type with Gitaly enabled' do
+    it_behaves_like "#uri_type"
+  end
+
+  describe '#uri_type with Rugged enabled', :enable_rugged do
+    it 'calls out to the Rugged implementation' do
+      allow_any_instance_of(Rugged::Tree).to receive(:path).with('files/html').and_call_original
+
+      commit.uri_type('files/html')
+    end
+
+    it_behaves_like '#uri_type'
+  end
+
   describe '.from_hash' do
     let(:new_commit) { described_class.from_hash(commit.to_hash, project) }