Add latest changes from gitlab-org/gitlab@13-6-stable-eev13.6.0-rc42

1 files changed, 57 insertions, 19 deletions

diff --git a/doc/development/testing_guide/end_to_end/best_practices.md b/doc/development/testing_guide/end_to_end/best_practices.md
index 58bae749dc5..4d12a0f79cb 100644
--- a/doc/development/testing_guide/end_to_end/best_practices.md
+++ b/doc/development/testing_guide/end_to_end/best_practices.md
@@ -1,6 +1,11 @@
+---
+stage: none
+group: Development
+info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#designated-technical-writers
+---
+
 # End-to-end testing Best Practices
 
-NOTE: **Note:**
 This is a tailored extension of the Best Practices [found in the testing guide](../best_practices.md).
 
 ## Link a test to its test-case issue
@@ -263,7 +268,7 @@ We don't run tests that require Administrator access against our Production envi
 
 When you add a new test that requires Administrator access, apply the RSpec metadata `:requires_admin` so that the test will not be included in the test suites executed against Production and other environments on which we don't want to run those tests.
 
-Note: When running tests locally or configuring a pipeline, the environment variable `QA_CAN_TEST_ADMIN_FEATURES` can be set to `false` to skip tests that have the `:requires_admin` tag.
+When running tests locally or configuring a pipeline, the environment variable `QA_CAN_TEST_ADMIN_FEATURES` can be set to `false` to skip tests that have the `:requires_admin` tag.
 
 ## Prefer `Commit` resource over `ProjectPush`
 
@@ -288,7 +293,6 @@ Resource::Repository::ProjectPush.fabricate! do |push|
 end
 ```
 
-NOTE: **Note:**
 A few exceptions for using a `ProjectPush` would be when your test calls for testing SSH integration or
 using the Git CLI.
 
@@ -321,39 +325,69 @@ In general, we use an `expect` statement to check that something _is_ as we expe
 
 ```ruby
 Page::Project::Pipeline::Show.perform do |pipeline|
-  expect(pipeline).to have_job("a_job")
+  expect(pipeline).to have_job('a_job')
 end
 ```
 
-### Ensure `expect` checks for negation efficiently
+### Create negatable matchers to speed `expect` checks
 
 However, sometimes we want to check that something is _not_ as we _don't_ want it to be. In other
-words, we want to make sure something is absent. In such a case we should use an appropriate
-predicate method that returns quickly, rather than waiting for a state that won't appear.
+words, we want to make sure something is absent. For unit tests and feature specs,
+we commonly use `not_to`
+because RSpec's built-in matchers are negatable, as are Capybara's, which means the following two statements are
+equivalent.
+
+```ruby
+except(page).not_to have_text('hidden')
+except(page).to have_no_text('hidden')
+```
+
+Unfortunately, that's not automatically the case for the predicate methods that we add to our
+[page objects](page_objects.md). We need to [create our own negatable matchers](https://relishapp.com/rspec/rspec-expectations/v/3-9/docs/custom-matchers/define-a-custom-matcher#matcher-with-separate-logic-for-expect().to-and-expect().not-to).
+
+The initial example uses the `have_job` matcher which is derived from the [`has_job?` predicate
+method of the `Page::Project::Pipeline::Show` page object](https://gitlab.com/gitlab-org/gitlab/-/blob/87864b3047c23b4308f59c27a3757045944af447/qa/qa/page/project/pipeline/show.rb#L53).
+To create a negatable matcher, we use `has_no_job?` for the negative case:
+
+```ruby
+RSpec::Matchers.define :have_job do |job_name|
+  match do |page_object|
+    page_object.has_job?(job_name)
+  end
+
+  match_when_negated do |page_object|
+    page_object.has_no_job?(job_name)
+  end
+end
+```
 
-It's most efficient to use a predicate method that returns immediately when there is no job, or waits
-until it disappears:
+And then the two `expect` statements in the following example are equivalent:
 
 ```ruby
-# Good
 Page::Project::Pipeline::Show.perform do |pipeline|
-  expect(pipeline).to have_no_job("a_job")
+  expect(pipeline).not_to have_job('a_job')
+  expect(pipeline).to have_no_job('a_job')
 end
 ```
 
-### Problematic alternatives
+[See this merge request for a real example of adding a custom matcher](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46302).
 
-Alternatively, if we want to check that a job doesn't exist it might be tempting to use `not_to`:
+NOTE: **Note:**
+We need to create custom negatable matchers only for the predicate methods we've added to the test framework, and only if we're using `not_to`. If we use `to have_no_*` a negatable matcher is not necessary.
+
+### Why we need negatable matchers
+
+Consider the following code, but assume that we _don't_ have a custom negatable matcher for `have_job`.
 
 ```ruby
 # Bad
 Page::Project::Pipeline::Show.perform do |pipeline|
-  expect(pipeline).not_to have_job("a_job")
+  expect(pipeline).not_to have_job('a_job')
 end
 ```
 
-For this statement to pass, `have_job("a_job")` has to return `false` so that `not_to` can negate it.
-The problem is that `have_job("a_job")` waits up to ten seconds for `"a job"` to appear before
+For this statement to pass, `have_job('a_job')` has to return `false` so that `not_to` can negate it.
+The problem is that `have_job('a_job')` waits up to ten seconds for `'a job'` to appear before
 returning `false`. Under the expected condition this test will take ten seconds longer than it needs to.
 
 Instead, we could force no wait:
@@ -361,9 +395,13 @@ Instead, we could force no wait:
 ```ruby
 # Not as bad but potentially flaky
 Page::Project::Pipeline::Show.perform do |pipeline|
-  expect(pipeline).not_to have_job("a_job", wait: 0)
+  expect(pipeline).not_to have_job('a_job', wait: 0)
 end
 ```
 
-The problem is that if `"a_job"` is present and we're waiting for it to disappear, this statement
-will fail.
+The problem is that if `'a_job'` is present and we're waiting for it to disappear, this statement will fail.
+
+Neither problem is present if we create a custom negatable matcher because the `has_no_job?` predicate method
+would be used, which would wait only as long as necessary for the job to disappear.
+
+Lastly, negatable matchers are preferred over using matchers of the form `have_no_*` because it's a common and familiar practice to negate matchers using `not_to`. If we facilitate that practice by adding negatable matchers, we make it easier for subsequent test authors to write efficient tests.