From 7d5b68d837545909204e6caf2352f34ac2f1ba7a Mon Sep 17 00:00:00 2001 From: ddavison Date: Fri, 1 Mar 2019 18:03:43 -0800 Subject: [PATCH 1/3] Implement dynamic validation on QA Pages Elements now have the ability to be required on pages or not Currently using the default wait mechanism Altered the ElementWithPattern Cop to fit new splat for init --- qa/qa.rb | 1 + qa/qa/ce/strategy.rb | 1 - qa/qa/page/base.rb | 5 +- qa/qa/page/element.rb | 25 +++++++-- qa/qa/page/main/login.rb | 16 +----- qa/qa/page/main/menu.rb | 8 +-- qa/qa/page/validatable.rb | 22 ++++++++ qa/qa/page/view.rb | 4 +- qa/qa/runtime/browser.rb | 1 + qa/qa/support/page/logging.rb | 7 ++- qa/spec/page/element_spec.rb | 56 +++++++++++++++++++ rubocop/cop/qa/element_with_pattern.rb | 20 ++++--- .../cop/qa/element_with_pattern_spec.rb | 11 +++- 13 files changed, 139 insertions(+), 38 deletions(-) create mode 100644 qa/qa/page/validatable.rb 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..7cc7f1a128e --- /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: 10) + 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 a5218fc9ab1..8023bee0e6d 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/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 - add_offense(node, location: pattern.source_range, message: MESSAGE % "qa-#{element_name.value.to_s.tr('_', '-')}") + return if args.first.nil? + + 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 From 3f4a1082ae5788d4135bf01a748036ff03656d32 Mon Sep 17 00:00:00 2001 From: ddavison Date: Tue, 19 Mar 2019 09:24:45 -0700 Subject: [PATCH 2/3] Up wait time to 60 seconds as before for LoginPage --- qa/qa/page/validatable.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qa/qa/page/validatable.rb b/qa/qa/page/validatable.rb index 7cc7f1a128e..8467d261285 100644 --- a/qa/qa/page/validatable.rb +++ b/qa/qa/page/validatable.rb @@ -12,7 +12,7 @@ module QA next unless element.required? # TODO: this wait needs to be replaced by the wait class - unless base_page.has_element?(element.name, wait: 10) + unless base_page.has_element?(element.name, wait: 60) raise Validatable::PageValidationError, "#{element.name} did not appear on #{self.name} as expected" end end From 83cfd20ca86d5f0992438c44707b074a711b7752 Mon Sep 17 00:00:00 2001 From: ddavison Date: Tue, 30 Apr 2019 17:02:20 -0700 Subject: [PATCH 3/3] Remove blocks from Runtime::Browser.visit calls within Mattermost tests Use .perform instead of .act in Mattermost test Signed-off-by: ddavison --- .../login/log_into_mattermost_via_gitlab_spec.rb | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) 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