Simplifies flaky MR review QA test

The test duplicated coverage provided by unit/feature tests, so this
change makes the test much simpler, covering only the happy path of
the MR review feature, plus additional coverage for an S1 regression.

Adds waits after a few actions so that the test doesn't continue to
another action while the previous is still not complete.

Sets `minimum` when finding elements by index because we know at least
how many elements to expect. And then we don't need an extra wait
before using `all` because `minimum` takes care of that - it will no
longer return an empty array if no elements are found, it will raise
an exception instead.

qa/qa/ee/page/merge_request/show.rb

@@ -108,16 +108,28 @@ module QA
 
           def start_review
             click_element :start_review
+
+            # After clicking the button, wait for it to disappear
+            # before moving on to the next part of the test
+            has_no_element? :start_review
           end
 
           def comment_now
             click_element :comment_now
+
+            # After clicking the button, wait for it to disappear
+            # before moving on to the next part of the test
+            has_no_element? :comment_now
           end
 
           def submit_pending_reviews
             within_element :review_bar do
               click_element :review_preview_toggle
               click_element :submit_review
+
+              # After clicking the button, wait for it to disappear
+              # before moving on to the next part of the test
+              has_no_element? :submit_review
             end
           end
 

qa/qa/page/base.rb

@@ -81,8 +81,8 @@ module QA
         find_element(name, class: 'active')
       end
 
-      def all_elements(name)
-        all(element_selector_css(name))
+      def all_elements(name, **kwargs)
+        all(element_selector_css(name), **kwargs)
       end
 
       def check_element(name)
@@ -206,12 +206,7 @@ module QA
       end
 
       def within_element_by_index(name, index)
-        # Finding all elements can be flaky if the elements don't all load
-        # immediately. So we wait for any to appear before trying to find a
-        # specific one.
-        has_element?(name)
-
-        page.within all_elements(name)[index] do
+        page.within all_elements(name, minimum: index + 1)[index] do
           yield
         end
       end

qa/qa/page/merge_request/show.rb

@@ -62,10 +62,14 @@ module QA
 
         def click_discussions_tab
           click_element :notes_tab
+
+          finished_loading?
         end
 
         def click_diffs_tab
           click_element :diffs_tab
+
+          finished_loading?
         end
 
         def click_pipeline_link

qa/qa/specs/features/ee/browser_ui/3_create/merge_request/add_batch_comments_in_merge_request_spec.rb

@@ -16,7 +16,7 @@ module QA
         end
       end
 
-      it 'user submits, discards batch comments' do
+      it 'user submits a non-diff review' do
         Flow::Login.sign_in
 
         merge_request.visit!
@@ -24,52 +24,29 @@ module QA
         Page::MergeRequest::Show.perform do |show|
           show.click_discussions_tab
 
+          # You can't start a review immediately, so we have to add a
+          # comment (or start a thread) first
           show.start_discussion("I'm starting a new discussion")
-          expect(show).to have_content("I'm starting a new discussion")
-
           show.type_reply_to_discussion("Could you please check this?")
-          show.comment_now
+          show.start_review
+          show.submit_pending_reviews
+
+          expect(show).to have_content("I'm starting a new discussion")
           expect(show).to have_content("Could you please check this?")
           expect(show).to have_content("0/1 thread resolved")
+        end
+      end
 
-          show.type_reply_to_discussion("Could you also check that?")
-          show.resolve_review_discussion
-          show.start_review
-          expect(show).to have_content("Could you also check that?")
-          expect(show).to have_content("Finish review 1")
+      it 'user submits a diff review' do
+        Flow::Login.sign_in
 
-          show.click_diffs_tab
+        merge_request.visit!
 
+        Page::MergeRequest::Show.perform do |show|
+          show.click_diffs_tab
           show.add_comment_to_diff("Can you check this line of code?")
-          show.comment_now
-          expect(show).to have_content("Can you check this line of code?")
-
-          show.type_reply_to_discussion("And this syntax as well?")
-          show.resolve_review_discussion
           show.start_review
-          expect(show).to have_content("And this syntax as well?")
-          expect(show).to have_content("Finish review 2")
-
           show.submit_pending_reviews
-          expect(show).to have_content("2/2 threads resolved")
-
-          show.toggle_comments
-          show.type_reply_to_discussion("Unresolving this discussion")
-          show.unresolve_review_discussion
-          show.comment_now
-          expect(show).to have_content("1/2 threads resolved")
-        end
-
-        Page::MergeRequest::Show.perform do |show|
-          show.click_discussions_tab
-
-          show.type_reply_to_discussion("Planning to discard this comment")
-          show.start_review
-
-          expect(show).to have_content("Finish review 1")
-          show.discard_pending_reviews
-
-          expect(show).not_to have_content("Planning to discard this comment")
         end
 
         # Overwrite the added file to create a system note as required to
@@ -90,11 +67,13 @@ module QA
         end
         project.wait_for_push(commit_message)
 
-        merge_request.visit!
         Page::MergeRequest::Show.perform do |show|
-          show.resolve_discussion_at_index(1)
+          show.click_discussions_tab
+          show.refresh
+          show.resolve_discussion_at_index(0)
 
-          expect(show).to have_content("2/2 threads resolved")
+          expect(show).to have_content("Can you check this line of code?")
+          expect(show).to have_content("1/1 thread resolved")
         end
       end
     end

qa/qa/support/page/logging.rb

@@ -49,8 +49,8 @@ module QA
           element
         end
 
-        def all_elements(name)
-          log("finding all :#{name}")
+        def all_elements(name, **kwargs)
+          log("finding all :#{name} with args #{kwargs}")
 
           elements = super