Raise on failure by default

Many cases of flaky tests have been caused by allowing the test
to continue even after a precondition isn't satisfied. This
change makes it harder for that to happen by raising an error
if the condition of a wait/retry block isn't met.

qa/qa/ee/scenario/test/geo.rb

@@ -154,9 +154,6 @@ module QA
                 QA::Page::Main::OAuth.perform do |oauth|
                   oauth.authorize! if oauth.needs_authorization?
                 end
-
-                # Log out so that tests are in an initially unauthenticated state
-                QA::Page::Main::Menu.perform(&:sign_out)
               end
             end
 

qa/qa/flow/login.rb

@@ -12,6 +12,12 @@ module QA
 
         yield
 
+        # Workaround for a bug preventing sign out from secondary nodes
+        # See https://gitlab.com/gitlab-org/gitlab/issues/198289
+        if address == :geo_secondary
+          Runtime::Browser.visit(:geo_primary, Page::Dashboard::Projects)
+        end
+
         Page::Main::Menu.perform(&:sign_out)
       end
 

qa/qa/page/base.rb

@@ -26,13 +26,13 @@ module QA
         wait_for_requests
       end
 
-      def wait_until(max_duration: 60, sleep_interval: 0.1, reload: true, raise_on_failure: false)
+      def wait_until(max_duration: 60, sleep_interval: 0.1, reload: true, raise_on_failure: true)
         Support::Waiter.wait_until(max_duration: max_duration, sleep_interval: sleep_interval, raise_on_failure: raise_on_failure) do
           yield || (reload && refresh && false)
         end
       end
 
-      def retry_until(max_attempts: 3, reload: false, sleep_interval: 0, raise_on_failure: false)
+      def retry_until(max_attempts: 3, reload: false, sleep_interval: 0, raise_on_failure: true)
         Support::Retrier.retry_until(max_attempts: max_attempts, reload_page: (reload && self), sleep_interval: sleep_interval, raise_on_failure: raise_on_failure) do
           yield
         end

qa/qa/specs/features/ee/browser_ui/1_manage/group/group_audit_logs_1_spec.rb

@@ -9,7 +9,7 @@ module QA
 
     shared_examples 'group audit event logs' do |expected_events|
       it 'logs audit events' do
-        wait_for_audit_events(expected_events)
+        wait_for_audit_events(expected_events, group)
 
         Page::Group::Menu.perform(&:go_to_audit_events_settings)
         expected_events.each do |expected_event|
@@ -32,7 +32,7 @@ module QA
       end
 
       before do
-        @event_count = get_audit_event_count
+        @event_count = get_audit_event_count(@group)
       end
 
       let(:project) do
@@ -42,15 +42,27 @@ module QA
       end
 
       let(:user) { Resource::User.fabricate_or_use(Runtime::Env.gitlab_qa_username_1, Runtime::Env.gitlab_qa_password_1) }
+      let(:group) { @group }
 
       context 'Add group' do
+        let(:group_name) { 'new group' }
+
         before do
+          @event_count = 0
           sign_in
-          Resource::Group.fabricate_via_browser_ui!.visit!
+          Resource::Group.fabricate_via_browser_ui! do |group|
+            group.name = group_name
+          end.visit!
           Page::Group::Menu.perform(&:click_group_general_settings_item)
         end
 
-        it_behaves_like 'group audit event logs', ["Add group"]
+        it_behaves_like 'group audit event logs', ['Add group'] do
+          let(:group) do
+            Resource::Group.fabricate_via_api! do |group|
+              group.name = group_name
+            end
+          end
+        end
       end
 
       context 'Change repository size limit', :requires_admin do
@@ -63,7 +75,7 @@ module QA
             settings.click_save_name_visibility_settings_button
           end
         end
-        it_behaves_like 'group audit event logs', ["Change repository size limit"]
+        it_behaves_like 'group audit event logs', ['Change repository size limit']
       end
 
       context 'Update group name' do
@@ -78,7 +90,7 @@ module QA
           end
         end
 
-        it_behaves_like 'group audit event logs', ["Change name"]
+        it_behaves_like 'group audit event logs', ['Change name']
       end
 
       context 'Add user, change access level, remove user' do
@@ -93,7 +105,7 @@ module QA
           end
         end
 
-        it_behaves_like 'group audit event logs', ["Add user access as guest", "Change access level", "Remove user access"]
+        it_behaves_like 'group audit event logs', ['Add user access as guest', 'Change access level', 'Remove user access']
       end
 
       context 'Add and remove project access' do
@@ -114,7 +126,7 @@ module QA
           @group.visit!
         end
 
-        it_behaves_like 'group audit event logs', ["Add project access", "Remove project access"]
+        it_behaves_like 'group audit event logs', ['Add project access', 'Remove project access']
       end
     end
 
@@ -127,16 +139,16 @@ module QA
       end
     end
 
-    def get_audit_event_count
-      response = get Runtime::API::Request.new(api_client, "/groups/#{@group.id}/audit_events").url
+    def get_audit_event_count(group)
+      response = get Runtime::API::Request.new(api_client, "/groups/#{group.id}/audit_events").url
       parse_body(response).length
     end
 
-    def wait_for_audit_events(expected_events)
+    def wait_for_audit_events(expected_events, group)
       new_event_count = @event_count + expected_events.length
 
-      Support::Retrier.retry_until(sleep_interval: 1) do
-        get_audit_event_count == new_event_count
+      Support::Retrier.retry_until(max_duration: QA::Support::Repeater::DEFAULT_MAX_WAIT_TIME, sleep_interval: 1) do
+        get_audit_event_count(group) == new_event_count
       end
     end
   end

qa/qa/support/page/logging.rb

@@ -16,13 +16,6 @@ module QA
           super
         end
 
-        def wait_until(max_duration: 60, sleep_interval: 0.1, reload: true, raise_on_failure: false)
-          log("next wait uses reload: #{reload}")
-          # Logging of wait start/end/duration is handled by QA::Support::Waiter
-
-          super
-        end
-
         def scroll_to(selector, text: nil)
           msg = "scrolling to :#{selector}"
           msg += " with text: #{text}" if text

qa/qa/support/retrier.rb

@@ -34,7 +34,7 @@ module QA
         result
       end
 
-      def retry_until(max_attempts: nil, max_duration: nil, reload_page: nil, sleep_interval: 0, raise_on_failure: false, retry_on_exception: false)
+      def retry_until(max_attempts: nil, max_duration: nil, reload_page: nil, sleep_interval: 0, raise_on_failure: true, retry_on_exception: false)
         # For backwards-compatibility
         max_attempts = 3 if max_attempts.nil? && max_duration.nil?
 

qa/qa/support/waiter.rb

@@ -7,7 +7,7 @@ module QA
 
       module_function
 
-      def wait_until(max_duration: singleton_class::DEFAULT_MAX_WAIT_TIME, reload_page: nil, sleep_interval: 0.1, raise_on_failure: false, retry_on_exception: false)
+      def wait_until(max_duration: singleton_class::DEFAULT_MAX_WAIT_TIME, reload_page: nil, sleep_interval: 0.1, raise_on_failure: true, retry_on_exception: false)
         QA::Runtime::Logger.debug(
           <<~MSG.tr("\n", ' ')
             with wait_until: max_duration: #{max_duration};

qa/spec/page/logging_spec.rb

@@ -27,24 +27,6 @@ describe QA::Support::Page::Logging do
       .to output(%r{refreshing http://current-url}).to_stdout_from_any_process
   end
 
-  it 'logs wait' do
-    expect { subject.wait_until(max_duration: 0) {} }
-      .to output(/next wait uses reload: true/).to_stdout_from_any_process
-    expect { subject.wait_until(max_duration: 0) {} }
-      .to output(/with wait_until/).to_stdout_from_any_process
-    expect { subject.wait_until(max_duration: 0) {} }
-      .to output(/ended wait_until$/).to_stdout_from_any_process
-  end
-
-  it 'logs wait with reload false' do
-    expect { subject.wait_until(max_duration: 0, reload: false) {} }
-      .to output(/next wait uses reload: false/).to_stdout_from_any_process
-    expect { subject.wait_until(max_duration: 0, reload: false) {} }
-      .to output(/with wait_until/).to_stdout_from_any_process
-    expect { subject.wait_until(max_duration: 0, reload: false) {} }
-      .to output(/ended wait_until$/).to_stdout_from_any_process
-  end
-
   it 'logs scroll_to' do
     expect { subject.scroll_to(:element) }
       .to output(/scrolling to :element/).to_stdout_from_any_process

qa/spec/support/retrier_spec.rb

@@ -14,12 +14,12 @@ describe QA::Support::Retrier do
     context 'when the condition is true' do
       it 'logs max attempts (3 by default)' do
         expect { subject.retry_until { true } }
-          .to output(/with retry_until: max_attempts: 3; reload_page: ; sleep_interval: 0; raise_on_failure: false; retry_on_exception: false/).to_stdout_from_any_process
+          .to output(/with retry_until: max_attempts: 3; reload_page: ; sleep_interval: 0; raise_on_failure: true; retry_on_exception: false/).to_stdout_from_any_process
       end
 
       it 'logs max duration' do
         expect { subject.retry_until(max_duration: 1) { true } }
-          .to output(/with retry_until: max_duration: 1; reload_page: ; sleep_interval: 0; raise_on_failure: false; retry_on_exception: false/).to_stdout_from_any_process
+          .to output(/with retry_until: max_duration: 1; reload_page: ; sleep_interval: 0; raise_on_failure: true; retry_on_exception: false/).to_stdout_from_any_process
       end
 
       it 'logs the end' do
@@ -30,12 +30,12 @@ describe QA::Support::Retrier do
 
     context 'when the condition is false' do
       it 'logs the start' do
-        expect { subject.retry_until(max_duration: 0) { false } }
+        expect { subject.retry_until(max_duration: 0, raise_on_failure: false) { false } }
           .to output(/with retry_until: max_duration: 0; reload_page: ; sleep_interval: 0; raise_on_failure: false; retry_on_exception: false/).to_stdout_from_any_process
       end
 
       it 'logs the end' do
-        expect { subject.retry_until(max_duration: 0) { false } }
+        expect { subject.retry_until(max_duration: 0, raise_on_failure: false) { false } }
         .to output(/ended retry_until$/).to_stdout_from_any_process
       end
     end
@@ -54,8 +54,8 @@ describe QA::Support::Retrier do
       subject.retry_until
     end
 
-    it 'sets raise_on_failure to false by default' do
-      expect(subject).to receive(:repeat_until).with(hash_including(raise_on_failure: false))
+    it 'sets raise_on_failure to true by default' do
+      expect(subject).to receive(:repeat_until).with(hash_including(raise_on_failure: true))
 
       subject.retry_until
     end

qa/spec/support/waiter_spec.rb

@@ -46,8 +46,8 @@ describe QA::Support::Waiter do
       subject.wait_until
     end
 
-    it 'sets raise_on_failure to false by default' do
-      expect(subject).to receive(:repeat_until).with(hash_including(raise_on_failure: false))
+    it 'sets raise_on_failure to true by default' do
+      expect(subject).to receive(:repeat_until).with(hash_including(raise_on_failure: true))
 
       subject.wait_until
     end