From fb8b0041d86080f0f8d53f13be1016b0b199a47f Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 30 Mar 2016 18:53:30 -0400 Subject: First pass at a Testing styleguide [ci skip] --- doc/development/README.md | 1 + doc/development/testing.md | 105 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 doc/development/testing.md (limited to 'doc/development') diff --git a/doc/development/README.md b/doc/development/README.md index 1b281809afc..a8bc4fe5abb 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -9,4 +9,5 @@ - [Shell commands](shell_commands.md) in the GitLab codebase - [Sidekiq debugging](sidekiq_debugging.md) - [SQL guidelines](sql.md) for SQL guidelines +- [Testing standards and style guidelines](testing.md) - [UI guide](ui_guide.md) for building GitLab with existing css styles and elements diff --git a/doc/development/testing.md b/doc/development/testing.md new file mode 100644 index 00000000000..d37ef5d8785 --- /dev/null +++ b/doc/development/testing.md @@ -0,0 +1,105 @@ +# Testing Standards and Style Guidelines + +This guide outlines standards and best practices for automated testing of GitLab +CE and EE. + +It is meant to be an _extension_ of the [thoughtbot testing +styleguide](https://github.com/thoughtbot/guides/tree/master/style/testing). If +this guide defines a rule that contradicts the thoughtbot guide, this guide +takes precedence. Some guidelines may be repeated verbatim to stress their +importance. + +## Factories + +GitLab uses [factory_girl] as a test +fixture replacement. + +- Factory definitions live in `spec/factories/`, named using the pluralization + of their corresponding model (`User` factories are defined in `users.rb`). +- There should be only one top-level factory definition per file. +- Make use of [Traits] to clean up definitions and usages. +- When defining a factory, don't define attributes that are not required for the + resulting record to pass validation. +- When instantiating from a factory, don't supply extraneous attributes that + aren't required by the test. + +[factory_girl]: https://github.com/thoughtbot/factory_girl +[Traits]: http://www.rubydoc.info/gems/factory_girl/file/GETTING_STARTED.md#Traits + +## JavaScript + +GitLab uses [Teaspoon] to run its [Jasmine] JavaScript specs. They can be run on +the command line via `bundle exec teaspoon`, or via a web browser at +`http://localhost:3000/teaspoon` when the Rails server is running. + +- JavaScript tests live in `spec/javascripts/`, matching the folder structure of + `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js.coffee` has a corresponding + `spec/javascripts/behaviors/autosize_spec.js.coffee` file. +- Haml fixtures required for JavaScript tests live in + `spec/javascripts/fixtures`. They should contain the bare minimum amount of + markup necessary for the test. + + > **Warning:** Keep in mind that a Rails view may change and + invalidate your test, but everything will still pass because your fixture + doesn't reflect the latest view. + +- Keep in mind that in a CI environment, these tests are run in a headless + browser and you will not have access to certain APIs, such as + [`Notification`](https://developer.mozilla.org/en-US/docs/Web/API/notification), + which will have to be stubbed. + +[Teaspoon]: https://github.com/modeset/teaspoon +[Jasmine]: https://github.com/jasmine/jasmine + +## RSpec + +### General Guidelines + +- Use a single, top-level `describe ClassName` block. +- Use `described_class` instead of repeating the class name being described. +- Use `.method` to describe class methods and `#method` to describe instance + methods. +- Use `context` to test branching logic. +- Don't `describe` symbols (see [Gotchas](gotchas.md#dont-describe-symbols)). +- Prefer `not_to` to `to_not`. +- Try to match the ordering of tests to the ordering within the class. + +### Test speed + +GitLab has a massive test suite that, without parallelization, can take more +than an hour to run. It's important that we make an effort to write tests that +are accurate and effective _as well as_ fast. + +Here are some things to keep in mind regarding test performance: + +- `double` and `spy` are faster than `FactoryGirl.build(...)` +- `FactoryGirl.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! +- Use `create(:empty_project)` instead of `create(:project)` when you don't need + the underlying repository. Filesystem operations are slow! +- Don't mark a feature as requiring JavaScript (through `@javascript` in + Spinach or `js: true` in RSpec) unless it's _actually_ required for the test + to be valid. Headless browser testing is slow! + +### Features / Integration + +- Feature specs live in `spec/features/` and should be named + `ROLE_ACTION_spec.rb`, such as `user_changes_password_spec.rb`. +- Use only one `feature` block per feature spec file. +- Use scenario titles that describe the success and failure paths. +- Avoid scenario titles that add no information, such as "successfully." +- Avoid scenario titles that repeat the feature title. + +## 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. -- cgit v1.2.1 From 0b9d9816f8ef1d871a050cea5d8bc3d9203c3d18 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 30 Mar 2016 19:11:02 -0400 Subject: Add a note about Four-Phase Test to Testing guide [ci skip] --- doc/development/testing.md | 2 ++ 1 file changed, 2 insertions(+) (limited to 'doc/development') diff --git a/doc/development/testing.md b/doc/development/testing.md index d37ef5d8785..a7e85ecebed 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -63,6 +63,8 @@ the command line via `bundle exec teaspoon`, or via a web browser at - Don't `describe` symbols (see [Gotchas](gotchas.md#dont-describe-symbols)). - Prefer `not_to` to `to_not`. - Try to match the ordering of tests to the ordering within the class. +- Try to follow the [Four-Phase Test](https://robots.thoughtbot.com/four-phase-test) + pattern, using newlines to separate phases. ### Test speed -- cgit v1.2.1 From 60f4081e135bdcd893d60192e652c4b829c656dd Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 1 Apr 2016 20:29:48 -0400 Subject: Factories don't have to be limited to `ActiveRecord` objects [ci skip] --- doc/development/testing.md | 2 ++ 1 file changed, 2 insertions(+) (limited to 'doc/development') diff --git a/doc/development/testing.md b/doc/development/testing.md index a7e85ecebed..3d1c4ccab47 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -22,6 +22,8 @@ fixture replacement. resulting record to pass validation. - When instantiating from a factory, don't supply extraneous attributes that aren't required by the test. +- Factories don't have to be limited to `ActiveRecord` objects. + [See example](https://gitlab.com/gitlab-org/gitlab-ce/commit/0b8cefd3b2385a21cfed779bd659978c0402766d). [factory_girl]: https://github.com/thoughtbot/factory_girl [Traits]: http://www.rubydoc.info/gems/factory_girl/file/GETTING_STARTED.md#Traits -- cgit v1.2.1 From 2e83b562030e078ce6d37f81915590426a57c820 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 1 Apr 2016 20:30:24 -0400 Subject: Add a section about `let` to the Testing guide [ci skip] --- doc/development/testing.md | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) (limited to 'doc/development') diff --git a/doc/development/testing.md b/doc/development/testing.md index 3d1c4ccab47..187d852e533 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -11,8 +11,7 @@ importance. ## Factories -GitLab uses [factory_girl] as a test -fixture replacement. +GitLab uses [factory_girl] as a test fixture replacement. - Factory definitions live in `spec/factories/`, named using the pluralization of their corresponding model (`User` factories are defined in `users.rb`). @@ -65,8 +64,30 @@ the command line via `bundle exec teaspoon`, or via a web browser at - Don't `describe` symbols (see [Gotchas](gotchas.md#dont-describe-symbols)). - Prefer `not_to` to `to_not`. - Try to match the ordering of tests to the ordering within the class. -- Try to follow the [Four-Phase Test](https://robots.thoughtbot.com/four-phase-test) - pattern, using newlines to separate phases. +- Try to follow the [Four-Phase Test][four-phase-test] pattern, using newlines + to separate phases. + +[four-phase-test]: https://robots.thoughtbot.com/four-phase-test + +### `let` variables + +GitLab's RSpec suite has made extensive use of `let` variables to reduce +duplication. However, this sometimes [comes at the cost of clarity][lets-not], +so we need to set some guidelines for their use going forward: + +- `let` variables are preferable to instance variables. Local variables are + preferable to `let` variables. +- Use `let` to reduce duplication throughout an entire spec file. +- Don't use `let` to define variables used by a single test; define them as + local variables inside the test's `it` block. +- Don't define a `let` variable inside the top-level `describe` block that's + only used in a more deeply-nested `context` or `describe` block. Keep the + definition as close as possible to where it's used. +- Try to avoid overriding the definition of one `let` variable with another. +- Don't define a `let` variable that's only used by the definition of another. + Use a helper method instead. + +[lets-not]: https://robots.thoughtbot.com/lets-not ### Test speed -- cgit v1.2.1 From 6ffda88c973ce9be263d0e9142a3105f2cccf000 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 1 Apr 2016 20:36:44 -0400 Subject: Add a link back to Development documentation to Testing guide [ci skip] --- doc/development/testing.md | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'doc/development') diff --git a/doc/development/testing.md b/doc/development/testing.md index 187d852e533..23417845f16 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -128,3 +128,7 @@ 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) -- cgit v1.2.1 From 3911e77eb4244f010af4050c140bc2d08e3c34df Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Tue, 5 Apr 2016 23:08:22 +0200 Subject: Remove TODO for not documented stuff --- doc/development/ui_guide.md | 4 ---- 1 file changed, 4 deletions(-) (limited to 'doc/development') diff --git a/doc/development/ui_guide.md b/doc/development/ui_guide.md index 2f01defc11d..a3e260a5f89 100644 --- a/doc/development/ui_guide.md +++ b/doc/development/ui_guide.md @@ -1,9 +1,5 @@ # UI Guide for building GitLab -## Best practices for creating new pages in GitLab - -TODO: write some best practices when develop GitLab features. - ## GitLab UI development kit We created a page inside GitLab where you can check commonly used html and css elements. -- cgit v1.2.1 From cedcc1453cbbd4a8ba43cf932b8765d891f19e0c Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 Apr 2016 01:19:21 -0400 Subject: Minor Testing guide copyedits [ci skip] --- doc/development/testing.md | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) (limited to 'doc/development') diff --git a/doc/development/testing.md b/doc/development/testing.md index 23417845f16..672e3fb4649 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -16,16 +16,18 @@ GitLab uses [factory_girl] as a test fixture replacement. - Factory definitions live in `spec/factories/`, named using the pluralization of their corresponding model (`User` factories are defined in `users.rb`). - There should be only one top-level factory definition per file. -- Make use of [Traits] to clean up definitions and usages. +- FactoryGirl methods are mixed in to all RSpec groups. This means you can (and + should) call `create(...)` instead of `FactoryGirl.create(...)`. +- Make use of [traits] to clean up definitions and usages. - When defining a factory, don't define attributes that are not required for the resulting record to pass validation. -- When instantiating from a factory, don't supply extraneous attributes that - aren't required by the test. +- When instantiating from a factory, don't supply attributes that aren't + required by the test. - Factories don't have to be limited to `ActiveRecord` objects. [See example](https://gitlab.com/gitlab-org/gitlab-ce/commit/0b8cefd3b2385a21cfed779bd659978c0402766d). [factory_girl]: https://github.com/thoughtbot/factory_girl -[Traits]: http://www.rubydoc.info/gems/factory_girl/file/GETTING_STARTED.md#Traits +[traits]: http://www.rubydoc.info/gems/factory_girl/file/GETTING_STARTED.md#Traits ## JavaScript @@ -40,9 +42,9 @@ the command line via `bundle exec teaspoon`, or via a web browser at `spec/javascripts/fixtures`. They should contain the bare minimum amount of markup necessary for the test. - > **Warning:** Keep in mind that a Rails view may change and - invalidate your test, but everything will still pass because your fixture - doesn't reflect the latest view. + > **Warning:** Keep in mind that a Rails view may change and + invalidate your test, but everything will still pass because your fixture + doesn't reflect the latest view. - Keep in mind that in a CI environment, these tests are run in a headless browser and you will not have access to certain APIs, such as @@ -102,7 +104,7 @@ Here are some things to keep in mind regarding test performance: - Don't `create` an object when `build`, `build_stubbed`, `attributes_for`, `spy`, or `double` will do. Database persistence is slow! - Use `create(:empty_project)` instead of `create(:project)` when you don't need - the underlying repository. Filesystem operations are slow! + the underlying Git repository. Filesystem operations are slow! - Don't mark a feature as requiring JavaScript (through `@javascript` in Spinach or `js: true` in RSpec) unless it's _actually_ required for the test to be valid. Headless browser testing is slow! -- cgit v1.2.1 From c7ec5929b1f580e7078d11774df2e8c0d1abbe03 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 Apr 2016 21:49:47 -0400 Subject: First pass at a Code Review guide Largely borrowed from thoughtbot's code review guide, so attribution is included. [ci skip] --- doc/development/README.md | 2 ++ doc/development/code_review.md | 75 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 doc/development/code_review.md (limited to 'doc/development') diff --git a/doc/development/README.md b/doc/development/README.md index 2f4e7845ccc..3f3ef068f96 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -2,6 +2,8 @@ - [Architecture](architecture.md) of GitLab - [CI setup](ci_setup.md) for testing GitLab +- [Code review guidelines](code_review.md) for reviewing code and having code + reviewed. - [Gotchas](gotchas.md) to avoid - [How to dump production data to staging](db_dump.md) - [Instrumentation](instrumentation.md) diff --git a/doc/development/code_review.md b/doc/development/code_review.md new file mode 100644 index 00000000000..ab4236404c6 --- /dev/null +++ b/doc/development/code_review.md @@ -0,0 +1,75 @@ +# Code Review Guidelines + +This guide contains advice and best practices for performing code review, and +having your code reviewed. + +All merge requests for GitLab CE and EE, whether written by a GitLab team member +or a volunteer contributor, must go through a code review process to ensure the +code is effective, understandable, and maintainable. + +Any developer can, and is encouraged to, perform code review on merge requests +of colleagues and contributors. However, the final decision to accept a merge +request is up to one of our merge request "endbosses", denoted on the +[team page](https://about.gitlab.com/team). + +## Everyone + +- Accept that many programming decisions are opinions. Discuss tradeoffs, which + you prefer, and reach a resolution quickly. +- Ask questions; don't make demands. ("What do you think about naming this + `:user_id`?") +- Ask for clarification. ("I didn't understand. Can you clarify?") +- Avoid selective ownership of code. ("mine", "not mine", "yours") +- Avoid using terms that could be seen as referring to personal traits. ("dumb", + "stupid"). Assume everyone is attractive, intelligent, and well-meaning. +- Be explicit. Remember people don't always understand your intentions online. +- Be humble. ("I'm not sure - let's look it up.") +- Don't use hyperbole. ("always", "never", "endlessly", "nothing") +- Consider one-on-one chats or video calls if there are too many "I didn't + understand" or "Alternative solution:" comments. Post a follow-up comment + summarizing one-on-one discussion. + +## Having your code reviewed + +- The first reviewer of your code is _you_. Before you perform that first push + of your shiny new branch, read through the entire diff. Does it make sense? + Did you include something unrelated to the overall purpose of the changes? Did + you forget to remove any debugging code? +- Be grateful for the reviewer's suggestions. ("Good call. I'll make that + change.") +- Don't take it personally. The review is of the code, not of you. +- Explain why the code exists. ("It's like that because of these reasons. Would + it be more clear if I rename this class/file/method/variable?") +- Extract unrelated changes and refactorings into future merge requests/issues. +- Seek to understand the reviewer's perspective. +- Try to respond to every comment. +- Push commits based on earlier rounds of feedback as isolated commits to the + branch. Do not squash until the branch is ready to merge. Reviewers should be + able to read individual updates based on their earlier feedback. + +## Reviewing code + +Understand why the change is necessary (fixes a bug, improves the user +experience, refactors the existing code). Then: + +- Communicate which ideas you feel strongly about and those you don't. +- Identify ways to simplify the code while still solving the problem. +- Offer alternative implementations, but assume the author already considered + them. ("What do you think about using a custom validator here?") +- Seek to understand the author's perspective. +- If you don't understand a piece of code, _say so_. There's a good chance + someone else would be confused by it as well. +- After a round of line notes, it can be helpful to post a summary note such as + "LGTM :thumbsup:", or "Just a couple things to address." +- Avoid accepting a merge request before the build succeeds ("Merge when build + succeeds" is fine). + +## Credits + +Largely based on the [thoughtbot code review guide]. + +[thoughtbot code review guide]: https://github.com/thoughtbot/guides/tree/master/code-review + +--- + +[Return to Development documentation](README.md) -- cgit v1.2.1 From 0c6923e2d13b6648c8e08017450a01e7068edfb9 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sun, 10 Apr 2016 22:54:32 -0400 Subject: Re-add a note about sarcasm to the Code Review guide [ci skip] --- doc/development/code_review.md | 3 +++ 1 file changed, 3 insertions(+) (limited to 'doc/development') diff --git a/doc/development/code_review.md b/doc/development/code_review.md index ab4236404c6..40ae55ab905 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -25,6 +25,9 @@ request is up to one of our merge request "endbosses", denoted on the - Be explicit. Remember people don't always understand your intentions online. - Be humble. ("I'm not sure - let's look it up.") - Don't use hyperbole. ("always", "never", "endlessly", "nothing") +- Be careful about the use of sarcasm. Everything we do is public; what seems + like good-natured ribbing to you and a long-time colleague might come off as + mean and unwelcoming to a person new to the project. - Consider one-on-one chats or video calls if there are too many "I didn't understand" or "Alternative solution:" comments. Post a follow-up comment summarizing one-on-one discussion. -- cgit v1.2.1 From 16926a676bdea4bfbbbaf9d390373073d2ff8bbd Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 11 Apr 2016 12:23:37 +0200 Subject: Store block timings as transaction values This makes it easier to query, simplifies the code, and makes it possible to figure out what transaction the data belongs to (simply because it's now stored _in_ the transaction). This new setup keeps track of both the real/wall time _and_ CPU time spent in a block, both measured using milliseconds (to keep all units the same). --- doc/development/instrumentation.md | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) (limited to 'doc/development') diff --git a/doc/development/instrumentation.md b/doc/development/instrumentation.md index c0192bd6709..f7e148fb3e4 100644 --- a/doc/development/instrumentation.md +++ b/doc/development/instrumentation.md @@ -2,36 +2,34 @@ GitLab Performance Monitoring allows instrumenting of custom blocks of Ruby code. This can be used to measure the time spent in a specific part of a larger -chunk of code. The resulting data is written to a separate series. +chunk of code. The resulting data is stored as a field in the transaction that +executed the block. -To start measuring a block of Ruby code you should use -`Gitlab::Metrics.measure` and give it a name for the series to store the data -in: +To start measuring a block of Ruby code you should use `Gitlab::Metrics.measure` +and give it a name: ```ruby -Gitlab::Metrics.measure(:user_logins) do +Gitlab::Metrics.measure(:foo) do ... end ``` -The first argument of this method is the series name and should be plural. This -name will be prefixed with `rails_` or `sidekiq_` depending on whether the code -was run in the Rails application or one of the Sidekiq workers. In the -above example the final series names would be as follows: +Two values are measured for a block: -- rails_user_logins -- sidekiq_user_logins +1. The real time elapsed, stored in NAME_real_time +2. The CPU time elapsed, stored in NAME_cpu_time -Series names should be plural as this keeps the naming style in line with the -other series names. +Both the real and CPU timings are measured in milliseconds. -By default metrics measured using a block contain a single value, "duration", -which contains the number of milliseconds it took to execute the block. Custom -values can be added by passing a Hash as the 2nd argument. Custom tags can be -added by passing a Hash as the 3rd argument. A simple example is as follows: +Multiple calls to the same block will result in the final values being the sum +of all individual values. Take this code for example: ```ruby -Gitlab::Metrics.measure(:example_series, { number: 10 }, { class: self.class.to_s }) do - ... +3.times do + Gitlab::Metrics.measure(:sleep) do + sleep 1 + end end ``` + +Here the final value of `sleep_real_time` will be `3`, _not_ `1`. -- cgit v1.2.1 From d9110a7ecab52ab0716a42c2075cebdf8028d5e7 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 11 Apr 2016 13:27:24 +0200 Subject: Track call counts in Gitlab::Metrics.measure_block --- doc/development/instrumentation.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'doc/development') diff --git a/doc/development/instrumentation.md b/doc/development/instrumentation.md index f7e148fb3e4..c1cf2e77c26 100644 --- a/doc/development/instrumentation.md +++ b/doc/development/instrumentation.md @@ -14,10 +14,11 @@ Gitlab::Metrics.measure(:foo) do end ``` -Two values are measured for a block: +3 values are measured for a block: -1. The real time elapsed, stored in NAME_real_time -2. The CPU time elapsed, stored in NAME_cpu_time +1. The real time elapsed, stored in NAME_real_time. +2. The CPU time elapsed, stored in NAME_cpu_time. +3. The call count, stored in NAME_call_count. Both the real and CPU timings are measured in milliseconds. -- cgit v1.2.1