Merge branch 'qa-extend-click-visit-actions' into 'master'

Implement dynamic validation on QA Pages

Closes gitlab-qa#280

See merge request gitlab-org/gitlab-ce!25704

14 files changed, 145 insertions, 46 deletions

diff --git a/qa/qa.rb b/qa/qa.rb
index f580691f952..944dcc31917 100644
--- a/qa/qa.rb
+++ b/qa/qa.rb
@@ -130,6 +130,7 @@ module QA
     autoload :View, 'qa/page/view'
     autoload :Element, 'qa/page/element'
     autoload :Validator, 'qa/page/validator'
+    autoload :Validatable, 'qa/page/validatable'
 
     module Main
       autoload :Login, 'qa/page/main/login'
diff --git a/qa/qa/ce/strategy.rb b/qa/qa/ce/strategy.rb
index d7748a976f0..7e2d02424fe 100644
--- a/qa/qa/ce/strategy.rb
+++ b/qa/qa/ce/strategy.rb
@@ -13,7 +13,6 @@ module QA
         # The login page could take some time to load the first time it is visited.
         # We visit the login page and wait for it to properly load only once before the tests.
         QA::Runtime::Browser.visit(:gitlab, QA::Page::Main::Login)
-        QA::Page::Main::Login.perform(&:assert_page_loaded)
       end
     end
   end
diff --git a/qa/qa/page/base.rb b/qa/qa/page/base.rb
index c395e5f6011..389f4e0032e 100644
--- a/qa/qa/page/base.rb
+++ b/qa/qa/page/base.rb
@@ -8,6 +8,7 @@ module QA
       prepend Support::Page::Logging if Runtime::Env.debug?
       include Capybara::DSL
       include Scenario::Actable
+      extend Validatable
       extend SingleForwardable
 
       ElementNotFound = Class.new(RuntimeError)
@@ -93,8 +94,10 @@ module QA
         find_element(name).set(false)
       end
 
-      def click_element(name)
+      # replace with (..., page = self.class)
+      def click_element(name, page = nil)
         find_element(name).click
+        page.validate_elements_present! if page
       end
 
       def fill_element(name, content)
diff --git a/qa/qa/page/element.rb b/qa/qa/page/element.rb
index d92e71467fe..7a01320901d 100644
--- a/qa/qa/page/element.rb
+++ b/qa/qa/page/element.rb
@@ -1,28 +1,41 @@
 # frozen_string_literal: true
 
+require 'active_support/core_ext/array/extract_options'
+
 module QA
   module Page
     class Element
-      attr_reader :name
+      attr_reader :name, :attributes
 
-      def initialize(name, pattern = nil)
+      def initialize(name, *options)
         @name = name
-        @pattern = pattern || selector
+        @attributes = options.extract_options!
+        @attributes[:pattern] ||= selector
+
+        options.each do |option|
+          if option.is_a?(String) || option.is_a?(Regexp)
+            @attributes[:pattern] = option
+          end
+        end
       end
 
       def selector
         "qa-#{@name.to_s.tr('_', '-')}"
       end
 
+      def required?
+        !!@attributes[:required]
+      end
+
       def selector_css
         ".#{selector}"
       end
 
       def expression
-        if @pattern.is_a?(String)
-          @_regexp ||= Regexp.new(Regexp.escape(@pattern))
+        if @attributes[:pattern].is_a?(String)
+          @_regexp ||= Regexp.new(Regexp.escape(@attributes[:pattern]))
         else
-          @pattern
+          @attributes[:pattern]
         end
       end
 
diff --git a/qa/qa/page/main/login.rb b/qa/qa/page/main/login.rb
index 99b3d1b83d3..8970eeb6678 100644
--- a/qa/qa/page/main/login.rb
+++ b/qa/qa/page/main/login.rb
@@ -39,19 +39,7 @@ module QA
         end
 
         view 'app/views/layouts/devise.html.haml' do
-          element :login_page
-        end
-
-        def assert_page_loaded
-          unless page_loaded?
-            raise QA::Runtime::Browser::NotRespondingError, "Login page did not load at #{QA::Page::Main::Login.perform(&:current_url)}"
-          end
-        end
-
-        def page_loaded?
-          wait(max: 60) do
-            has_element?(:login_page)
-          end
+          element :login_page, required: true
         end
 
         def sign_in_using_credentials(user = nil)
@@ -159,7 +147,7 @@ module QA
 
           fill_element :login_field, user.username
           fill_element :password_field, user.password
-          click_element :sign_in_button
+          click_element :sign_in_button, Page::Main::Menu
         end
 
         def set_initial_password_if_present
diff --git a/qa/qa/page/main/menu.rb b/qa/qa/page/main/menu.rb
index e98d531c86e..5eb24d2d2ba 100644
--- a/qa/qa/page/main/menu.rb
+++ b/qa/qa/page/main/menu.rb
@@ -10,15 +10,15 @@ module QA
         end
 
         view 'app/views/layouts/header/_default.html.haml' do
-          element :navbar
-          element :user_avatar
+          element :navbar, required: true
+          element :user_avatar, required: true
           element :user_menu, '.dropdown-menu' # rubocop:disable QA/ElementWithPattern
         end
 
         view 'app/views/layouts/nav/_dashboard.html.haml' do
           element :admin_area_link
-          element :projects_dropdown
-          element :groups_dropdown
+          element :projects_dropdown, required: true
+          element :groups_dropdown, required: true
           element :snippets_link
         end
 
diff --git a/qa/qa/page/validatable.rb b/qa/qa/page/validatable.rb
new file mode 100644
index 00000000000..8467d261285
--- /dev/null
+++ b/qa/qa/page/validatable.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+module QA
+  module Page
+    module Validatable
+      PageValidationError = Class.new(StandardError)
+
+      def validate_elements_present!
+        base_page = self.new
+
+        elements.each do |element|
+          next unless element.required?
+
+          # TODO: this wait needs to be replaced by the wait class
+          unless base_page.has_element?(element.name, wait: 60)
+            raise Validatable::PageValidationError, "#{element.name} did not appear on #{self.name} as expected"
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/qa/qa/page/view.rb b/qa/qa/page/view.rb
index 96f3917a8ab..613059b2d32 100644
--- a/qa/qa/page/view.rb
+++ b/qa/qa/page/view.rb
@@ -50,8 +50,8 @@ module QA
           @elements = []
         end
 
-        def element(name, pattern = nil)
-          @elements.push(Page::Element.new(name, pattern))
+        def element(name, *args)
+          @elements.push(Page::Element.new(name, *args))
         end
       end
     end
diff --git a/qa/qa/runtime/browser.rb b/qa/qa/runtime/browser.rb
index 174a52bd376..3bf4b3bbbfb 100644
--- a/qa/qa/runtime/browser.rb
+++ b/qa/qa/runtime/browser.rb
@@ -33,6 +33,7 @@ module QA
 
       def self.visit(address, page = nil, &block)
         new.visit(address, page, &block)
+        page.validate_elements_present!
       end
 
       def self.configure!
diff --git a/qa/qa/specs/features/browser_ui/1_manage/login/log_into_mattermost_via_gitlab_spec.rb b/qa/qa/specs/features/browser_ui/1_manage/login/log_into_mattermost_via_gitlab_spec.rb
index b1d641b507f..67610b62ed7 100644
--- a/qa/qa/specs/features/browser_ui/1_manage/login/log_into_mattermost_via_gitlab_spec.rb
+++ b/qa/qa/specs/features/browser_ui/1_manage/login/log_into_mattermost_via_gitlab_spec.rb
@@ -4,16 +4,14 @@ module QA
   context 'Manage', :orchestrated, :mattermost do
     describe 'Mattermost login' do
       it 'user logs into Mattermost using GitLab OAuth' do
-        Runtime::Browser.visit(:gitlab, Page::Main::Login) do
-          Page::Main::Login.act { sign_in_using_credentials }
+        Runtime::Browser.visit(:gitlab, Page::Main::Login)
+        Page::Main::Login.perform(&:sign_in_using_credentials)
 
-          Runtime::Browser.visit(:mattermost, Page::Mattermost::Login) do
-            Page::Mattermost::Login.act { sign_in_using_oauth }
+        Runtime::Browser.visit(:mattermost, Page::Mattermost::Login)
+        Page::Mattermost::Login.perform(&:sign_in_using_oauth)
 
-            Page::Mattermost::Main.perform do |page|
-              expect(page).to have_content(/(Welcome to: Mattermost|Logout GitLab Mattermost)/)
-            end
-          end
+        Page::Mattermost::Main.perform do |page|
+          expect(page).to have_content(/(Welcome to: Mattermost|Logout GitLab Mattermost)/)
         end
       end
     end
diff --git a/qa/qa/support/page/logging.rb b/qa/qa/support/page/logging.rb
index ff505fdbddd..3fe567d7757 100644
--- a/qa/qa/support/page/logging.rb
+++ b/qa/qa/support/page/logging.rb
@@ -56,8 +56,11 @@ module QA
           elements
         end
 
-        def click_element(name)
-          log("clicking :#{name}")
+        def click_element(name, page = nil)
+          msg = ["clicking :#{name}"]
+          msg << ", expecting to be at #{page.class}" if page
+
+          log(msg.compact.join(' '))
 
           super
         end
diff --git a/qa/spec/page/element_spec.rb b/qa/spec/page/element_spec.rb
index d5d6dff69da..f746fe06e40 100644
--- a/qa/spec/page/element_spec.rb
+++ b/qa/spec/page/element_spec.rb
@@ -50,4 +50,60 @@ describe QA::Page::Element do
       expect(subject.matches?('some_name selector')).to be false
     end
   end
+
+  describe 'attributes' do
+    context 'element with no args' do
+      subject { described_class.new(:something) }
+
+      it 'defaults pattern to #selector' do
+        expect(subject.attributes[:pattern]).to eq 'qa-something'
+        expect(subject.attributes[:pattern]).to eq subject.selector
+      end
+
+      it 'is not required by default' do
+        expect(subject.required?).to be false
+      end
+    end
+
+    context 'element with a pattern' do
+      subject { described_class.new(:something, /link_to 'something'/) }
+
+      it 'has an attribute[pattern] of the pattern' do
+        expect(subject.attributes[:pattern]).to eq /link_to 'something'/
+      end
+
+      it 'is not required by default' do
+        expect(subject.required?).to be false
+      end
+    end
+
+    context 'element with requirement; no pattern' do
+      subject { described_class.new(:something, required: true) }
+
+      it 'has an attribute[pattern] of the selector' do
+        expect(subject.attributes[:pattern]).to eq 'qa-something'
+        expect(subject.attributes[:pattern]).to eq subject.selector
+      end
+
+      it 'is required' do
+        expect(subject.required?).to be true
+      end
+    end
+
+    context 'element with requirement and pattern' do
+      subject { described_class.new(:something, /link_to 'something_else_entirely'/, required: true) }
+
+      it 'has an attribute[pattern] of the passed pattern' do
+        expect(subject.attributes[:pattern]).to eq /link_to 'something_else_entirely'/
+      end
+
+      it 'is required' do
+        expect(subject.required?).to be true
+      end
+
+      it 'has a selector of the name' do
+        expect(subject.selector).to eq 'qa-something'
+      end
+    end
+  end
 end
diff --git a/rubocop/cop/qa/element_with_pattern.rb b/rubocop/cop/qa/element_with_pattern.rb
index 9d80946f1ba..d14eeaaeaf3 100644
--- a/rubocop/cop/qa/element_with_pattern.rb
+++ b/rubocop/cop/qa/element_with_pattern.rb
@@ -1,18 +1,21 @@
+# frozen_string_literal: true
+
 require_relative '../../qa_helpers'
 
 module RuboCop
   module Cop
     module QA
-      # This cop checks for the usage of factories in migration specs
+      # This cop checks for the usage of patterns in QA elements
       #
       # @example
       #
       #   # bad
-      #   let(:user) { create(:user) }
+      #   element :some_element, "link_to 'something'"
+      #   element :some_element, /link_to 'something'/
       #
       #   # good
-      #   let(:users) { table(:users) }
-      #   let(:user) { users.create!(name: 'User 1', username: 'user1') }
+      #   element :some_element
+      #   element :some_element, required: true
       class ElementWithPattern < RuboCop::Cop::Cop
         include QAHelpers
 
@@ -22,10 +25,13 @@ module RuboCop
           return unless in_qa_file?(node)
           return unless method_name(node).to_s == 'element'
 
-          element_name, pattern = node.arguments
-          return unless pattern
+          element_name, *args = node.arguments
+
+          return if args.first.nil?
 
-          add_offense(node, location: pattern.source_range, message: MESSAGE % "qa-#{element_name.value.to_s.tr('_', '-')}")
+          args.first.each_node(:str) do |arg|
+            add_offense(arg, message: MESSAGE % "qa-#{element_name.value.to_s.tr('_', '-')}")
+          end
         end
 
         private
diff --git a/spec/rubocop/cop/qa/element_with_pattern_spec.rb b/spec/rubocop/cop/qa/element_with_pattern_spec.rb
index c5beb40f9fd..ef20d9a1f26 100644
--- a/spec/rubocop/cop/qa/element_with_pattern_spec.rb
+++ b/spec/rubocop/cop/qa/element_with_pattern_spec.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 require 'rubocop'
@@ -23,7 +25,7 @@ describe RuboCop::Cop::QA::ElementWithPattern do
         element :groups_filter, 'search_field_tag :filter'
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't use a pattern for element, create a corresponding `qa-groups-filter` instead.
         element :groups_filter_placeholder, /Search by name/
-                                            ^^^^^^^^^^^^^^^^ Don't use a pattern for element, create a corresponding `qa-groups-filter-placeholder` instead.
+                                             ^^^^^^^^^^^^^^ Don't use a pattern for element, create a corresponding `qa-groups-filter-placeholder` instead.
       end
       RUBY
     end
@@ -35,6 +37,13 @@ describe RuboCop::Cop::QA::ElementWithPattern do
         element :groups_filter_placeholder
       end
       RUBY
+
+      expect_no_offenses(<<-RUBY)
+      view 'app/views/shared/groups/_search_form.html.haml' do
+        element :groups_filter, required: true
+        element :groups_filter_placeholder, required: false
+      end
+      RUBY
     end
   end