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 <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
