Check for element before evaluate_scriptfeature/reliable-rspec-with-eval-script

Tip from
https://robots.thoughtbot.com/write-reliable-asynchronous-integration-tests-with-capybara#directly-interacting-with-javascript

15 files changed, 39 insertions, 20 deletions

diff --git a/changelogs/unreleased/feature-reliable-rspec-with-eval-script.yml b/changelogs/unreleased/feature-reliable-rspec-with-eval-script.yml
new file mode 100644
index 00000000000..1f36d84092a
--- /dev/null
+++ b/changelogs/unreleased/feature-reliable-rspec-with-eval-script.yml
@@ -0,0 +1,5 @@
+---
+title: Get true failure from evalulate_script by checking for element beforehand
+merge_request: 14898
+author:
+type: fixed
diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md
index 613423dbd9a..624fad4c97c 100644
--- a/doc/development/testing_guide/best_practices.md
+++ b/doc/development/testing_guide/best_practices.md
@@ -35,6 +35,8 @@ Here are some things to keep in mind regarding test performance:
   [Gotchas](../gotchas.md#dont-assert-against-the-absolute-value-of-a-sequence-generated-attribute)).
 - Don't supply the `:each` argument to hooks since it's the default.
 - On `before` and `after` hooks, prefer it scoped to `:context` over `:all`
+- When using `evaluate_script("$('.js-foo').testSomething()")`(or `execute_script`) which acts on a given element,
+  use a Capyabara matcher, `find('.js-foo')`, beforehand to ensure the element actually exists.
 
 [four-phase-test]: https://robots.thoughtbot.com/four-phase-test
 
diff --git a/features/steps/project/issues/filter_labels.rb b/features/steps/project/issues/filter_labels.rb
index d34fa694789..b467af53c98 100644
--- a/features/steps/project/issues/filter_labels.rb
+++ b/features/steps/project/issues/filter_labels.rb
@@ -28,12 +28,6 @@ class Spinach::Features::ProjectIssuesFilterLabels < Spinach::FeatureSteps
     end
   end
 
-  step 'I click link "bug"' do
-    page.find('.js-label-select', visible: true).click
-    sleep 0.5
-    execute_script("$('.dropdown-menu-labels li:contains(\"bug\") a').click()")
-  end
-
   step 'I click "dropdown close button"' do
     page.first('.labels-filter .dropdown-title .dropdown-menu-close-icon').click
     sleep 2
diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb
index 621cae5d80d..6e04f09f322 100644
--- a/features/steps/project/source/browse_files.rb
+++ b/features/steps/project/source/browse_files.rb
@@ -46,10 +46,6 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
     expect(page).to have_content new_gitignore_content
   end
 
-  step 'I should see its content with new lines preserved at end of file' do
-    expect(evaluate_script('ace.edit("editor").getValue()')).to eq "Sample\n\n\n"
-  end
-
   step 'I click link "Raw"' do
     click_link 'Open raw'
   end
@@ -70,20 +66,11 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
     click_link 'Fork'
   end
 
-  step 'I can edit code' do
-    set_new_content
-    expect(evaluate_script('ace.edit("editor").getValue()')).to eq new_gitignore_content
-  end
-
   step 'I edit code' do
     expect(page).to have_selector('.file-editor')
     set_new_content
   end
 
-  step 'I edit code with new lines at end of file' do
-    execute_script('ace.edit("editor").setValue("Sample\n\n\n")')
-  end
-
   step 'I fill the new file name' do
     fill_in :file_name, with: new_file_name
   end
@@ -395,6 +382,7 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
   private
 
   def set_new_content
+    find('#editor')
     execute_script("ace.edit('editor').setValue('#{new_gitignore_content}')")
   end
 
diff --git a/spec/features/boards/boards_spec.rb b/spec/features/boards/boards_spec.rb
index 91c4e5037de..60ed17c0c81 100644
--- a/spec/features/boards/boards_spec.rb
+++ b/spec/features/boards/boards_spec.rb
@@ -171,12 +171,14 @@ describe 'Issue Boards', :js do
         expect(page).to have_selector('.card', count: 20)
         expect(page).to have_content('Showing 20 of 58 issues')
 
+        find('.board .board-list')
         evaluate_script("document.querySelectorAll('.board .board-list')[1].scrollTop = document.querySelectorAll('.board .board-list')[1].scrollHeight")
         wait_for_requests
 
         expect(page).to have_selector('.card', count: 40)
         expect(page).to have_content('Showing 40 of 58 issues')
 
+        find('.board .board-list')
         evaluate_script("document.querySelectorAll('.board .board-list')[1].scrollTop = document.querySelectorAll('.board .board-list')[1].scrollHeight")
         wait_for_requests
 
@@ -449,11 +451,13 @@ describe 'Issue Boards', :js do
           expect(page).to have_selector('.card', count: 20)
           expect(page).to have_content('Showing 20 of 51 issues')
 
+          find('.board .board-list')
           evaluate_script("document.querySelectorAll('.board .board-list')[1].scrollTop = document.querySelectorAll('.board .board-list')[1].scrollHeight")
 
           expect(page).to have_selector('.card', count: 40)
           expect(page).to have_content('Showing 40 of 51 issues')
 
+          find('.board .board-list')
           evaluate_script("document.querySelectorAll('.board .board-list')[1].scrollTop = document.querySelectorAll('.board .board-list')[1].scrollHeight")
 
           expect(page).to have_selector('.card', count: 51)
diff --git a/spec/features/ci_lint_spec.rb b/spec/features/ci_lint_spec.rb
index 9accd7bb07c..9bc23baf6cf 100644
--- a/spec/features/ci_lint_spec.rb
+++ b/spec/features/ci_lint_spec.rb
@@ -10,6 +10,7 @@ describe 'CI Lint', :js do
       visit ci_lint_path
       # Ace editor updates a hidden textarea and it happens asynchronously
       # `sleep 0.1` is actually needed here because of this
+      find('#ci-editor')
       execute_script("ace.edit('ci-editor').setValue(" + yaml_content.to_json + ");")
       sleep 0.1
       click_on 'Validate'
diff --git a/spec/features/dashboard/issues_spec.rb b/spec/features/dashboard/issues_spec.rb
index 5610894fd9a..a8919976c31 100644
--- a/spec/features/dashboard/issues_spec.rb
+++ b/spec/features/dashboard/issues_spec.rb
@@ -87,8 +87,10 @@ RSpec.describe 'Dashboard Issues' do
       project_path = "/#{project.path_with_namespace}"
       project_json = { name: project.name_with_namespace, url: project_path }.to_json
 
-      # similate selection, and prevent overlap by dropdown menu
+      # simulate selection, and prevent overlap by dropdown menu
+      first('.project-item-select', visible: false)
       execute_script("$('.project-item-select').val('#{project_json}').trigger('change');")
+      find('#select2-drop-mask', visible: false)
       execute_script("$('#select2-drop-mask').remove();")
 
       find('.new-project-item-link').trigger('click')
diff --git a/spec/features/issues/markdown_toolbar_spec.rb b/spec/features/issues/markdown_toolbar_spec.rb
index 6869c2c869d..fee8fd9b365 100644
--- a/spec/features/issues/markdown_toolbar_spec.rb
+++ b/spec/features/issues/markdown_toolbar_spec.rb
@@ -16,6 +16,7 @@ feature 'Issue markdown toolbar', :js do
     find('#note-body').native.send_key(:enter)
     find('#note-body').native.send_keys('bold')
 
+    find('.js-main-target-form #note-body')
     page.evaluate_script('document.querySelectorAll(".js-main-target-form #note-body")[0].setSelectionRange(4, 9)')
 
     first('.toolbar-btn').click
@@ -28,6 +29,7 @@ feature 'Issue markdown toolbar', :js do
     find('#note-body').native.send_key(:enter)
     find('#note-body').native.send_keys('underline')
 
+    find('.js-main-target-form #note-body')
     page.evaluate_script('document.querySelectorAll(".js-main-target-form #note-body")[0].setSelectionRange(4, 50)')
 
     find('.toolbar-btn:nth-child(2)').click
diff --git a/spec/features/merge_requests/conflicts_spec.rb b/spec/features/merge_requests/conflicts_spec.rb
index b0432ed8fc6..ba976bc7216 100644
--- a/spec/features/merge_requests/conflicts_spec.rb
+++ b/spec/features/merge_requests/conflicts_spec.rb
@@ -60,12 +60,14 @@ feature 'Merge request conflict resolution', :js do
       within find('.files-wrapper .diff-file', text: 'files/ruby/popen.rb') do
         click_button 'Edit inline'
         wait_for_requests
+        find('.files-wrapper .diff-file pre')
         execute_script('ace.edit($(".files-wrapper .diff-file pre")[0]).setValue("One morning");')
       end
 
       within find('.files-wrapper .diff-file', text: 'files/ruby/regex.rb') do
         click_button 'Edit inline'
         wait_for_requests
+        find('.files-wrapper .diff-file pre')
         execute_script('ace.edit($(".files-wrapper .diff-file pre")[1]).setValue("Gregor Samsa woke from troubled dreams");')
       end
 
@@ -139,6 +141,7 @@ feature 'Merge request conflict resolution', :js do
       it 'conflicts are resolved in Edit inline mode' do
         within find('.files-wrapper .diff-file', text: 'files/markdown/ruby-style-guide.md') do
           wait_for_requests
+          find('.files-wrapper .diff-file pre')
           execute_script('ace.edit($(".files-wrapper .diff-file pre")[0]).setValue("Gregor Samsa woke from troubled dreams");')
         end
 
diff --git a/spec/features/merge_requests/edit_mr_spec.rb b/spec/features/merge_requests/edit_mr_spec.rb
index 4538555c168..4362f8b3fcc 100644
--- a/spec/features/merge_requests/edit_mr_spec.rb
+++ b/spec/features/merge_requests/edit_mr_spec.rb
@@ -66,6 +66,7 @@ feature 'Edit Merge Request' do
     end
 
     def get_textarea_height
+      find('#merge_request_description')
       page.evaluate_script('document.getElementById("merge_request_description").offsetHeight')
     end
   end
diff --git a/spec/features/merge_requests/mini_pipeline_graph_spec.rb b/spec/features/merge_requests/mini_pipeline_graph_spec.rb
index dcc70338d7f..bf21a719901 100644
--- a/spec/features/merge_requests/mini_pipeline_graph_spec.rb
+++ b/spec/features/merge_requests/mini_pipeline_graph_spec.rb
@@ -52,10 +52,12 @@ feature 'Mini Pipeline Graph', :js do
     end
 
     it 'should expand when hovered' do
+      find('.mini-pipeline-graph-dropdown-toggle')
       before_width = evaluate_script("$('.mini-pipeline-graph-dropdown-toggle:visible').outerWidth();")
 
       toggle.hover
 
+      find('.mini-pipeline-graph-dropdown-toggle')
       after_width = evaluate_script("$('.mini-pipeline-graph-dropdown-toggle:visible').outerWidth();")
 
       expect(before_width).to be < after_width
diff --git a/spec/features/projects/blobs/edit_spec.rb b/spec/features/projects/blobs/edit_spec.rb
index 6c625ed17aa..965028a6f90 100644
--- a/spec/features/projects/blobs/edit_spec.rb
+++ b/spec/features/projects/blobs/edit_spec.rb
@@ -20,6 +20,7 @@ feature 'Editing file blob', :js do
     def edit_and_commit
       wait_for_requests
       find('.js-edit-blob').click
+      find('#editor')
       execute_script('ace.edit("editor").setValue("class NextFeature\nend\n")')
       click_button 'Commit changes'
     end
diff --git a/spec/features/projects/user_creates_files_spec.rb b/spec/features/projects/user_creates_files_spec.rb
index cbe70a93942..d84b91ddc32 100644
--- a/spec/features/projects/user_creates_files_spec.rb
+++ b/spec/features/projects/user_creates_files_spec.rb
@@ -60,6 +60,7 @@ describe 'User creates files' do
       end
 
       it 'creates and commit a new file', :js do
+        find('#editor')
         execute_script("ace.edit('editor').setValue('*.rbca')")
         fill_in(:file_name, with: 'not_a_file.md')
         fill_in(:commit_message, with: 'New commit message', visible: true)
@@ -75,6 +76,7 @@ describe 'User creates files' do
       end
 
       it 'creates and commit a new file with new lines at the end of file', :js do
+        find('#editor')
         execute_script('ace.edit("editor").setValue("Sample\n\n\n")')
         fill_in(:file_name, with: 'not_a_file.md')
         fill_in(:commit_message, with: 'New commit message', visible: true)
@@ -86,6 +88,7 @@ describe 'User creates files' do
 
         find('.js-edit-blob').click
 
+        find('#editor')
         expect(evaluate_script('ace.edit("editor").getValue()')).to eq("Sample\n\n\n")
       end
 
@@ -94,6 +97,7 @@ describe 'User creates files' do
 
         expect(page).to have_selector('.file-editor')
 
+        find('#editor')
         execute_script("ace.edit('editor').setValue('*.rbca')")
         fill_in(:commit_message, with: 'New commit message', visible: true)
         click_button('Commit changes')
@@ -108,6 +112,7 @@ describe 'User creates files' do
       it 'creates and commit a new file specifying a new branch', :js do
         expect(page).to have_selector('.file-editor')
 
+        find('#editor')
         execute_script("ace.edit('editor').setValue('*.rbca')")
         fill_in(:file_name, with: 'not_a_file.md')
         fill_in(:commit_message, with: 'New commit message', visible: true)
@@ -136,6 +141,7 @@ describe 'User creates files' do
 
         expect(page).to have_selector('.file-editor')
 
+        find('#editor')
         execute_script("ace.edit('editor').setValue('*.rbca')")
 
         fill_in(:file_name, with: 'not_a_file.md')
diff --git a/spec/features/projects/user_edits_files_spec.rb b/spec/features/projects/user_edits_files_spec.rb
index e8d83a661d4..d26ee653415 100644
--- a/spec/features/projects/user_edits_files_spec.rb
+++ b/spec/features/projects/user_edits_files_spec.rb
@@ -23,6 +23,7 @@ describe 'User edits files' do
       find('.js-edit-blob').click
       find('.file-editor', match: :first)
 
+      find('#editor')
       execute_script("ace.edit('editor').setValue('*.rbca')")
 
       expect(evaluate_script('ace.edit("editor").getValue()')).to eq('*.rbca')
@@ -40,6 +41,7 @@ describe 'User edits files' do
       find('.js-edit-blob').click
       find('.file-editor', match: :first)
 
+      find('#editor')
       execute_script("ace.edit('editor').setValue('*.rbca')")
       fill_in(:commit_message, with: 'New commit message', visible: true)
       click_button('Commit changes')
@@ -57,6 +59,7 @@ describe 'User edits files' do
 
       find('.file-editor', match: :first)
 
+      find('#editor')
       execute_script("ace.edit('editor').setValue('*.rbca')")
       fill_in(:commit_message, with: 'New commit message', visible: true)
       fill_in(:branch_name, with: 'new_branch_name', visible: true)
@@ -74,6 +77,7 @@ describe 'User edits files' do
       find('.js-edit-blob').click
       find('.file-editor', match: :first)
 
+      find('#editor')
       execute_script("ace.edit('editor').setValue('*.rbca')")
       click_link('Preview changes')
 
@@ -103,6 +107,7 @@ describe 'User edits files' do
 
       find('.file-editor', match: :first)
 
+      find('#editor')
       execute_script("ace.edit('editor').setValue('*.rbca')")
 
       expect(evaluate_script('ace.edit("editor").getValue()')).to eq('*.rbca')
@@ -119,6 +124,7 @@ describe 'User edits files' do
 
       find('.file-editor', match: :first)
 
+      find('#editor')
       execute_script("ace.edit('editor').setValue('*.rbca')")
       fill_in(:commit_message, with: 'New commit message', visible: true)
       click_button('Commit changes')
@@ -145,6 +151,7 @@ describe 'User edits files' do
         expect(page).not_to have_link('Fork')
         expect(page).not_to have_button('Cancel')
 
+        find('#editor')
         execute_script("ace.edit('editor').setValue('*.rbca')")
         fill_in(:commit_message, with: 'Another commit', visible: true)
         click_button('Commit changes')
diff --git a/spec/support/select2_helper.rb b/spec/support/select2_helper.rb
index 6b1853c2364..b179ce90ae4 100644
--- a/spec/support/select2_helper.rb
+++ b/spec/support/select2_helper.rb
@@ -16,6 +16,7 @@ module Select2Helper
 
     selector = options.fetch(:from)
 
+    first("#{selector}", visible: false)
     if options[:multiple]
       execute_script("$('#{selector}').select2('val', ['#{value}']).trigger('change');")
     else