Dduvall has uploaded a new change for review. https://gerrit.wikimedia.org/r/172905
Change subject: Strict configuration lookup ...................................................................... Strict configuration lookup The absence of a configuration key lookup now raises a ConfigurationError unless the `:default` option is provided. Testing revealed too many false positives due to falling back to the base configuration when an alternative was expected. Change-Id: I3124c9c775e1781255a1ca601522796b0111780b --- M lib/mediawiki_selenium/environment.rb M spec/environment_spec.rb 2 files changed, 67 insertions(+), 54 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/selenium refs/changes/05/172905/1 diff --git a/lib/mediawiki_selenium/environment.rb b/lib/mediawiki_selenium/environment.rb index 24a3485..8810728 100644 --- a/lib/mediawiki_selenium/environment.rb +++ b/lib/mediawiki_selenium/environment.rb @@ -7,14 +7,6 @@ class Environment include Comparable - REQUIRED_CONFIG = [ - :browser, - :mediawiki_api_url, - :mediawiki_password, - :mediawiki_url, - :mediawiki_user, - ] - attr_reader :config protected :config @@ -154,30 +146,29 @@ # Whether browsers should be left open after each scenario completes. # def keep_browser_open? - lookup(:keep_browser_open) == "true" + lookup(:keep_browser_open, default: "false") == "true" end # Returns the configured value for the given env variable name. # # @param key [Symbol] Environment variable name. - # @param id [Symbol] Alternative variable suffix. + # @param options [Hash] Options. + # @option options [Symbol] :id Alternative ID. + # @option options [Object] :default Default if no configuration is found. # # @return [String] # - def lookup(key, id = nil) - full_key = id.nil? ? key : "#{key}_#{id}" - full_key = full_key.to_sym - value = @config[full_key] + def lookup(key, options = {}) + key = "#{key}_#{options[:id]}" if options.fetch(:id, nil) + key = normalize_key(key) + + value = @config[key] if value.nil? || value.to_s.empty? - if id.nil? - if REQUIRED_CONFIG.include?(full_key) - raise ConfigurationError, full_key - else - nil - end + if options.include?(:default) + options[:default] else - lookup(key) + raise ConfigurationError, key end else value @@ -187,14 +178,17 @@ # Returns the configured values for the given env variable names. # # @param keys [Array<Symbol>] Environment variable names. - # @param id [Symbol] Alternative variable suffix. + # @param options [Hash] Options. + # @option options [Symbol] :id Alternative ID. + # @option options [Object] :default Default if no configuration is found. # # @return [Array<String>] # - def lookup_all(keys, id = nil) + # @see #lookup + # + def lookup_all(keys, options = {}) keys.each.with_object({}) do |key, hash| - value = lookup(key, id) - hash[key] = value unless value.nil? + hash[key] = lookup(key, options) end end @@ -233,7 +227,7 @@ # @return [Boolean] # def remote? - RemoteBrowserFactory::REQUIRED_CONFIG.all? { |name| lookup(name) } + RemoteBrowserFactory::REQUIRED_CONFIG.all? { |name| lookup(name, default: false) } end # Executes teardown tasks including instructing all browser factories to @@ -363,22 +357,26 @@ # @return [Environment] The modified environment. # def with_alternative(names, id, &blk) - with(lookup_all(Array(names), id), &blk) + with(lookup_all(Array(names), id: id), &blk) end private def browser_config - lookup_all(browser_factory.all_binding_keys) + lookup_all(browser_factory.all_binding_keys, default: nil).reject { |k, v| v.nil? } end def password_variable - name = lookup(:mediawiki_password_variable) - (name.nil? || name.empty?) ? :mediawiki_password : name.to_sym + name = lookup(:mediawiki_password_variable, default: "") + name.empty? ? :mediawiki_password : normalize_key(name) end def normalize_config(hash) - hash.each.with_object({}) { |(k, v), acc| acc[k.to_s.downcase.to_sym] = v } + hash.each.with_object({}) { |(k, v), acc| acc[normalize_key(k)] = v } + end + + def normalize_key(key) + key.to_s.downcase.to_sym end def with(overrides = {}, &blk) diff --git a/spec/environment_spec.rb b/spec/environment_spec.rb index 7e98ad6..1c3f65c 100644 --- a/spec/environment_spec.rb +++ b/spec/environment_spec.rb @@ -203,48 +203,63 @@ end describe "#lookup" do - subject { env.lookup(key, id) } + subject { env.lookup(key, options) } let(:config) { { foo: "foo_value", foo_b: "foo_b_value", bar: "bar_value" } } - context "given no alternative ID" do - let(:id) { nil } + context "for a key that exists" do let(:key) { :foo } + let(:options) { {} } - it "looks up the given key only" do + it "returns the configuration" do expect(subject).to eq("foo_value") - end - - context "and a key that doesn't exist" do - let(:key) { :baz } - - it { is_expected.to be(nil) } end end - context "given an alternative ID" do - let(:id) { :b } + context "for a key that doesn't exist" do + let(:key) { :baz } - context "for an alternative that exists" do - let(:key) { :foo } + context "given no default value" do + let(:options) { {} } - it "returns the alternative value" do - expect(subject).to eq("foo_b_value") + it "raises a ConfigurationError" do + expect { subject }.to raise_error(ConfigurationError) end end - context "for an alternative that doesn't exist" do - let(:key) { :bar } + context "given a default value" do + let(:options) { { default: default } } + let(:default) { double(Object) } - it "falls back to the base value" do - expect(subject).to eq("bar_value") + it { is_expected.to be(default) } + end + end + + context "for an alternative that exists" do + let(:key) { :foo } + let(:options) { { id: :b } } + + it "returns the configured alternative" do + expect(subject).to eq("foo_b_value") + end + end + + context "for an alternative that doesn't exist" do + let(:key) { :foo } + + context "given no default value" do + let(:options) { { id: :c } } + + it "raises a ConfigurationError" do + expect { subject }.to raise_error(ConfigurationError) end end - context "and a key that doesn't exist" do - let(:key) { :baz } + context "given a default value" do + let(:options) { { id: :c, default: default } } + let(:default) { double(Object) } - it { is_expected.to be(nil) } + it { is_expected.to be(default) } end end end -- To view, visit https://gerrit.wikimedia.org/r/172905 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3124c9c775e1781255a1ca601522796b0111780b Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/selenium Gerrit-Branch: env-abstraction-layer Gerrit-Owner: Dduvall <dduv...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits