Merge branch 'master' into bootstrap4

11 files changed, 54 insertions, 65 deletions

diff --git a/doc/development/code_review.md b/doc/development/code_review.md
index 7165b8062a7..d03b7fa23ca 100644
--- a/doc/development/code_review.md
+++ b/doc/development/code_review.md
@@ -29,6 +29,10 @@ There are a few rules to get your merge request accepted:
     to ask one of the [Merge request coaches][team].
   1. The reviewer will assign the merge request to a maintainer once the
     reviewer is satisfied with the state of the merge request.
+1. Keep in mind that maintainers are also going to perform a final code review.
+   The ideal scenario is that the reviewer has already addressed any concerns
+   the maintainer would have found, and the maintainer only has to perform the
+   merge, but be prepared for further review comments.
 
 For more guidance, see [CONTRIBUTING.md](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md).
 
@@ -207,3 +211,4 @@ Largely based on the [thoughtbot code review guide].
 [projects]: https://about.gitlab.com/handbook/engineering/projects/
 [team]: https://about.gitlab.com/team/
 [build handbook]: https://about.gitlab.com/handbook/build/handbook/build#how-to-work-with-build
+[^1]: Please note that specs other than JavaScript specs are considered backend code.
diff --git a/doc/development/database_debugging.md b/doc/development/database_debugging.md
index 32f392f1303..9c31265e417 100644
--- a/doc/development/database_debugging.md
+++ b/doc/development/database_debugging.md
@@ -11,7 +11,7 @@ Available `RAILS_ENV`
 
  - `production` (generally not for your main GDK db, but you may need this for e.g. omnibus)
  - `development` (this is your main GDK db)
- - `test` (used for tests like rspec and spinach)
+ - `test` (used for tests like rspec)
 
 
 ## Nuke everything and start over
diff --git a/doc/development/fe_guide/development_process.md b/doc/development/fe_guide/development_process.md
index 5504a6421d5..d240dbe8b02 100644
--- a/doc/development/fe_guide/development_process.md
+++ b/doc/development/fe_guide/development_process.md
@@ -22,9 +22,9 @@ Please use your best judgement when to use it and please contribute new points t
 - [ ] Are all [departments](https://about.gitlab.com/handbook/engineering/#engineering-teams) that are needed from your perspective already involved in the issue? (For example is UX missing?)
 - [ ] Is the specification complete? Are you missing decisions? How about error handling/defaults/edge cases? Take your time to understand the needed implementation and go through its flow.
 - [ ] Are all necessary UX specifications available that you will need in order to implement? Are there new UX components/patterns in the designs? Then contact the UI component team early on. How should error messages or validation be handled?
-- [ ] **Library usage** Use Vuex as soon as you have even a medium state to manage, use Vue router if you need to have different views internally and want to link from the outside. Check what libraries we already have for which occassions.
+- [ ] **Library usage** Use Vuex as soon as you have even a medium state to manage, use Vue router if you need to have different views internally and want to link from the outside. Check what libraries we already have for which occasions.
 - [ ] **Plan your implementation:**
-    - [ ] **Architecture plan:** Create a plan aligned with GitLab's architecture, how you are going to do the implementation, for example Vue application setup and its components (through [onion skinning](https://gitlab.com/gitlab-org/gitlab-ce/issues/35873#note_39994091)), Store structure and data flow, which existing Vue components can you reuse. Its a good idea to go through your plan with another engineer to refine it.
+    - [ ] **Architecture plan:** Create a plan aligned with GitLab's architecture, how you are going to do the implementation, for example Vue application setup and its components (through [onion skinning](https://gitlab.com/gitlab-org/gitlab-ce/issues/35873#note_39994091)), Store structure and data flow, which existing Vue components can you reuse. It's a good idea to go through your plan with another engineer to refine it.
     - [ ] **Backend:** The best way is to kickoff the implementation in a call and discuss with the assigned Backend engineer what you will need from the backend and also when. Can you reuse existing API's? How is the performance with the planned architecture? Maybe create together a JSON mock object to already start with development.
     - [ ] **Communication:** It also makes sense to have for bigger features an own slack channel (normally called #f_{feature_name}) and even weekly demo calls with all people involved.
     - [ ] **Dependency Plan:** Are there big dependencies in the plan between you and others, then maybe create an execution diagram to show what is blocking which part and the order of the different parts.
@@ -56,7 +56,7 @@ Please use your best judgement when to use it and please contribute new points t
 - [ ] If you have multiple MR's then also smoke test against the final merge.
 - [ ] Are there any big changes on how and especially how frequently we use the API then let production know about it
 - [ ] Smoke test of the RC on dev., staging., canary deployments and .com
-- [ ] Follow up on issues that came out of the review. Create isssues for discovered edge cases that should be covered in future iterations.
+- [ ] Follow up on issues that came out of the review. Create issues for discovered edge cases that should be covered in future iterations.
 
 ---
 
diff --git a/doc/development/fe_guide/vuex.md b/doc/development/fe_guide/vuex.md
index 6a89bfc7721..8997a5889dc 100644
--- a/doc/development/fe_guide/vuex.md
+++ b/doc/development/fe_guide/vuex.md
@@ -68,10 +68,10 @@ Often we need to provide data from haml to our Vue application. Let's store it i
 You can use `mapState` to access state properties in the components.
 
 ### `actions.js`
-An action is a playload of information to send data from our application to our store.
+An action is a payload of information to send data from our application to our store.
 
 An action is usually composed by a `type` and a `payload` and they describe what happened.
-Enforcing that every change is described as an action lets us have a clear understanting of what is going on in the app.
+Enforcing that every change is described as an action lets us have a clear understanding of what is going on in the app.
 
 In this file, we will write the actions that will call the respective mutations:
 
@@ -87,7 +87,7 @@ In this file, we will write the actions that will call the respective mutations:
   export const fetchUsers = ({ state, dispatch }) => {
     dispatch('requestUsers');
 
-    axios.get(state.endoint)
+    axios.get(state.endpoint)
       .then(({ data }) => dispatch('receiveUsersSuccess', data))
       .catch((error) => {
         dispatch('receiveUsersError', error)
@@ -102,7 +102,7 @@ In this file, we will write the actions that will call the respective mutations:
   export const addUser = ({ state, dispatch }, user) => {
     dispatch('requestAddUser');
 
-    axios.post(state.endoint, user)
+    axios.post(state.endpoint, user)
       .then(({ data }) => dispatch('receiveAddUserSuccess', data))
       .catch((error) => dispatch('receiveAddUserError', error));
   }
@@ -126,7 +126,7 @@ The component MUST only dispatch the `fetchNamespace` action. Actions namespaced
 The `fetch` action will be responsible to dispatch `requestNamespace`, `receiveNamespaceSuccess` and `receiveNamespaceError`
 
 By following this pattern we guarantee:
-1. All aplications follow the same pattern, making it easier for anyone to maintain the code
+1. All applications follow the same pattern, making it easier for anyone to maintain the code
 1. All data in the application follows the same lifecycle pattern
 1. Actions are contained and human friendly
 1. Unit tests are easier
@@ -149,7 +149,7 @@ import { mapActions } from 'vuex';
 };
 ```
 
-#### `mutations.js`
+### `mutations.js`
 The mutations specify how the application state changes in response to actions sent to the store.
 The only way to change state in a Vuex store should be by committing a mutation.
 
@@ -175,7 +175,7 @@ Remember that actions only describe that something happened, they don't describe
       state.isLoading = false;
     },
     [types.REQUEST_ADD_USER](state, user) {
-     state.isAddingUser = true;
+      state.isAddingUser = true;
     },
     [types.RECEIVE_ADD_USER_SUCCESS](state, user) {
       state.isAddingUser = false;
@@ -183,12 +183,12 @@ Remember that actions only describe that something happened, they don't describe
     },
     [types.REQUEST_ADD_USER_ERROR](state, error) {
       state.isAddingUser = true;
-      state.errorAddingUser = error∂;
+      state.errorAddingUser = error;
     },
   };
 ```
 
-#### `getters.js`
+### `getters.js`
 Sometimes we may need to get derived state based on store state, like filtering for a specific prop.
 Using a getter will also cache the result based on dependencies due to [how computed props work](https://vuejs.org/v2/guide/computed.html#Computed-Caching-vs-Methods)
 This can be done through the `getters`:
@@ -213,7 +213,7 @@ import { mapGetters } from 'vuex';
 };
 ```
 
-#### `mutations_types.js`
+### `mutations_types.js`
 From [vuex mutations docs][vuex-mutations]:
 > It is a commonly seen pattern to use constants for mutation types in various Flux implementations. This allows the code to take advantage of tooling like linters, and putting all constants in a single file allows your collaborators to get an at-a-glance view of what mutations are possible in the entire application.
 
@@ -289,7 +289,7 @@ export default {
 ```
 
 ### Vuex Gotchas
-1. Do not call a mutation directly. Always use an action to commit a mutation. Doing so will keep consistency through out the application. From Vuex docs:
+1. Do not call a mutation directly. Always use an action to commit a mutation. Doing so will keep consistency throughout the application. From Vuex docs:
 
   >  why don't we just call store.commit('action') directly? Well, remember that mutations must be synchronous? Actions aren't. We can perform asynchronous operations inside an action.
 
@@ -342,7 +342,7 @@ describe('component', () => {
     };
 
     // populate the store
-    store.dipatch('addUser', user);
+    store.dispatch('addUser', user);
 
     vm = new Component({
       store,
@@ -352,6 +352,18 @@ describe('component', () => {
 });
 ```
 
+#### Testing Vuex actions and getters
+Because we're currently using [`babel-plugin-rewire`](https://github.com/speedskater/babel-plugin-rewire), you may encounter the following error when testing your Vuex actions and getters:
+`[vuex] actions should be function or object with "handler" function`
+
+To prevent this error from happening, you need to export an empty function as `default`:
+```
+// getters.js or actions.js
+
+// prevent babel-plugin-rewire from generating an invalid default during karma tests
+export default () => {};
+```
+
 [vuex-docs]: https://vuex.vuejs.org
 [vuex-structure]: https://vuex.vuejs.org/en/structure.html
 [vuex-mutations]: https://vuex.vuejs.org/en/mutations.html
diff --git a/doc/development/query_recorder.md b/doc/development/query_recorder.md
index 12e90101139..26d3355e94d 100644
--- a/doc/development/query_recorder.md
+++ b/doc/development/query_recorder.md
@@ -2,7 +2,7 @@
 
 QueryRecorder is a tool for detecting the [N+1 queries problem](http://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations) from tests.
 
-> Implemented in [spec/support/query_recorder.rb](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/support/query_recorder.rb) via [9c623e3e](https://gitlab.com/gitlab-org/gitlab-ce/commit/9c623e3e5d7434f2e30f7c389d13e5af4ede770a)
+> Implemented in [spec/support/query_recorder.rb](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/support/helpers/query_recorder.rb) via [9c623e3e](https://gitlab.com/gitlab-org/gitlab-ce/commit/9c623e3e5d7434f2e30f7c389d13e5af4ede770a)
 
 As a rule, merge requests [should not increase query counts](merge_request_performance_guidelines.md#query-counts). If you find yourself adding something like `.includes(:author, :assignee)` to avoid having `N+1` queries, consider using QueryRecorder to enforce this with a test. Without this, a new feature which causes an additional model to be accessed will silently reintroduce the problem.
 
diff --git a/doc/development/rake_tasks.md b/doc/development/rake_tasks.md
index fdfa1f10402..31addcaf675 100644
--- a/doc/development/rake_tasks.md
+++ b/doc/development/rake_tasks.md
@@ -65,12 +65,11 @@ To make sure that indices still fit. You could find great details in:
 ## Run tests
 
 In order to run the test you can use the following commands:
-- `rake spinach` to run the spinach suite
 - `rake spec` to run the rspec suite
 - `rake karma` to run the karma test suite
 - `rake gitlab:test` to run all the tests
 
-Note: Both `rake spinach` and `rake spec` takes significant time to pass.
+Note: `rake spec` takes significant time to pass.
 Instead of running full test suite locally you can save a lot of time by running
 a single test or directory related to your changes. After you submit merge request
 CI will run full test suite for you. Green CI status in the merge request means
@@ -82,12 +81,10 @@ files it can find, also the ones in `/tmp`
 To run a single test file you can use:
 
 - `bin/rspec spec/controllers/commit_controller_spec.rb` for a rspec test
-- `bin/spinach features/project/issues/milestones.feature` for a spinach test
 
 To run several tests inside one directory:
 
 - `bin/rspec spec/requests/api/` for the rspec tests if you want to test API only
-- `bin/spinach features/profile/` for the spinach tests if you want to test only profile pages
 
 ### Speed-up tests, rake tasks, and migrations
 
diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md
index 61fa5459b91..a76a5096b69 100644
--- a/doc/development/testing_guide/best_practices.md
+++ b/doc/development/testing_guide/best_practices.md
@@ -12,8 +12,7 @@ Here are some things to keep in mind regarding test performance:
 - `FactoryBot.build(...)` and `.build_stubbed` are faster than `.create`.
 - Don't `create` an object when `build`, `build_stubbed`, `attributes_for`,
   `spy`, or `double` will do. Database persistence is slow!
-- Don't mark a feature as requiring JavaScript (through `@javascript` in
-  Spinach or `:js` in RSpec) unless it's _actually_ required for the test
+- Don't mark a feature as requiring JavaScript (through `:js` in RSpec) unless it's _actually_ required for the test
   to be valid. Headless browser testing is slow!
 
 [parallelization]: ci.md#test-suite-parallelization-on-the-ci
@@ -134,11 +133,24 @@ really fast since:
 - gitlab-shell and Gitaly setup are skipped
 - Test repositories setup are skipped
 
-Note that in some cases, you might have to add some `require_dependency 'foo'`
-in your file under test since Rails autoloading is not available in these cases.
+`fast_spec_helper` also support autoloading classes that are located inside the
+`lib/` directory. It means that as long as your class / module is using only
+code from the `lib/` directory you will not need to explicitly load any
+dependencies. `fast_spec_helper` also loads all ActiveSupport extensions,
+including core extensions that are commonly used in the Rails environment.
 
-This shouldn't be a problem since explicitely listing dependencies should be
-considered a good practice anyway.
+Note that in some cases, you might still have to load some dependencies using
+`require_dependency` when a code is using gems or a dependency is not located
+in `lib/`.
+
+For example, if you want to test your code that is calling the
+`Gitlab::UntrustedRegexp` class, which under the hood uses `re2` library, you
+should either add `require_dependency 're2'` to files in your library that
+need `re2` gem, to make this requirement explicit, or you can add it to the
+spec itself, but the former is preferred.
+
+It takes around one second to load tests that are using `fast_spec_helper`
+instead of 30+ seconds in case of a regular `spec_helper`.
 
 ### `let` variables
 
diff --git a/doc/development/testing_guide/ci.md b/doc/development/testing_guide/ci.md
index e90de55068d..0d8e150e090 100644
--- a/doc/development/testing_guide/ci.md
+++ b/doc/development/testing_guide/ci.md
@@ -24,8 +24,7 @@ Our current CI parallelization setup is as follows:
   uploaded to S3.
 
 After that, the next pipeline will use the up-to-date
-`knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file. The same strategy
-is used for Spinach tests as well.
+`knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file. 
 
 ### Monitoring
 
diff --git a/doc/development/testing_guide/frontend_testing.md b/doc/development/testing_guide/frontend_testing.md
index 0d0d511582b..3b2b9c8c947 100644
--- a/doc/development/testing_guide/frontend_testing.md
+++ b/doc/development/testing_guide/frontend_testing.md
@@ -280,26 +280,6 @@ describe "Admin::AbuseReports", :js do
 end
 ```
 
-### Spinach errors due to missing JavaScript
-
-NOTE: **Note:** Since we are discouraging the use of Spinach when writing new
-feature tests, you shouldn't ever need to use this. This information is kept
-available for legacy purposes only.
-
-In Spinach, the JavaScript driver is enabled differently. In the `*.feature`
-file for the failing spec, add the `@javascript` flag above the Scenario:
-
-```
-@javascript
-Scenario: Developer can approve merge request
-  Given I am a "Shop" developer
-  And I visit project "Shop" merge requests page
-  And merge request 'Bug NS-04' must be approved
-  And I click link "Bug NS-04"
-  When I click link "Approve"
-  Then I should see approved merge request "Bug NS-04"
-```
-
 [jasmine-focus]: https://jasmine.github.io/2.5/focused_specs.html
 [jasmine-jquery]: https://github.com/velesin/jasmine-jquery
 [karma]: http://karma-runner.github.io/
diff --git a/doc/development/testing_guide/index.md b/doc/development/testing_guide/index.md
index 74d09eb91ff..0cd63a54b55 100644
--- a/doc/development/testing_guide/index.md
+++ b/doc/development/testing_guide/index.md
@@ -72,21 +72,6 @@ Everything you should know about how to run end-to-end tests using
 
 ---
 
-## Spinach (feature) tests
-
-GitLab [moved from Cucumber to Spinach](https://github.com/gitlabhq/gitlabhq/pull/1426)
-for its feature/integration tests in September 2012.
-
-As of March 2016, we are [trying to avoid adding new Spinach
-tests](https://gitlab.com/gitlab-org/gitlab-ce/issues/14121) going forward,
-opting for [RSpec feature](#features-integration) specs.
-
-Adding new Spinach scenarios is acceptable _only if_ the new scenario requires
-no more than one new `step` definition. If more than that is required, the
-test should be re-implemented using RSpec instead.
-
----
-
 [Return to Development documentation](../README.md)
 
 [^1]: /ci/yaml/README.html#dependencies
diff --git a/doc/development/testing_guide/testing_levels.md b/doc/development/testing_guide/testing_levels.md
index 51794f7f4df..07ced36f0c1 100644
--- a/doc/development/testing_guide/testing_levels.md
+++ b/doc/development/testing_guide/testing_levels.md
@@ -81,7 +81,6 @@ possible).
 | Tests path | Testing engine | Notes |
 | ---------- | -------------- | ----- |
 | `spec/features/` | [Capybara] + [RSpec] | If your spec has the `:js` metadata, the browser driver will be [Poltergeist], otherwise it's using [RackTest]. |
-| `features/` | Spinach | Spinach tests are deprecated, [you shouldn't add new Spinach tests](#spinach-feature-tests). |
 
 ### Consider **not** writing a system test!