Skip to content

Commit 35bf7c7

Browse files
rymairymai
committed
Firs pass at improving the testing documentation
[ci skip] Signed-off-by: Rémy Coutable <[email protected]>
1 parent dbbf481 commit 35bf7c7

File tree

2 files changed

+194
-31
lines changed

2 files changed

+194
-31
lines changed

‎doc/development/fe_guide/testing.md

+3-4
Original file line numberDiff line numberDiff line change
@@ -80,18 +80,17 @@ If an integration test depends on JavaScript to run correctly, you need to make
8080
sure the spec is configured to enable JavaScript when the tests are run. If you
8181
don't do this you'll see vague error messages from the spec runner.
8282

83-
To enable a JavaScript driver in an `rspec` test, add `js: true` to the
83+
To enable a JavaScript driver in an `rspec` test, add `:js` to the
8484
individual spec or the context block containing multiple specs that need
8585
JavaScript enabled:
8686

8787
```ruby
88-
8988
# For one spec
90-
it 'presents information about abuse report', js: true do
89+
it 'presents information about abuse report', :js do
9190
# assertions...
9291
end
9392

94-
describe "Admin::AbuseReports", js: true do
93+
describe "Admin::AbuseReports", :js do
9594
it 'presents information about abuse report' do
9695
# assertions...
9796
end

‎doc/development/testing.md

+191-27
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,144 @@ this guide defines a rule that contradicts the thoughtbot guide, this guide
99
takes precedence. Some guidelines may be repeated verbatim to stress their
1010
importance.
1111

12+
## Definitions
13+
14+
### Unit tests
15+
16+
Formal definition: https://en.wikipedia.org/wiki/Unit_testing
17+
18+
These kind of tests ensure that a single unit of code (a method) works as expected (given an input, it has a predictable output). These tests should be isolated as much as possible (for instance model methods that don't do anything with the database shouldn't need a DB record).
19+
20+
| Code path | Tests path | Testing engine | Notes |
21+
| --------- | ---------- | -------------- | ----- |
22+
| `app/finders/` | `spec/finders/` | RSpec | |
23+
| `app/helpers/` | `spec/helpers/` | RSpec | |
24+
| `app/migrations/` | `spec/migrations/` | RSpec | |
25+
| `app/policies/` | `spec/policies/` | RSpec | |
26+
| `app/presenters/` | `spec/presenters/` | RSpec | |
27+
| `app/routing/` | `spec/routing/` | RSpec | |
28+
| `app/serializers/` | `spec/serializers/` | RSpec | |
29+
| `app/services/` | `spec/services/` | RSpec | |
30+
| `app/tasks/` | `spec/tasks/` | RSpec | |
31+
| `app/uploaders/` | `spec/uploaders/` | RSpec | |
32+
| `app/views/` | `spec/views/` | RSpec | |
33+
| `app/workers/` | `spec/workers/` | RSpec | |
34+
| `app/assets/javascripts/` | `spec/javascripts/` | Karma | More details in the [JavaScript](#javascript) section. |
35+
36+
### Integration tests
37+
38+
Formal definition: https://en.wikipedia.org/wiki/Integration_testing
39+
40+
These kind of tests ensure that individual parts of the application work well together, without the overhead of the actual app environment (i.e. the browser). These tests should assert at the request/response level: status code, headers, body. They're useful to test permissions, redirections, what view is rendered etc.
41+
42+
| Code path | Tests path | Testing engine | Notes |
43+
| --------- | ---------- | -------------- | ----- |
44+
| `app/controllers/` | `spec/controllers/` | RSpec | |
45+
| `lib/api/` | `spec/requests/api/` | RSpec | |
46+
| `lib/ci/api/` | `spec/requests/ci/api/` | RSpec | |
47+
| `app/assets/javascripts/` | `spec/javascripts/` | Karma | More details in the [JavaScript](#javascript) section. |
48+
49+
#### About controller tests
50+
51+
In an ideal world, controllers should be thin. However, when this is not the
52+
case, it's acceptable to write a system test without JavaScript instead of a
53+
controller test. The reason is that testing a fat controller usually involves a
54+
lot of stubbing, things like:
55+
56+
```ruby
57+
controller.instance_variable_set(:@user, user)
58+
```
59+
60+
and use methods which are deprecated in Rails 5 ([#23768]).
61+
62+
[#23768]: https://gitlab.com/gitlab-org/gitlab-ce/issues/23768
63+
64+
#### About Karma
65+
66+
As you may have noticed, Karma is both in the Unit tests and the Integration
67+
tests category. That's because Karma is a tool that provides an environment to
68+
run JavaScript tests, so you can either run unit tests (e.g. test a single
69+
JavaScript method), or integration tests (e.g. test a component that is composed
70+
of multiple components).
71+
72+
### System tests or Feature tests
73+
74+
Formal definition: https://en.wikipedia.org/wiki/System_testing.
75+
76+
These kind of tests ensure the application works as expected from a user point
77+
of view (aka black-box testing). These tests should test a happy path for a
78+
given page or set of pages, and a test case should be added for any regression
79+
that couldn't have been caught at lower levels with better tests (i.e. if a
80+
regression is found, regression tests should be added at the lowest-level
81+
possible).
82+
83+
| Tests path | Testing engine | Notes |
84+
| ---------- | -------------- | ----- |
85+
| `spec/features/` | [Capybara] + [RSpec] | If your spec has the `:js` metadata, the browser driver will be [Poltergeist], otherwise it's using [RackTest]. |
86+
| `features/` | Spinach | Spinach tests are deprecated, [you shouldn't add new Spinach tests](#spinach-feature-tests). |
87+
88+
[Capybara]: https://github.com/teamcapybara/capybara
89+
[RSpec]: https://github.com/rspec/rspec-rails#feature-specs
90+
[Poltergeist]: https://github.com/teamcapybara/capybara#poltergeist
91+
[RackTest]: https://github.com/teamcapybara/capybara#racktest
92+
93+
#### Good practices
94+
95+
- Create only the necessary records in the database
96+
- Test a happy path and a less happy path but that's it
97+
- Every other possible paths should be tested with Unit or Integration tests
98+
- Test what's displayed on the page, not the internal of ActiveRecord models
99+
- It's ok to look for DOM elements but don't abuse it since it makes the tests
100+
more brittle
101+
102+
If we're confident that the low-level components work well (and we should be if
103+
we have enough Unit & Integration tests), we shouldn't need to duplicate their
104+
thorough testing at the System test level.
105+
106+
It's very easy to add tests, but a lot harder to remove or improve tests, so one
107+
should take care of not introducing too many (slow and duplicated) specs.
108+
109+
The reason why we should follow these good practices are as follows:
110+
111+
- System tests are slow to run since they spin up the entire application stack
112+
in a headless browser, and even slower when they integrate a JS driver
113+
- With System tests run with a driver that supports JavaScript, the tests are
114+
run in different thread than the application. This means it does not share a
115+
database connection and your test will have to commit the transactions in
116+
order for the running application to see the data (and vice-versa). In that
117+
case we need to truncate the database after each spec instead of simply
118+
rolling back a transaction (the faster strategy that's in use for other kind
119+
of tests). This is slower than transactions, however, so we want to use
120+
truncation only when necessary.
121+
122+
## How to test at the correct level?
123+
124+
As many things in life, deciding what to test at each level of testing is a
125+
trade-off:
126+
127+
- Unit tests are usually cheap, and you should consider them like the basement
128+
of your house: you need them to be confident that your code is behaving
129+
correctly. However if you run only unit tests without integration / system tests, you might miss the [big] [picture]!
130+
- Integration tests are bit more expensive but don't abuse them. A feature test
131+
is often better than an integration test that is stubbing a lot of internals.
132+
- System tests are expensive (compared to unit tests), even more if they require
133+
a JavaScript driver. Make sure to follow the guidelines in the [Speed](#test-speed)
134+
section.
135+
136+
Another way to see it is to think about the "cost of tests", this is well
137+
explained [in this article][tests-cost] and the basic idea is that the cost of a
138+
test includes:
139+
140+
- The time it takes to write the test
141+
- The time it takes to run the test every time the suite runs
142+
- The time it takes to understand the test
143+
- The time it takes to fix the test if it breaks and the underlying code is OK
144+
- Maybe, the time it takes to change the code to make the code testable.
145+
146+
[big]: https://twitter.com/timbray/status/822470746773409794
147+
[picture]: https://twitter.com/withzombies/status/829716565834752000
148+
[tests-cost]: https://medium.com/table-xi/high-cost-tests-and-high-value-tests-a86e27a54df#.2ulyh3a4e
149+
12150
## Factories
13151

14152
GitLab uses [factory_girl] as a test fixture replacement.
@@ -117,11 +255,20 @@ it 'is overdue' do
117255
end
118256
```
119257

120-
### Test speed
258+
### System / Features tests
121259

122-
GitLab has a massive test suite that, without parallelization, can take more
123-
than an hour to run. It's important that we make an effort to write tests that
124-
are accurate and effective _as well as_ fast.
260+
- Feature specs should be named `ROLE_ACTION_spec.rb`, such as
261+
`user_changes_password_spec.rb`.
262+
- Use only one `feature` block per feature spec file.
263+
- Use scenario titles that describe the success and failure paths.
264+
- Avoid scenario titles that add no information, such as "successfully".
265+
- Avoid scenario titles that repeat the feature title.
266+
267+
## Test speed
268+
269+
GitLab has a massive test suite that, without [parallelization], can take hours
270+
to run. It's important that we make an effort to write tests that are accurate
271+
and effective _as well as_ fast.
125272

126273
Here are some things to keep in mind regarding test performance:
127274

@@ -132,38 +279,40 @@ Here are some things to keep in mind regarding test performance:
132279
- Use `create(:empty_project)` instead of `create(:project)` when you don't need
133280
the underlying Git repository. Filesystem operations are slow!
134281
- Don't mark a feature as requiring JavaScript (through `@javascript` in
135-
Spinach or `js: true` in RSpec) unless it's _actually_ required for the test
282+
Spinach or `:js` in RSpec) unless it's _actually_ required for the test
136283
to be valid. Headless browser testing is slow!
137284

138-
### Features / Integration
285+
[parallelization]: #test-suite-parallelization-on-the-ci
139286

140-
GitLab uses [rspec-rails feature specs] to test features in a browser
141-
environment. These are [capybara] specs running on the headless [poltergeist]
142-
driver.
287+
### Monitoring
143288

144-
- Feature specs live in `spec/features/` and should be named
145-
`ROLE_ACTION_spec.rb`, such as `user_changes_password_spec.rb`.
146-
- Use only one `feature` block per feature spec file.
147-
- Use scenario titles that describe the success and failure paths.
148-
- Avoid scenario titles that add no information, such as "successfully."
149-
- Avoid scenario titles that repeat the feature title.
289+
The GitLab test suite is [monitored] and a [public dashboard] is available for
290+
everyone to see. Feel free to look at the slowest test files and try to improve
291+
them.
150292

151-
[rspec-rails feature specs]: https://github.com/rspec/rspec-rails#feature-specs
152-
[capybara]: https://github.com/teamcapybara/capybara
153-
[poltergeist]: https://github.com/teampoltergeist/poltergeist
293+
[monitored]: /development/performance.html#rspec-profiling
294+
[public dashboard]: https://redash.gitlab.com/public/dashboards/l1WhHXaxrCWM5Ai9D7YDqHKehq6OU3bx5gssaiWe?org_slug=default
154295

155-
## Spinach (feature) tests
296+
## Test suite parallelization on the CI
156297

157-
GitLab [moved from Cucumber to Spinach](https://github.com/gitlabhq/gitlabhq/pull/1426)
158-
for its feature/integration tests in September 2012.
298+
Our current CI parallelization setup is as follows:
159299

160-
As of March 2016, we are [trying to avoid adding new Spinach
161-
tests](https://gitlab.com/gitlab-org/gitlab-ce/issues/14121) going forward,
162-
opting for [RSpec feature](#features-integration) specs.
300+
1. The `knapsack` job in the prepare stage that is supposed to ensure we have a `knapsack/rspec_report.json` file:
301+
- The `knapsack/rspec_report.json` file is fetched from the cache with the
302+
`knapsack` key, if it's not here we initialize the file with `{}`.
303+
1. Each `rspec x y` job are run with `knapsack rspec` and should have an evenly
304+
distributed share of tests:
305+
- It works because the jobs have access to the `knapsack/rspec_report.json`
306+
since the "artifacts from all previous stages are passed by default". [^1]
307+
- the jobs set their own report path to `KNAPSACK_REPORT_PATH=knapsack/spinach_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json`
308+
- if knapsack is doing its job, test files that are run should be listed under
309+
`Report specs`, not under `Leftover specs`
310+
1. The `update-knapsack` job takes all the `knapsack/spinach_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json` files from
311+
the `rspec x y` jobs and merge them all together into a single
312+
`knapsack/rspec_report.json` that is then cached with the `knapsack` key
163313

164-
Adding new Spinach scenarios is acceptable _only if_ the new scenario requires
165-
no more than one new `step` definition. If more than that is required, the
166-
test should be re-implemented using RSpec instead.
314+
After that, the next pipeline will use the up-to-date
315+
`knapsack/rspec_report.json` file.
167316

168317
## Testing Rake Tasks
169318

@@ -201,6 +350,21 @@ describe 'gitlab:shell rake tasks' do
201350
end
202351
```
203352

353+
## Spinach (feature) tests
354+
355+
GitLab [moved from Cucumber to Spinach](https://github.com/gitlabhq/gitlabhq/pull/1426)
356+
for its feature/integration tests in September 2012.
357+
358+
As of March 2016, we are [trying to avoid adding new Spinach
359+
tests](https://gitlab.com/gitlab-org/gitlab-ce/issues/14121) going forward,
360+
opting for [RSpec feature](#features-integration) specs.
361+
362+
Adding new Spinach scenarios is acceptable _only if_ the new scenario requires
363+
no more than one new `step` definition. If more than that is required, the
364+
test should be re-implemented using RSpec instead.
365+
204366
---
205367

206368
[Return to Development documentation](README.md)
369+
370+
[^1]: /ci/yaml/README.html#dependencies

0 commit comments

Comments
 (0)