Commit 4146fcd6 authored by Mark Lapierre's avatar Mark Lapierre

Raise error if test leaves browser logged in

All QA tests expect to be able to log in at the start of the test.

That's not possible if a test leaves the browser logged in when it
finishes. Normally this isn't a problem because Capybara resets the
session after each test. But it does that in an `after` block, so when
a test, like this one, logs in in an `after(:all) block, the browser
returns to a logged in state *after* Capybara has logged it out. And
then the next test will fail.

E.g.: https://gitlab.com/gitlab-org/gitlab/issues/34736

This adds a `config.after(:context)` block to fail if a test
leaves the browser logged in.
parent 71b371b9
...@@ -65,3 +65,23 @@ This library [saves the screenshots in the RSpec's `after` hook](https://github. ...@@ -65,3 +65,23 @@ This library [saves the screenshots in the RSpec's `after` hook](https://github.
Given this fact, we should limit the use of `before(:all)` to only those operations where a screenshot is not Given this fact, we should limit the use of `before(:all)` to only those operations where a screenshot is not
necessary in case of failure and QA logs would be enough for debugging. necessary in case of failure and QA logs would be enough for debugging.
## Ensure tests do not leave the browser logged in
All QA tests expect to be able to log in at the start of the test.
That's not possible if a test leaves the browser logged in when it finishes. Normally this isn't a problem because [Capybara resets the session after each test](https://github.com/teamcapybara/capybara/blob/9ebc5033282d40c73b0286e60217515fd1bb0b5d/lib/capybara/rspec.rb#L18). But Capybara does that in an `after` block, so when a test logs in in an `after(:context)` block, the browser returns to a logged in state *after* Capybara had logged it out. And so the next test will fail.
For an example see: <https://gitlab.com/gitlab-org/gitlab/issues/34736>
Ideally, any actions peformed in an `after(:context)` (or [`before(:context)`](#limit-the-use-of-beforeall-hook)) block would be performed via the API. But if it's necessary to do so via the UI (e.g., if API functionality doesn't exist), make sure to log out at the end of the block.
```ruby
after(:all) do
login unless Page::Main::Menu.perform(&:signed_in?)
# Do something while logged in
Page::Main::Menu.perform(&:sign_out)
end
```
...@@ -19,6 +19,12 @@ module QA ...@@ -19,6 +19,12 @@ module QA
self.class.configure! self.class.configure!
end end
def self.blank_page?
['', 'about:blank', 'data:,'].include?(Capybara.current_session.driver.browser.current_url)
rescue
true
end
## ##
# Visit a page that belongs to a GitLab instance under given address. # Visit a page that belongs to a GitLab instance under given address.
# #
......
...@@ -16,17 +16,14 @@ module QA ...@@ -16,17 +16,14 @@ module QA
} }
] ]
Runtime::Browser.visit(:gitlab, Page::Main::Login)
Page::Main::Login.perform(&:sign_in_using_credentials)
@template_container_group_name = "instance-template-container-group-#{SecureRandom.hex(8)}" @template_container_group_name = "instance-template-container-group-#{SecureRandom.hex(8)}"
template_container_group = QA::Resource::Group.fabricate! do |group| template_container_group = QA::Resource::Group.fabricate_via_api! do |group|
group.path = @template_container_group_name group.path = @template_container_group_name
group.description = 'Instance template container group' group.description = 'Instance template container group'
end end
@template_project = Resource::Project.fabricate! do |project| @template_project = Resource::Project.fabricate_via_api! do |project|
project.name = 'template-project-1' project.name = 'template-project-1'
project.group = template_container_group project.group = template_container_group
end end
...@@ -36,14 +33,13 @@ module QA ...@@ -36,14 +33,13 @@ module QA
push.files = @files push.files = @files
push.commit_message = 'Add test files' push.commit_message = 'Add test files'
end end
Page::Main::Menu.perform(&:sign_out_if_signed_in)
end end
context 'built-in' do context 'built-in' do
before do before do
# Log out if already logged in Page::Main::Menu.perform(&:sign_out_if_signed_in)
Page::Main::Menu.perform do |menu|
menu.sign_out if menu.has_personal_area?(wait: 0)
end
Runtime::Browser.visit(:gitlab, Page::Main::Login) Runtime::Browser.visit(:gitlab, Page::Main::Login)
Page::Main::Login.perform(&:sign_in_using_admin_credentials) Page::Main::Login.perform(&:sign_in_using_admin_credentials)
...@@ -76,10 +72,7 @@ module QA ...@@ -76,10 +72,7 @@ module QA
# Failure issue: https://gitlab.com/gitlab-org/quality/staging/issues/61 # Failure issue: https://gitlab.com/gitlab-org/quality/staging/issues/61
context 'instance level', :quarantine do context 'instance level', :quarantine do
before do before do
# Log out if already logged in Page::Main::Menu.perform(&:sign_out_if_signed_in)
Page::Main::Menu.perform do |menu|
menu.sign_out if menu.has_personal_area?(wait: 0)
end
Runtime::Browser.visit(:gitlab, Page::Main::Login) Runtime::Browser.visit(:gitlab, Page::Main::Login)
Page::Main::Login.perform(&:sign_in_using_admin_credentials) Page::Main::Login.perform(&:sign_in_using_admin_credentials)
...@@ -126,9 +119,7 @@ module QA ...@@ -126,9 +119,7 @@ module QA
before do before do
# Log out if already logged in. This is necessary because # Log out if already logged in. This is necessary because
# a previous test might have logged in as admin # a previous test might have logged in as admin
Page::Main::Menu.perform do |menu| Page::Main::Menu.perform(&:sign_out_if_signed_in)
menu.sign_out if menu.has_personal_area?(wait: 0)
end
Runtime::Browser.visit(:gitlab, Page::Main::Login) Runtime::Browser.visit(:gitlab, Page::Main::Login)
Page::Main::Login.perform(&:sign_in_using_credentials) Page::Main::Login.perform(&:sign_in_using_credentials)
......
...@@ -23,6 +23,24 @@ RSpec.configure do |config| ...@@ -23,6 +23,24 @@ RSpec.configure do |config|
QA::Runtime::Logger.debug("Starting test: #{example.full_description}") if QA::Runtime::Env.debug? QA::Runtime::Logger.debug("Starting test: #{example.full_description}") if QA::Runtime::Env.debug?
end end
config.after(:context) do
if !QA::Runtime::Browser.blank_page? && QA::Page::Main::Menu.perform(&:signed_in?)
QA::Page::Main::Menu.perform(&:sign_out)
raise(
<<~ERROR
The test left the browser signed in.
Usually, Capybara prevents this from happening but some things can
interfere. For example, if it has an `after(:context)` block that logs
in, the browser will stay logged in and this will cause the next test
to fail.
Please make sure the test does not leave the browser signed in.
ERROR
)
end
end
config.expect_with :rspec do |expectations| config.expect_with :rspec do |expectations|
expectations.include_chain_clauses_in_custom_matcher_descriptions = true expectations.include_chain_clauses_in_custom_matcher_descriptions = true
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment