From 67c38a6573f35333cf9b1a399431b86e8b376443 Mon Sep 17 00:00:00 2001 From: Mark Lapierre Date: Wed, 27 Mar 2019 19:03:03 +0000 Subject: Set feature flag via command line First attempt at allowing a feature flag to be set via the command line when running tests. This will enable the flag, run the tests, and then disable the flag. Using OptionParser meant changing how scenarios get the instance address, so this also allows the address to be set as a command line option. It's backwards compatible (you can still provide the address as the command line option after the scenario) --- qa/README.md | 31 ++++++++++++++-- qa/qa.rb | 2 ++ qa/qa/resource/api_fabricator.rb | 3 -- qa/qa/runtime/address.rb | 7 ++++ qa/qa/runtime/feature.rb | 36 +++++++++++++++++++ qa/qa/scenario/bootable.rb | 10 ++++-- qa/qa/scenario/shared_attributes.rb | 12 +++++++ qa/qa/scenario/template.rb | 31 ++++++++++++++-- qa/qa/scenario/test/instance/all.rb | 1 + qa/qa/scenario/test/instance/smoke.rb | 1 + qa/qa/scenario/test/integration/mattermost.rb | 9 +++-- qa/qa/support/api.rb | 3 ++ qa/spec/runtime/feature_spec.rb | 41 ++++++++++++++++++++++ qa/spec/runtime/scenario_spec.rb | 8 +++++ qa/spec/scenario/bootable_spec.rb | 9 ++++- qa/spec/scenario/template_spec.rb | 28 +++++++++++++++ qa/spec/scenario/test/integration/github_spec.rb | 2 +- .../scenario/test/integration/mattermost_spec.rb | 11 ++++-- .../shared_examples/scenario_shared_examples.rb | 40 ++++++++++++++++----- 19 files changed, 259 insertions(+), 26 deletions(-) create mode 100644 qa/qa/runtime/feature.rb create mode 100644 qa/qa/scenario/shared_attributes.rb create mode 100644 qa/spec/runtime/feature_spec.rb create mode 100644 qa/spec/scenario/template_spec.rb diff --git a/qa/README.md b/qa/README.md index 735868e7640..7d66f7d5abc 100644 --- a/qa/README.md +++ b/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 diff --git a/qa/qa.rb b/qa/qa.rb index ec8aef31e48..672f4aa928a 100644 --- a/qa/qa.rb +++ b/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. diff --git a/qa/qa/resource/api_fabricator.rb b/qa/qa/resource/api_fabricator.rb index 98eebac0880..de04467ff5b 100644 --- a/qa/qa/resource/api_fabricator.rb +++ b/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) diff --git a/qa/qa/runtime/address.rb b/qa/qa/runtime/address.rb index ffad3974b02..af0537dc17c 100644 --- a/qa/qa/runtime/address.rb +++ b/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 diff --git a/qa/qa/runtime/feature.rb b/qa/qa/runtime/feature.rb new file mode 100644 index 00000000000..1b4ae7adbbe --- /dev/null +++ b/qa/qa/runtime/feature.rb @@ -0,0 +1,36 @@ +# 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 diff --git a/qa/qa/scenario/bootable.rb b/qa/qa/scenario/bootable.rb index dd12ea6d492..038418be023 100644 --- a/qa/qa/scenario/bootable.rb +++ b/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? diff --git a/qa/qa/scenario/shared_attributes.rb b/qa/qa/scenario/shared_attributes.rb new file mode 100644 index 00000000000..40d5c6b1ff1 --- /dev/null +++ b/qa/qa/scenario/shared_attributes.rb @@ -0,0 +1,12 @@ +# 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 diff --git a/qa/qa/scenario/template.rb b/qa/qa/scenario/template.rb index cb1a1de6b9a..b8ea26e805e 100644 --- a/qa/qa/scenario/template.rb +++ b/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 diff --git a/qa/qa/scenario/test/instance/all.rb b/qa/qa/scenario/test/instance/all.rb index a07c26431bd..168ac4c09a1 100644 --- a/qa/qa/scenario/test/instance/all.rb +++ b/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 diff --git a/qa/qa/scenario/test/instance/smoke.rb b/qa/qa/scenario/test/instance/smoke.rb index a7d2cb27f27..43f0623483e 100644 --- a/qa/qa/scenario/test/instance/smoke.rb +++ b/qa/qa/scenario/test/instance/smoke.rb @@ -8,6 +8,7 @@ module QA # class Smoke < Template include Bootable + include SharedAttributes tags :smoke end diff --git a/qa/qa/scenario/test/integration/mattermost.rb b/qa/qa/scenario/test/integration/mattermost.rb index ece6fba75c9..f5072ee227c 100644 --- a/qa/qa/scenario/test/integration/mattermost.rb +++ b/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 diff --git a/qa/qa/support/api.rb b/qa/qa/support/api.rb index 229bfb44fa5..31cff5a241c 100644 --- a/qa/qa/support/api.rb +++ b/qa/qa/support/api.rb @@ -1,6 +1,9 @@ module QA module Support module Api + HTTP_STATUS_OK = 200 + HTTP_STATUS_CREATED = 201 + def post(url, payload) RestClient::Request.execute( method: :post, diff --git a/qa/spec/runtime/feature_spec.rb b/qa/spec/runtime/feature_spec.rb new file mode 100644 index 00000000000..192299b7857 --- /dev/null +++ b/qa/spec/runtime/feature_spec.rb @@ -0,0 +1,41 @@ +# 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 diff --git a/qa/spec/runtime/scenario_spec.rb b/qa/spec/runtime/scenario_spec.rb index 7009192bcc0..70fc71ffc02 100644 --- a/qa/spec/runtime/scenario_spec.rb +++ b/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/ diff --git a/qa/spec/scenario/bootable_spec.rb b/qa/spec/scenario/bootable_spec.rb index 273aac7677e..bd89b21f7fb 100644 --- a/qa/spec/scenario/bootable_spec.rb +++ b/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 diff --git a/qa/spec/scenario/template_spec.rb b/qa/spec/scenario/template_spec.rb new file mode 100644 index 00000000000..f97fc22daf9 --- /dev/null +++ b/qa/spec/scenario/template_spec.rb @@ -0,0 +1,28 @@ +# 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 diff --git a/qa/spec/scenario/test/integration/github_spec.rb b/qa/spec/scenario/test/integration/github_spec.rb index c2aeb1ded1d..6112ba7c694 100644 --- a/qa/spec/scenario/test/integration/github_spec.rb +++ b/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 diff --git a/qa/spec/scenario/test/integration/mattermost_spec.rb b/qa/spec/scenario/test/integration/mattermost_spec.rb index 59caf2ba2cd..4e75e72f4d2 100644 --- a/qa/spec/scenario/test/integration/mattermost_spec.rb +++ b/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 diff --git a/qa/spec/shared_examples/scenario_shared_examples.rb b/qa/spec/shared_examples/scenario_shared_examples.rb index 5fd55d7d96b..697e6cb39c8 100644 --- a/qa/spec/shared_examples/scenario_shared_examples.rb +++ b/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 -- cgit v1.2.1