Merge branch 'qa-ml-feature-flag-command-line' into 'master'

[QA] Toggle Gitaly N+1 detector via feature flag and run tests

Closes gitlab-qa#374

See merge request gitlab-org/gitlab-ce!26060

qa/README.md

@@ -55,16 +55,19 @@ You can also supply specific tests to run as another parameter. For example, to
 run the repository-related specs, you can execute:
 
 ```
-bin/qa Test::Instance::All http://localhost qa/specs/features/repository/
+bin/qa Test::Instance::All http://localhost -- qa/specs/features/browser_ui/3_create/repository
 ```
 
 Since the arguments would be passed to `rspec`, you could use all `rspec`
 options there. For example, passing `--backtrace` and also line number:
 
 ```
-bin/qa Test::Instance::All http://localhost qa/specs/features/project/create_spec.rb:3 --backtrace
+bin/qa Test::Instance::All http://localhost -- qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb:6 --backtrace
 ```
 
+Note that the separator `--` is required; all subsequent options will be
+ignored by the QA framework and passed to `rspec`.
+
 ### Overriding the authenticated user
 
 Unless told otherwise, the QA tests will run as the default `root` user seeded
@@ -117,7 +120,7 @@ tests that are expected to fail while a fix is in progress (similar to how
  can be used).
 
 ```
-bin/qa Test::Instance::All http://localhost --tag quarantine
+bin/qa Test::Instance::All http://localhost -- --tag quarantine
 ```
 
 If `quarantine` is used with other tags, tests will only be run if they have at
@@ -128,3 +131,25 @@ For example, suppose one test has `:smoke` and `:quarantine` metadata, and
 another test has `:ldap` and `:quarantine` metadata. If the tests are run with
 `--tag smoke --tag quarantine`, only the first test will run. The test with
 `:ldap` will not run even though it also has `:quarantine`.
+
+### Running tests with a feature flag enabled
+
+Tests can be run with with a feature flag enabled by using the command-line
+option `--enable-feature FEATURE_FLAG`. For example, to enable the feature flag
+that enforces Gitaly request limits, you would use the command:
+
+```
+bin/qa Test::Instance::All http://localhost --enable-feature gitaly_enforce_requests_limits
+```
+
+This will instruct the QA framework to enable the `gitaly_enforce_requests_limits`
+feature flag ([via the API](https://docs.gitlab.com/ee/api/features.html)), run
+all the tests in the `Test::Instance::All` scenario, and then disable the
+feature flag again.
+
+Note: the QA framework doesn't currently allow you to easily toggle a feature
+flag during a single test, [as you can in unit tests](https://docs.gitlab.com/ee/development/feature_flags.html#specs),
+but [that capability is planned](https://gitlab.com/gitlab-org/quality/team-tasks/issues/77).
+
+Note also that the `--` separator isn't used because `--enable-feature` is a QA
+framework option, not an `rspec` option.
\ No newline at end of file

qa/qa.rb

@@ -17,6 +17,7 @@ module QA
     autoload :Env, 'qa/runtime/env'
     autoload :Address, 'qa/runtime/address'
     autoload :Path, 'qa/runtime/path'
+    autoload :Feature, 'qa/runtime/feature'
     autoload :Fixtures, 'qa/runtime/fixtures'
     autoload :Logger, 'qa/runtime/logger'
 
@@ -89,6 +90,7 @@ module QA
     autoload :Bootable, 'qa/scenario/bootable'
     autoload :Actable, 'qa/scenario/actable'
     autoload :Template, 'qa/scenario/template'
+    autoload :SharedAttributes, 'qa/scenario/shared_attributes'
 
     ##
     # Test scenario entrypoints.

qa/qa/resource/api_fabricator.rb

@@ -8,9 +8,6 @@ module QA
     module ApiFabricator
       include Capybara::DSL
 
-      HTTP_STATUS_OK = 200
-      HTTP_STATUS_CREATED = 201
-
       ResourceNotFoundError = Class.new(RuntimeError)
       ResourceFabricationFailedError = Class.new(RuntimeError)
       ResourceURLMissingError = Class.new(RuntimeError)

qa/qa/runtime/address.rb

@@ -15,6 +15,13 @@ module QA
           @instance.to_s
         end
       end
+
+      def self.valid?(value)
+        uri = URI.parse(value)
+        uri.is_a?(URI::HTTP) && !uri.host.nil?
+      rescue URI::InvalidURIError
+        false
+      end
     end
   end
 end

qa/qa/runtime/feature.rb

+# frozen_string_literal: true
+
+module QA
+  module Runtime
+    module Feature
+      extend self
+      extend Support::Api
+
+      SetFeatureError = Class.new(RuntimeError)
+
+      def enable(key)
+        QA::Runtime::Logger.info("Enabling feature: #{key}")
+        set_feature(key, true)
+      end
+
+      def disable(key)
+        QA::Runtime::Logger.info("Disabling feature: #{key}")
+        set_feature(key, false)
+      end
+
+      private
+
+      def api_client
+        @api_client ||= Runtime::API::Client.new(:gitlab)
+      end
+
+      def set_feature(key, value)
+        request = Runtime::API::Request.new(api_client, "/features/#{key}")
+        response = post(request.url, { value: value })
+        unless response.code == QA::Support::Api::HTTP_STATUS_CREATED
+          raise SetFeatureError, "Setting feature flag #{key} to #{value} failed with `#{response}`."
+        end
+      end
+    end
+  end
+end

qa/qa/scenario/bootable.rb

@@ -23,7 +23,7 @@ module QA
 
           arguments.parse!(argv)
 
-          self.perform(Runtime::Scenario.attributes, *arguments.default_argv)
+          self.perform(Runtime::Scenario.attributes, *argv)
         end
 
         private
@@ -33,7 +33,13 @@ module QA
         end
 
         def options
-          @options ||= []
+          # Scenario options/attributes are global. There's only ever one
+          # scenario at a time, but they can be inherited and we want scenarios
+          # to share the attributes of their ancestors. For example, `Mattermost`
+          # inherits from `Test::Instance::All` but if this were an instance
+          # variable then `Mattermost` wouldn't have access to the attributes
+          # in `All`
+          @@options ||= [] # rubocop:disable Style/ClassVars
         end
 
         def has_attributes?

qa/qa/scenario/shared_attributes.rb

+# frozen_string_literal: true
+
+module QA
+  module Scenario
+    module SharedAttributes
+      include Bootable
+
+      attribute :gitlab_address, '--address URL', 'Address of the instance to test'
+      attribute :enable_feature, '--enable-feature FEATURE_FLAG', 'Enable a feature before running tests'
+    end
+  end
+end

qa/qa/scenario/template.rb

@@ -18,19 +18,44 @@ module QA
         end
       end
 
-      def perform(address, *rspec_options)
-        Runtime::Scenario.define(:gitlab_address, address)
+      def perform(options, *args)
+        extract_address(:gitlab_address, options, args)
 
         ##
         # Perform before hooks, which are different for CE and EE
         #
         Runtime::Release.perform_before_hooks
 
+        Runtime::Feature.enable(options[:enable_feature]) if options.key?(:enable_feature)
+
         Specs::Runner.perform do |specs|
           specs.tty = true
           specs.tags = self.class.focus
-          specs.options = rspec_options if rspec_options.any?
+          specs.options = args if args.any?
+        end
+      ensure
+        Runtime::Feature.disable(options[:enable_feature]) if options.key?(:enable_feature)
       end
+
+      def extract_option(name, options, args)
+        option = if options.key?(name)
+                   options[name]
+                 else
+                   args.shift
+                 end
+
+        Runtime::Scenario.define(name, option)
+
+        option
+      end
+
+      # For backwards-compatibility, if the gitlab instance address is not
+      # specified as an option parsed by OptionParser, it can be specified as
+      # the first argument
+      def extract_address(name, options, args)
+        address = extract_option(name, options, args)
+
+        raise ::ArgumentError, "The address provided for `#{name}` is not valid: #{address}" unless Runtime::Address.valid?(address)
       end
     end
   end

qa/qa/scenario/test/instance/all.rb

@@ -8,6 +8,7 @@ module QA
       module Instance
         class All < Template
           include Bootable
+          include SharedAttributes
         end
       end
     end

qa/qa/scenario/test/instance/smoke.rb

@@ -8,6 +8,7 @@ module QA
         #
         class Smoke < Template
           include Bootable
+          include SharedAttributes
 
           tags :smoke
         end

qa/qa/scenario/test/integration/mattermost.rb

@@ -9,10 +9,13 @@ module QA
         class Mattermost < Test::Instance::All
           tags :mattermost
 
-          def perform(address, mattermost, *rspec_options)
-            Runtime::Scenario.define(:mattermost_address, mattermost)
+          attribute :mattermost_address, '--mattermost-address URL', 'Address of the Mattermost server'
 
-            super(address, *rspec_options)
+          def perform(options, *args)
+            extract_address(:gitlab_address, options, args)
+            extract_address(:mattermost_address, options, args)
+
+            super(options, *args)
           end
         end
       end

qa/qa/support/api.rb

 module QA
   module Support
     module Api
+      HTTP_STATUS_OK = 200
+      HTTP_STATUS_CREATED = 201
+
       def post(url, payload)
         RestClient::Request.execute(
           method: :post,

qa/spec/runtime/feature_spec.rb

+# frozen_string_literal: true
+
+describe QA::Runtime::Feature do
+  let(:api_client) { double('QA::Runtime::API::Client') }
+  let(:request) { Struct.new(:url).new('http://api') }
+  let(:response) { Struct.new(:code).new(201) }
+
+  before do
+    allow(described_class).to receive(:api_client).and_return(api_client)
+  end
+
+  describe '.enable' do
+    it 'enables a feature flag' do
+      expect(QA::Runtime::API::Request)
+        .to receive(:new)
+        .with(api_client, "/features/a-flag")
+        .and_return(request)
+      expect(described_class)
+        .to receive(:post)
+        .with(request.url, { value: true })
+        .and_return(response)
+
+      subject.enable('a-flag')
+    end
+  end
+
+  describe '.disable' do
+    it 'disables a feature flag' do
+      expect(QA::Runtime::API::Request)
+        .to receive(:new)
+        .with(api_client, "/features/a-flag")
+        .and_return(request)
+      expect(described_class)
+        .to receive(:post)
+        .with(request.url, { value: false })
+        .and_return(response)
+
+      subject.disable('a-flag')
+    end
+  end
+end

qa/spec/runtime/scenario_spec.rb

@@ -13,6 +13,14 @@ describe QA::Runtime::Scenario do
       .to eq(my_attribute: 'some-value', another_attribute: 'another-value')
   end
 
+  it 'replaces an existing attribute' do
+    subject.define(:my_attribute, 'some-value')
+    subject.define(:my_attribute, 'another-value')
+
+    expect(subject.my_attribute).to eq 'another-value'
+    expect(subject.attributes).to eq(my_attribute: 'another-value')
+  end
+
   it 'raises error when attribute is not known' do
     expect { subject.invalid_accessor }
       .to raise_error ArgumentError, /invalid_accessor/

qa/spec/scenario/bootable_spec.rb

@@ -4,14 +4,21 @@ describe QA::Scenario::Bootable do
       .include(described_class)
   end
 
+  before do
+    allow(subject).to receive(:options).and_return([])
+    allow(QA::Runtime::Scenario).to receive(:attributes).and_return({})
+  end
+
   it 'makes it possible to define the scenario attribute' do
     subject.class_eval do
       attribute :something, '--something SOMETHING', 'Some attribute'
       attribute :another, '--another ANOTHER', 'Some other attribute'
     end
 
+    # If we run just this test from the command line it fails unless
+    # we include the command line args that we use to select this test.
     expect(subject).to receive(:perform)
-      .with(something: 'test', another: 'other')
+      .with({ something: 'test', another: 'other' })
 
     subject.launch!(%w[--another other --something test])
   end

qa/spec/scenario/template_spec.rb

+# frozen_string_literal: true
+
+describe QA::Scenario::Template do
+  let(:feature) { spy('Runtime::Feature') }
+  let(:release) { spy('Runtime::Release') }
+
+  before do
+    stub_const('QA::Runtime::Release', release)
+    stub_const('QA::Runtime::Feature', feature)
+    allow(QA::Specs::Runner).to receive(:perform)
+    allow(QA::Runtime::Address).to receive(:valid?).and_return(true)
+  end
+
+  it 'allows a feature to be enabled' do
+    subject.perform({ enable_feature: 'a-feature' })
+
+    expect(feature).to have_received(:enable).with('a-feature')
+  end
+
+  it 'ensures an enabled feature is disabled afterwards' do
+    allow(QA::Specs::Runner).to receive(:perform).and_raise('failed test')
+
+    expect { subject.perform({ enable_feature: 'a-feature' }) }.to raise_error('failed test')
+
+    expect(feature).to have_received(:enable).with('a-feature')
+    expect(feature).to have_received(:disable).with('a-feature')
+  end
+end

qa/spec/scenario/test/integration/github_spec.rb

@@ -12,7 +12,7 @@ describe QA::Scenario::Test::Integration::Github do
       let(:tags) { [:github] }
 
       it 'requires a GitHub access token' do
-        subject.perform('gitlab_address')
+        subject.perform(args)
 
         expect(env).to have_received(:require_github_access_token!)
       end

qa/spec/scenario/test/integration/mattermost_spec.rb

@@ -4,14 +4,21 @@ describe QA::Scenario::Test::Integration::Mattermost do
   context '#perform' do
     it_behaves_like 'a QA scenario class' do
       let(:args) { %w[gitlab_address mattermost_address] }
+      let(:args) do
+        {
+          gitlab_address: 'http://gitlab_address',
+          mattermost_address: 'http://mattermost_address'
+        }
+      end
+      let(:named_options) { %w[--address http://gitlab_address --mattermost-address http://mattermost_address] }
       let(:tags) { [:mattermost] }
       let(:options) { ['path1']}
 
       it 'requires a GitHub access token' do
-        subject.perform(*args)
+        subject.perform(args)
 
         expect(attributes).to have_received(:define)
-          .with(:mattermost_address, 'mattermost_address')
+          .with(:mattermost_address, 'http://mattermost_address')
       end
     end
   end

qa/spec/shared_examples/scenario_shared_examples.rb

@@ -2,19 +2,23 @@
 
 shared_examples 'a QA scenario class' do
   let(:attributes) { spy('Runtime::Scenario') }
-  let(:release) { spy('Runtime::Release') }
   let(:runner) { spy('Specs::Runner') }
+  let(:release) { spy('Runtime::Release') }
+  let(:feature) { spy('Runtime::Feature') }
 
-  let(:args) { ['gitlab_address'] }
+  let(:args) { { gitlab_address: 'http://gitlab_address' } }
+  let(:named_options) { %w[--address http://gitlab_address] }
   let(:tags) { [] }
   let(:options) { %w[path1 path2] }
 
   before do
+    stub_const('QA::Specs::Runner', runner)
     stub_const('QA::Runtime::Release', release)
     stub_const('QA::Runtime::Scenario', attributes)
-    stub_const('QA::Specs::Runner', runner)
+    stub_const('QA::Runtime::Feature', feature)
 
     allow(runner).to receive(:perform).and_yield(runner)
+    allow(QA::Runtime::Address).to receive(:valid?).and_return(true)
   end
 
   it 'responds to perform' do
@@ -22,28 +26,48 @@ shared_examples 'a QA scenario class' do
   end
 
   it 'sets an address of the subject' do
-    subject.perform(*args)
+    subject.perform(args)
 
-    expect(attributes).to have_received(:define).with(:gitlab_address, 'gitlab_address')
+    expect(attributes).to have_received(:define).with(:gitlab_address, 'http://gitlab_address').at_least(:once)
   end
 
   it 'performs before hooks' do
-    subject.perform(*args)
+    subject.perform(args)
 
     expect(release).to have_received(:perform_before_hooks)
   end
 
   it 'sets tags on runner' do
-    subject.perform(*args)
+    subject.perform(args)
 
     expect(runner).to have_received(:tags=).with(tags)
   end
 
   context 'specifying RSpec options' do
     it 'sets options on runner' do
-      subject.perform(*args, *options)
+      subject.perform(args, *options)
 
       expect(runner).to have_received(:options=).with(options)
     end
   end
+
+  context 'with named command-line options' do
+    it 'converts options to attributes' do
+      described_class.launch!(named_options)
+
+      args do |k, v|
+        expect(attributes).to have_received(:define).with(k, v)
+      end
+    end
+
+    it 'raises an error if the option is invalid' do
+      expect { described_class.launch!(['--foo']) }.to raise_error(OptionParser::InvalidOption)
+    end
+
+    it 'passes on options after --' do
+      expect(described_class).to receive(:perform).with(attributes, *%w[--tag quarantine])
+
+      described_class.launch!(named_options.push(*%w[-- --tag quarantine]))
+    end
+  end
 end