Dduvall has submitted this change and it was merged.
Change subject: Refactored EAL configuration overrides
......................................................................
Refactored EAL configuration overrides
One big caveat of implementing `Environment#with` by cloning + using
instance evaluation over block evaluation with a normal closure is that
instance variables set within the scope of the given block are lost to
the original `Environment` object.
Step(...) do
@foo = "a"
with_alternative(...) do # or #on_wiki, #as_user, etc.
@foo = "b"
end
@foo == "b" # => false
end
This behavior would likely violate the principle of least surprise for
most users, so the cloning has been replaced by temporarily overwriting
environment config state, yielding normally (so the expected closure is
created), and restoring the original state.
Change-Id: I3444720eae3ff1d388e39529cb3589f786ead0ec
---
M lib/mediawiki_selenium/environment.rb
M spec/api_helper_spec.rb
M spec/environment_spec.rb
3 files changed, 93 insertions(+), 148 deletions(-)
Approvals:
Dduvall: Looks good to me, approved
jenkins-bot: Verified
diff --git a/lib/mediawiki_selenium/environment.rb
b/lib/mediawiki_selenium/environment.rb
index 823919f..31d1ca1 100644
--- a/lib/mediawiki_selenium/environment.rb
+++ b/lib/mediawiki_selenium/environment.rb
@@ -9,9 +9,6 @@
class Environment
include Comparable
- attr_reader :config
- protected :config
-
class << self
attr_accessor :default_configuration
@@ -52,12 +49,8 @@
self.default_configuration = "environments.yml"
def initialize(*configs)
- @config = configs.map { |config| normalize_config(config)
}.reduce(:merge)
- @factory_cache = {}
- end
-
- def initialize_clone(other)
- @config = other.config.clone
+ @_config = configs.map { |config| normalize_config(config)
}.reduce(:merge)
+ @_factory_cache = {}
end
# Whether the given environment is equal to this one. Two environments are
@@ -68,7 +61,7 @@
# @return [Boolean]
#
def ==(other)
- @config == other.config
+ config == other.config
end
# Returns the configured value for the given env variable name.
@@ -92,7 +85,7 @@
# @yieldparam user [String] Alternative MediaWiki user.
# @yieldparam password [String] Alternative MediaWiki password.
#
- # @return [Environment]
+ # @return self
#
def as_user(id, &blk)
with_alternative([:mediawiki_user, password_variable], id, &blk)
@@ -115,7 +108,7 @@
def browser_factory(browser = browser_name)
browser = browser.to_s.downcase.to_sym
- @factory_cache[[remote?, browser]] ||= BrowserFactory.new(browser).tap
do |factory|
+ @_factory_cache[[remote?, browser]] ||= BrowserFactory.new(browser).tap
do |factory|
factory.bind(:_browser_session)
factory.extend(RemoteBrowserFactory) if remote?
end
@@ -174,7 +167,7 @@
#
# @yield [*args] Overridden browser configuration.
#
- # @return [Environment]
+ # @return self
#
def in_browser(id, overrides = {}, &blk)
overrides = overrides.each.with_object({}) do |(name, value), hash|
@@ -212,7 +205,7 @@
key = "#{key}_#{options[:id]}" if options.fetch(:id, nil)
key = normalize_key(key)
- value = @config[key]
+ value = config[key]
if value.nil? || value.to_s.empty?
if options.include?(:default)
@@ -256,7 +249,7 @@
# @yield [wiki_url]
# @yieldparam wiki_url [String] Alternative wiki URL.
#
- # @return [Environment]
+ # @return self
#
def on_wiki(id, &blk)
with_alternative(:mediawiki_url, id, &blk)
@@ -296,7 +289,7 @@
# @yieldparam browser [Watir::Browser] Browser object, before it's closed.
#
def teardown(status = :passed)
- @factory_cache.each do |_, factory|
+ @_factory_cache.each do |_, factory|
factory.each do |browser|
yield browser if block_given?
browser.close unless keep_browser_open?
@@ -351,12 +344,12 @@
# @yield [url]
# @yieldparam url [String] Wiki URL.
#
- # @return [Environment]
+ # @return self
#
- def visit_wiki(id, &blk)
+ def visit_wiki(id)
on_wiki(id) do |url|
browser.goto url
- instance_exec(url, &blk) unless blk.nil?
+ yield url if block_given?
end
end
@@ -413,10 +406,16 @@
#
# @yield [*args] Values of the overridden configuration.
#
- # @return [Environment] The modified environment.
+ # @return self
#
def with_alternative(names, id, &blk)
with(lookup_all(Array(names), id: id), &blk)
+ end
+
+ protected
+
+ def config
+ @_config
end
private
@@ -438,12 +437,15 @@
key.to_s.downcase.to_sym
end
- def with(overrides = {}, &blk)
+ def with(overrides = {})
overrides = normalize_config(overrides)
+ original_config = @_config.dup
- clone.tap do |env|
- env.config.merge!(overrides)
- env.instance_exec(*overrides.values, &blk) unless blk.nil?
+ begin
+ @_config = @_config.merge(overrides)
+ yield *overrides.values if block_given?
+ ensure
+ @_config = original_config
end
end
end
diff --git a/spec/api_helper_spec.rb b/spec/api_helper_spec.rb
index 33bece0..1035c80 100644
--- a/spec/api_helper_spec.rb
+++ b/spec/api_helper_spec.rb
@@ -44,35 +44,6 @@
it "returns a cached client" do
expect(subject).to be(client)
end
-
- context "from an altered environment" do
- subject { env2.api }
-
- let(:env2) { env.on_wiki(:b) }
-
- context "with the same API URL" do
- let(:alternative_api_url) { api_url }
-
- it "returns a cached client" do
- expect(subject).to be(client)
- end
- end
-
- context "with a different API URL" do
- let(:alternative_api_url) { "http://another.example/api"; }
-
- let(:client2) { double(MediawikiApi::Client) }
-
- before do
- expect(MediawikiApi::Client).to
receive(:new).with(alternative_api_url).and_return(client2)
- expect(client2).to receive(:log_in).with("mw user", "mw
password")
- end
-
- it "returns a new client" do
- expect(subject).not_to be(client)
- end
- end
- end
end
end
@@ -85,11 +56,12 @@
let(:alternative_api_url) { "http://another.example/api"; }
it "executes in the new environment using the alternative wiki and API
URL" do
- _ = self
+ expect { |block| env.on_wiki(:b, &block) }.
+ to yield_with_args(alternative_wiki_url, alternative_api_url)
- env.on_wiki(:b) do |wiki_url, api_url|
- _.expect(wiki_url).to _.eq(_.alternative_wiki_url)
- _.expect(api_url).to _.eq(_.alternative_api_url)
+ env.on_wiki(:b) do
+ expect(env[:mediawiki_url]).to eq(alternative_wiki_url)
+ expect(env[:mediawiki_api_url]).to eq(alternative_api_url)
end
end
end
@@ -97,12 +69,13 @@
context "and no explicit API URL is configured for the wiki" do
let(:alternative_api_url) { "" }
- it "constructs one relative to the wiki URL" do
- _ = self
+ it "constructs one at /w/api.php relative to the wiki URL" do
+ expect { |block| env.on_wiki(:b, &block) }.
+ to yield_with_args(alternative_wiki_url,
"http://another.example/w/api.php";)
- env.on_wiki(:b) do |wiki_url, api_url|
- _.expect(wiki_url).to _.eq(_.alternative_wiki_url)
- _.expect(api_url).to _.eq("http://another.example/w/api.php";)
+ env.on_wiki(:b) do
+ expect(env[:mediawiki_url]).to eq(alternative_wiki_url)
+ expect(env[:mediawiki_api_url]).to
eq("http://another.example/w/api.php";)
end
end
end
diff --git a/spec/environment_spec.rb b/spec/environment_spec.rb
index a3218d9..b417314 100644
--- a/spec/environment_spec.rb
+++ b/spec/environment_spec.rb
@@ -4,7 +4,7 @@
describe Environment do
subject { env }
- let(:env) { Environment.new(config).extend(ApiHelper) }
+ let(:env) { Environment.new(config) }
let(:config) { minimum_config }
let(:minimum_config) do
@@ -92,8 +92,6 @@
end
describe "#as_user" do
- subject { env.as_user(:b) {} }
-
let(:config) do
{
mediawiki_user: "user",
@@ -103,21 +101,13 @@
}
end
- let(:new_env) { double(Environment) }
- let(:new_config) { double(Hash) }
-
- before do
- expect(env).to receive(:clone).and_return(new_env)
- expect(new_env).to receive(:config).and_return(new_config)
- end
-
it "executes in the new environment for the alternative user and its
password" do
- expect(new_config).to receive(:merge!).with(
- mediawiki_user: "user b",
- mediawiki_password: "pass b",
- )
- expect(new_env).to receive(:instance_exec).with("user b", "pass b")
- subject
+ expect { |block| env.as_user(:b, &block) }.to yield_with_args("user
b", "pass b")
+
+ env.as_user(:b) do
+ expect(env[:mediawiki_user]).to eq("user b")
+ expect(env[:mediawiki_password]).to eq("pass b")
+ end
end
end
@@ -220,32 +210,23 @@
end
describe "#in_browser" do
- subject { env.in_browser(id, overrides) {} }
-
- let(:id) { :a }
- let(:overrides) { {} }
-
- let(:new_env) { double(Environment) }
- let(:new_config) { double(Hash) }
-
- before do
- expect(env).to receive(:clone).and_return(new_env)
- expect(new_env).to receive(:config).and_return(new_config)
- end
-
it "executes in the new environment with a new browser session" do
- expect(new_config).to receive(:merge!).with(_browser_session: id)
- expect(new_env).to receive(:instance_exec).with(id)
- subject
+ expect { |block| env.in_browser(:a, &block) }.to yield_with_args(:a)
+
+ env.in_browser(:a) do
+ expect(env[:_browser_session]).to eq(:a)
+ end
end
context "given browser configuration overrides" do
- let(:overrides) { { language: "eo" } }
-
it "executes in the new environment with the prefixed overrides" do
- expect(new_config).to receive(:merge!).with(_browser_session: id,
browser_language: "eo")
- expect(new_env).to receive(:instance_exec).with("eo", id)
- subject
+ expect { |block| env.in_browser(:a, language: "eo", &block) }.
+ to yield_with_args("eo", :a)
+
+ env.in_browser(:a, language: "eo") do
+ expect(env[:browser_language]).to eq("eo")
+ expect(env[:_browser_session]).to eq(:a)
+ end
end
end
end
@@ -313,10 +294,6 @@
end
describe "#on_wiki" do
- subject { env.on_wiki(id) {} }
-
- let(:id) { :b }
-
let(:config) do
{
mediawiki_url: "http://an.example/wiki";,
@@ -325,18 +302,12 @@
}
end
- let(:new_env) { double(Environment) }
- let(:new_config) { double(Hash) }
-
- before do
- expect(env).to receive(:clone).and_return(new_env)
- expect(new_env).to receive(:config).and_return(new_config)
- end
-
it "executes in the new environment using the alternative wiki URL" do
- expect(new_config).to receive(:merge!).with(mediawiki_url:
"http://altb.example/wiki";)
- expect(new_env).to
receive(:instance_exec).with("http://altb.example/wiki";)
- subject
+ expect { |block| env.on_wiki(:b, &block) }.to
yield_with_args("http://altb.example/wiki";)
+
+ env.on_wiki(:b) do
+ expect(env[:mediawiki_url]).to eq("http://altb.example/wiki";)
+ end
end
end
@@ -387,7 +358,7 @@
describe "#wiki_url" do
subject { env.wiki_url(url) }
- let(:env) { Environment.new(mediawiki_url:
"http://an.example/wiki/";).extend(ApiHelper) }
+ let(:env) { Environment.new(mediawiki_url: "http://an.example/wiki/";) }
context "with no given url" do
let(:url) { nil }
@@ -432,50 +403,49 @@
end
describe "#with_alternative" do
- subject { env.with_alternative(names, id) {} }
-
let(:config) do
{
- mediawiki_url: "http://an.example/wiki";,
- mediawiki_url_b: "http://alt.example/wiki";,
- mediawiki_api_url: "http://an.example/api";,
- mediawiki_api_url_b: "http://alt.example/api";,
+ mediawiki_url: "http://a.example/wiki";,
+ mediawiki_url_b: "http://b.example/wiki";,
+ mediawiki_api_url: "http://a.example/api";,
+ mediawiki_api_url_b: "http://b.example/api";,
}
end
- let(:new_env) { double(Environment) }
- let(:new_config) { double(Hash) }
-
- before do
- expect(env).to receive(:clone).and_return(new_env)
- expect(new_env).to receive(:config).and_return(new_config)
- end
-
context "given one option name and an ID" do
- let(:names) { :mediawiki_url }
- let(:id) { :b }
-
it "executes in the new environment that substitutes it using the
alternative" do
- expect(new_config).to receive(:merge!).with(mediawiki_url:
"http://alt.example/wiki";)
- expect(new_env).to
receive(:instance_exec).with("http://alt.example/wiki";)
- subject
+ expect { |block| env.with_alternative(:mediawiki_url, :b, &block) }.
+ to yield_with_args("http://b.example/wiki";)
+
+ env.with_alternative(:mediawiki_url, :b) do
+ expect(env[:mediawiki_url]).to eq("http://b.example/wiki";)
+ end
end
end
context "given multiple option names and an ID" do
- let(:names) { [:mediawiki_url, :mediawiki_api_url] }
- let(:id) { :b }
-
it "executes in the new environment that substitutes both using the
alternatives" do
- expect(new_config).to receive(:merge!).with(
- mediawiki_url: "http://alt.example/wiki";,
- mediawiki_api_url: "http://alt.example/api";
- )
- expect(new_env).to receive(:instance_exec).with(
- "http://alt.example/wiki";,
- "http://alt.example/api";
- )
- subject
+ expect { |block| env.with_alternative([:mediawiki_url,
:mediawiki_api_url], :b, &block) }.
+ to yield_with_args("http://b.example/wiki";, "http://b.example/api";)
+
+ env.with_alternative([:mediawiki_url, :mediawiki_api_url], :b) do
+ expect(env[:mediawiki_url]).to eq("http://b.example/wiki";)
+ expect(env[:mediawiki_api_url]).to eq("http://b.example/api";)
+ end
+ end
+ end
+
+ context "following block evaluation" do
+ it "restores the original configuration" do
+ env.with_alternative(:mediawiki_url, :b)
+ expect(env[:mediawiki_url]).to eq("http://a.example/wiki";)
+ end
+
+ context "when an exception is raised within the block" do
+ it "restores the original configuration and lets the exception be
raised" do
+ expect { env.with_alternative(:mediawiki_url, :b) { raise "error"
} }.to raise_error
+ expect(env[:mediawiki_url]).to eq("http://a.example/wiki";)
+ end
end
end
end
--
To view, visit https://gerrit.wikimedia.org/r/180985
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3444720eae3ff1d388e39529cb3589f786ead0ec
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/selenium
Gerrit-Branch: env-abstraction-layer
Gerrit-Owner: Dduvall <[email protected]>
Gerrit-Reviewer: Cmcmahon <[email protected]>
Gerrit-Reviewer: Dduvall <[email protected]>
Gerrit-Reviewer: Hashar <[email protected]>
Gerrit-Reviewer: Zfilipin <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
