Dduvall has uploaded a new change for review.

  https://gerrit.wikimedia.org/r/230230

Change subject: Decouple screenshot-ing and artifacts from Cucumber hooks
......................................................................

Decouple screenshot-ing and artifacts from Cucumber hooks

Created `ScreenshotHelper` to encapsulate and generalize the
screenshot-ing previously implemented directly in a Cucumber `After`
hook.

Refactored `Environment#teardown` and friends to return artifacts in the
form `{ path => mime_type }` which can then be logged in the way
appropriate to the test runner.

Moved logging of SauceLabs job URL to a `RemoteBrowserFactory#teardown`
and refactored `HeadlessHelper` for the new artifacts system.

Bug: T108273
Change-Id: I4534fcb05180419ee9aa354f12616aadec160659
---
A features/screenshots.feature
A features/step_definitions/screenshot_steps.rb
M lib/mediawiki_selenium.rb
M lib/mediawiki_selenium/browser_factory/base.rb
M lib/mediawiki_selenium/environment.rb
M lib/mediawiki_selenium/remote_browser_factory.rb
M lib/mediawiki_selenium/support/env.rb
M lib/mediawiki_selenium/support/hooks.rb
M lib/mediawiki_selenium/support/modules/headless_helper.rb
A lib/mediawiki_selenium/support/modules/screenshot_helper.rb
A spec/screenshot_helper_spec.rb
11 files changed, 230 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.wikimedia.org:29418/mediawiki/selenium 
refs/changes/30/230230/1

diff --git a/features/screenshots.feature b/features/screenshots.feature
new file mode 100644
index 0000000..84be3f7
--- /dev/null
+++ b/features/screenshots.feature
@@ -0,0 +1,21 @@
+@integration
+Feature: Screenshots of failed scenarios
+
+  As a developer writing and running tests, it would be helpful to have a
+  screenshot of the browser window at the point where each scenario has
+  failed.
+
+  Background:
+    Given I have configured my environment with:
+      """
+      screenshot_failures: true
+      screenshot_failures_path: tmp/screenshots
+      """
+      And the "tmp/screenshots" directory exists
+      And I am using the screenshot helper
+
+  Scenario: A screenshot is taken for failed scenarios
+    Given the current scenario name is "Some scenario"
+      And I have started a browser
+    When the scenario fails
+    Then the file "tmp/screenshots/Some scenario.png" should exist
diff --git a/features/step_definitions/screenshot_steps.rb 
b/features/step_definitions/screenshot_steps.rb
new file mode 100644
index 0000000..52fedbd
--- /dev/null
+++ b/features/step_definitions/screenshot_steps.rb
@@ -0,0 +1,3 @@
+Given(/^I am using the screenshot helper$/) do
+  @env.extend(MediawikiSelenium::ScreenshotHelper)
+end
diff --git a/lib/mediawiki_selenium.rb b/lib/mediawiki_selenium.rb
index 505c50a..ee3a256 100644
--- a/lib/mediawiki_selenium.rb
+++ b/lib/mediawiki_selenium.rb
@@ -9,6 +9,7 @@
   autoload :PageFactory, 'mediawiki_selenium/page_factory'
   autoload :Raita, 'mediawiki_selenium/raita'
   autoload :RemoteBrowserFactory, 'mediawiki_selenium/remote_browser_factory'
+  autoload :ScreenshotHelper, 
'mediawiki_selenium/support/modules/screenshot_helper'
   autoload :StrictPending, 'mediawiki_selenium/support/modules/strict_pending'
   autoload :UserFactory, 'mediawiki_selenium/user_factory'
   autoload :UserFactoryHelper, 
'mediawiki_selenium/support/modules/user_factory_helper'
diff --git a/lib/mediawiki_selenium/browser_factory/base.rb 
b/lib/mediawiki_selenium/browser_factory/base.rb
index 8684bfd..70a6815 100644
--- a/lib/mediawiki_selenium/browser_factory/base.rb
+++ b/lib/mediawiki_selenium/browser_factory/base.rb
@@ -200,8 +200,12 @@
       # @param _env [Environment] Environment.
       # @param _status [Symbol] Status of the executed scenario.
       #
+      # @return [Hash{String => String}] Artifacts.
+      #
+      # @see Environment#teardown
+      #
       def teardown(_env, _status)
-        # abstract
+        {}
       end
 
       protected
diff --git a/lib/mediawiki_selenium/environment.rb 
b/lib/mediawiki_selenium/environment.rb
index f772707..61b9460 100644
--- a/lib/mediawiki_selenium/environment.rb
+++ b/lib/mediawiki_selenium/environment.rb
@@ -359,9 +359,13 @@
     # Executes teardown tasks including instructing all browser factories to
     # close any open browsers and perform their own teardown tasks.
     #
+    # Teardown tasks may produce artifacts, which will be returned in the form
+    # `{ path => mime_type }`.
+    #
     # @example Teardown environment resources after each scenario completes
     #   After do |scenario|
-    #     teardown(name: scenario.name, status: scenario.status)
+    #     artifacts = teardown(name: scenario.name, status: scenario.status)
+    #     artifacts.each { |path, mime_type| embed(path, mime_type) }
     #   end
     #
     # @param info [Hash] Hash of test case information.
@@ -369,15 +373,22 @@
     # @yield [browser]
     # @yieldparam browser [Watir::Browser] Browser object, before it's closed.
     #
+    # @return [Hash{String => String}] Hash of path/mime-type artifacts.
+    #
     def teardown(info = {})
+      artifacts = {}
+
       @_factory_cache.each do |(_, browser_name), factory|
         factory.each do |browser|
           yield browser if block_given?
           browser.close unless keep_browser_open? && browser_name != :phantomjs
         end
 
-        factory.teardown(self, info[:status] || :passed)
+        factory_artifacts = factory.teardown(self, info[:status] || :passed)
+        artifacts.merge!(factory_artifacts) if factory_artifacts.is_a?(Hash)
       end
+
+      {}
     end
 
     # Returns the current value for `:mediawiki_user` or the value for the
diff --git a/lib/mediawiki_selenium/remote_browser_factory.rb 
b/lib/mediawiki_selenium/remote_browser_factory.rb
index 6756891..64bcaf8 100644
--- a/lib/mediawiki_selenium/remote_browser_factory.rb
+++ b/lib/mediawiki_selenium/remote_browser_factory.rb
@@ -42,6 +42,8 @@
     # Submits status and Jenkins build info to Sauce Labs.
     #
     def teardown(env, status)
+      artifacts = super
+
       each do |browser|
         sid = browser.driver.session_id
         url = browser.driver.send(:bridge).http.send(:server_url)
@@ -62,7 +64,11 @@
         )
 
         Cucumber::Formatter::Sauce.current_session_id = sid
+
+        artifacts["http://saucelabs.com/jobs/#{sid}";] = 'text/url'
       end
+
+      artifacts
     end
 
     protected
diff --git a/lib/mediawiki_selenium/support/env.rb 
b/lib/mediawiki_selenium/support/env.rb
index a4d50e4..a6ec7af 100644
--- a/lib/mediawiki_selenium/support/env.rb
+++ b/lib/mediawiki_selenium/support/env.rb
@@ -6,5 +6,6 @@
 
 World(MediawikiSelenium::ApiHelper)
 World(MediawikiSelenium::PageFactory)
+World(MediawikiSelenium::ScreenshotHelper)
 World(MediawikiSelenium::StrictPending)
 World(MediawikiSelenium::UserFactoryHelper)
diff --git a/lib/mediawiki_selenium/support/hooks.rb 
b/lib/mediawiki_selenium/support/hooks.rb
index 8618742..70cff16 100644
--- a/lib/mediawiki_selenium/support/hooks.rb
+++ b/lib/mediawiki_selenium/support/hooks.rb
@@ -56,7 +56,7 @@
   end
 end
 
-Before do |scenario|
+Before do
   # Create a unique random string for this scenario
   @random_string = Random.new.rand.to_s
 
@@ -75,26 +75,12 @@
 end
 
 After do |scenario|
-  if scenario.respond_to?(:status)
-    require 'fileutils'
-
-    teardown(name: @scenario_name, status: scenario.status) do |browser|
-      # Embed remote session URLs
-      if remote? && browser.driver.respond_to?(:session_id)
-        embed("http://saucelabs.com/jobs/#{browser.driver.session_id}";, 
'text/url')
-      end
-
-      # Take screenshots
-      if scenario.failed? && lookup(:screenshot_failures, default: false) == 
'true'
-        screen_dir = lookup(:screenshot_failures_path, default: 'screenshots')
-        FileUtils.mkdir_p screen_dir
-        name = @scenario_name.gsub(/ /, '_')
-        path = "#{screen_dir}/#{name}.png"
-        browser.screenshot.save path
-        embed path, 'image/png'
-      end
+  artifacts =
+    if scenario.respond_to?(:status)
+      teardown(name: @scenario_name, status: scenario.status)
+    else
+      teardown(name: @scenario_name)
     end
-  else
-    teardown(name: @scenario_name)
-  end
+
+  artifacts.each { |path, mime_type| embed(path, mime_type) }
 end
diff --git a/lib/mediawiki_selenium/support/modules/headless_helper.rb 
b/lib/mediawiki_selenium/support/modules/headless_helper.rb
index 87f0050..0683d9e 100644
--- a/lib/mediawiki_selenium/support/modules/headless_helper.rb
+++ b/lib/mediawiki_selenium/support/modules/headless_helper.rb
@@ -96,21 +96,29 @@
     # @see Environment#teardown
     #
     def teardown(info = {})
-      super
-    ensure
-      if @_headless_capture
-        if info[:status] == :failed
-          dir = File.absolute_path(headless_capture_path)
-          FileUtils.mkdir_p(dir)
+      artifacts = {}
 
-          filename = "#{(info[:name] || 
'scenario').tr("#{File::SEPARATOR}\000", '-')}.mp4"
-          filename = File.join(dir, filename)
+      begin
+        artifacts = super
+      ensure
+        if @_headless_capture
+          if info[:status] == :failed
+            dir = File.absolute_path(headless_capture_path)
+            FileUtils.mkdir_p(dir)
 
-          @_headless_display.video.stop_and_save(filename)
-        else
-          @_headless_display.video.stop_and_discard
+            filename = "#{(info[:name] || 
'scenario').tr("#{File::SEPARATOR}\000", '-')}.mp4"
+            filename = File.join(dir, filename)
+
+            @_headless_display.video.stop_and_save(filename)
+
+            artifacts[filename] = 'video/mp4'
+          else
+            @_headless_display.video.stop_and_discard
+          end
         end
       end
+
+      artifacts
     end
   end
 end
diff --git a/lib/mediawiki_selenium/support/modules/screenshot_helper.rb 
b/lib/mediawiki_selenium/support/modules/screenshot_helper.rb
new file mode 100644
index 0000000..dd2a5bd
--- /dev/null
+++ b/lib/mediawiki_selenium/support/modules/screenshot_helper.rb
@@ -0,0 +1,58 @@
+require 'fileutils'
+
+module MediawikiSelenium
+  # Adds support to {Environment} for taking screenshots of the current
+  # browser window. If `screenshot_failures` is set to `true` for the current
+  # environment, one is automatically taken at the end of each failed
+  # test case.
+  #
+  module ScreenshotHelper
+    # Takes a screenshot of the given browser window and returns the path
+    # to the saved image.
+    #
+    # @param browser [Watir::Browser] Browser to take a sceeenshot of.
+    # @param name [String] Base name to use for the saved image file.
+    #
+    # @return [String] Path to the new screenshot.
+    #
+    def screenshot(browser, name)
+      dir = screenshot_failures_path
+      FileUtils.mkdir_p dir
+
+      name = name.tr("#{File::SEPARATOR}\000", '-')
+
+      path = File.join(dir, "#{name}.png")
+      browser.screenshot.save(path)
+
+      path
+    end
+
+    # Takes screenshots for failed tests.
+    #
+    # @param info [Hash] Test case information.
+    #
+    def teardown(info = {})
+      screenshots = []
+
+      artifacts = super(info) do |browser|
+        if info[:status] == :failed && screenshot_failures?
+          screenshots << screenshot(browser, info[:name] || 'scenario')
+        end
+
+        yield browser if block_given?
+      end
+
+      artifacts.merge(screenshots.each.with_object({}) { |img, arts| arts[img] 
= 'image/png' })
+    end
+
+    private
+
+    def screenshot_failures?
+      lookup(:screenshot_failures, default: false).to_s == 'true'
+    end
+
+    def screenshot_failures_path
+      lookup(:screenshot_failures_path, default: 'screenshots')
+    end
+  end
+end
diff --git a/spec/screenshot_helper_spec.rb b/spec/screenshot_helper_spec.rb
new file mode 100644
index 0000000..759bd9a
--- /dev/null
+++ b/spec/screenshot_helper_spec.rb
@@ -0,0 +1,95 @@
+require 'spec_helper'
+
+module MediawikiSelenium
+  describe ScreenshotHelper do
+    let(:env) { Environment.new(config).extend(ScreenshotHelper) }
+
+    let(:config) { {} }
+
+    describe '#screenshot' do
+      subject { env.screenshot(browser, name) }
+
+      let(:config) { { screenshot_failures_path: 'screenshots' } }
+
+      let(:browser) { double('Watir::Browser') }
+      let(:name) { 'Some test name' }
+
+      it 'takes a screenshot of the browser' do
+        screenshot = double('Watir::Screenshot')
+        expect(browser).to receive(:screenshot).and_return(screenshot)
+        expect(screenshot).to receive(:save).with('screenshots/Some test 
name.png')
+
+        subject
+      end
+
+      it 'returns the path to the image file' do
+        screenshot = double('Watir::Screenshot')
+        allow(browser).to receive(:screenshot).and_return(screenshot)
+        allow(screenshot).to receive(:save)
+
+        expect(subject).to eq('screenshots/Some test name.png')
+      end
+    end
+
+    describe '#teardown' do
+      subject { env.teardown(info) }
+
+      let(:info) { { name: 'Some test name', status: status } }
+      let(:status) { :passed }
+
+      let(:browser) { double(Watir::Browser) }
+
+      before do
+        # mock {Environment#teardown}
+        allow(env.browser_factory).to receive(:each) { |&blk| 
[browser].each(&blk) }
+        allow(env.browser_factory).to receive(:teardown)
+        allow(browser).to receive(:close)
+      end
+
+      it 'invokes the given block with each browser' do
+        allow(env).to receive(:screenshot)
+
+        expect { |blk| env.teardown(info, &blk) }.to yield_with_args(browser)
+      end
+
+      context 'configured to take screenshots of failures' do
+        let(:config) { { screenshot_failures: true } }
+
+        context 'when the test case has failed' do
+          let(:status) { :failed }
+
+          it 'takes a screenshot of each browser' do
+            expect(env).to receive(:screenshot).with(browser, 'Some test name')
+            subject
+          end
+
+          it 'includes each screenshot in the returned artifacts' do
+            allow(env).to receive(:screenshot).and_return('Some test name.png')
+
+            expect(subject).to include('Some test name.png')
+            expect(subject['Some test name.png']).to eq('image/png')
+          end
+        end
+
+        context 'when the test case has not failed' do
+          let(:status) { :passed }
+
+          it 'takes no screenshot' do
+            expect(env).not_to receive(:screenshot)
+            subject
+          end
+        end
+      end
+
+      context 'configured to not take screenshots of failures' do
+        let(:config) { { screenshot_failures: false } }
+        let(:status) { :failed }
+
+        it 'takes no screenshot' do
+          expect(env).not_to receive(:screenshot)
+          subject
+        end
+      end
+    end
+  end
+end

-- 
To view, visit https://gerrit.wikimedia.org/r/230230
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4534fcb05180419ee9aa354f12616aadec160659
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/selenium
Gerrit-Branch: master
Gerrit-Owner: Dduvall <dduv...@wikimedia.org>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to