diff options
Diffstat (limited to 'doc/development')
| -rw-r--r-- | doc/development/background_migrations.md | 9 | ||||
| -rw-r--r-- | doc/development/changelog.md | 54 | ||||
| -rw-r--r-- | doc/development/code_review.md | 3 | ||||
| -rw-r--r-- | doc/development/fe_guide/style_guide_js.md | 71 | ||||
| -rw-r--r-- | doc/development/migration_style_guide.md | 15 | ||||
| -rw-r--r-- | doc/development/rake_tasks.md | 17 | ||||
| -rw-r--r-- | doc/development/testing.md | 77 |
7 files changed, 201 insertions, 45 deletions
diff --git a/doc/development/background_migrations.md b/doc/development/background_migrations.md index e67db9ff142..f83a60e49e8 100644 --- a/doc/development/background_migrations.md +++ b/doc/development/background_migrations.md @@ -7,6 +7,11 @@ storing data in a single JSON column the data is stored in a separate table. ## When To Use Background Migrations +>**Note:** +When adding background migrations _you must_ make sure they are announced in the +monthly release post along with an estimate of how long it will take to complete +the migrations. + In the vast majority of cases you will want to use a regular Rails migration instead. Background migrations should _only_ be used when migrating _data_ in tables that have so many rows this process would take hours when performed in a @@ -91,6 +96,10 @@ BackgroundMigrationWorker.perform_bulk_in(5.minutes, jobs) ## Cleaning Up +>**Note:** +Cleaning up any remaining background migrations _must_ be done in either a major +or minor release, you _must not_ do this in a patch release. + Because background migrations can take a long time you can't immediately clean things up after scheduling them. For example, you can't drop a column that's used in the migration process as this would cause jobs to fail. This means that diff --git a/doc/development/changelog.md b/doc/development/changelog.md index ce39a379a0e..f869938fe11 100644 --- a/doc/development/changelog.md +++ b/doc/development/changelog.md @@ -15,11 +15,14 @@ following format: title: "Going through change[log]s" merge_request: 1972 author: Ozzy Osbourne +type: added ``` The `merge_request` value is a reference to a merge request that adds this entry, and the `author` key is used to give attribution to community contributors. **Both are optional**. +The `type` field maps the category of the change, +valid options are: added, fixed, changed, deprecated, removed, security, other. **Type field is mandatory**. Community contributors and core team members are encouraged to add their name to the `author` field. GitLab team members **should not**. @@ -94,6 +97,19 @@ Its simplest usage is to provide the value for `title`: $ bin/changelog 'Hey DZ, I added a feature to GitLab!' ``` +At this point the script would ask you to select the category of the change (mapped to the `type` field in the entry): + +```text +>> Please specify the category of your change: +1. New feature +2. Bug fix +3. Feature change +4. New deprecation +5. Feature removal +6. Security fix +7. Other +``` + The entry filename is based on the name of the current Git branch. If you run the command above on a branch called `feature/hey-dz`, it will generate a `changelogs/unreleased/feature-hey-dz.yml` file. @@ -106,26 +122,29 @@ create changelogs/unreleased/my-feature.yml title: Hey DZ, I added a feature to GitLab! merge_request: author: +type: ``` If you're working on the GitLab EE repository, the entry will be added to `changelogs/unreleased-ee/` instead. #### Arguments -| Argument | Shorthand | Purpose | -| ----------------- | --------- | --------------------------------------------- | -| [`--amend`] | | Amend the previous commit | -| [`--force`] | `-f` | Overwrite an existing entry | -| [`--merge-request`] | `-m` | Set merge request ID | -| [`--dry-run`] | `-n` | Don't actually write anything, just print | -| [`--git-username`] | `-u` | Use Git user.name configuration as the author | -| [`--help`] | `-h` | Print help message | +| Argument | Shorthand | Purpose | +| ----------------- | --------- | ---------------------------------------------------------------------------------------------------------- | +| [`--amend`] | | Amend the previous commit | +| [`--force`] | `-f` | Overwrite an existing entry | +| [`--merge-request`] | `-m` | Set merge request ID | +| [`--dry-run`] | `-n` | Don't actually write anything, just print | +| [`--git-username`] | `-u` | Use Git user.name configuration as the author | +| [`--type`] | `-t` | The category of the change, valid options are: added, fixed, changed, deprecated, removed, security, other | +| [`--help`] | `-h` | Print help message | [`--amend`]: #-amend [`--force`]: #-force-or-f [`--merge-request`]: #-merge-request-or-m [`--dry-run`]: #-dry-run-or-n [`--git-username`]: #-git-username-or-u +[`--type`]: #-type-or-t [`--help`]: #-help ##### `--amend` @@ -147,6 +166,7 @@ create changelogs/unreleased/feature-hey-dz.yml title: Added an awesome new feature to GitLab merge_request: author: +type: ``` ##### `--force` or `-f` @@ -164,6 +184,7 @@ create changelogs/unreleased/feature-hey-dz.yml title: Hey DZ, I added a feature to GitLab! merge_request: 1983 author: +type: ``` ##### `--merge-request` or `-m` @@ -178,6 +199,7 @@ create changelogs/unreleased/feature-hey-dz.yml title: Hey DZ, I added a feature to GitLab! merge_request: 1983 author: +type: ``` ##### `--dry-run` or `-n` @@ -192,6 +214,7 @@ create changelogs/unreleased/feature-hey-dz.yml title: Added an awesome new feature to GitLab merge_request: author: +type: $ ls changelogs/unreleased/ ``` @@ -211,6 +234,21 @@ create changelogs/unreleased/feature-hey-dz.yml title: Hey DZ, I added a feature to GitLab! merge_request: author: Jane Doe +type: +``` + +##### `--type` or `-t` + +Use the **`--type`** or **`-t`** argument to provide the `type` value: + +```text +$ bin/changelog 'Hey DZ, I added a feature to GitLab!' -t added +create changelogs/unreleased/feature-hey-dz.yml +--- +title: Hey DZ, I added a feature to GitLab! +merge_request: +author: +type: added ``` ### History and Reasoning diff --git a/doc/development/code_review.md b/doc/development/code_review.md index e3f37616757..64a89976300 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -11,6 +11,8 @@ There are a few rules to get your merge request accepted: **approved by a [frontend maintainer][projects]**. 1. If your merge request includes frontend and backend changes [^1], it must be **approved by a [frontend and a backend maintainer][projects]**. + 1. If your merge request includes a new dependency or a filesystem change, it must + be **approved by a [Build team member][team]**. See [how to work with the Build team][build handbook] for more details. 1. To lower the amount of merge requests maintainers need to review, you can ask or assign any [reviewers][projects] for a first review. 1. If you need some guidance (e.g. it's your first merge request), feel free @@ -194,3 +196,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 diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 149a0159680..9c72fda0229 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -11,7 +11,7 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns. #### ESlint -1. **Never** disable eslint rules unless you have a good reason. +1. **Never** disable eslint rules unless you have a good reason. You may see a lot of legacy files with `/* eslint-disable some-rule, some-other-rule */` at the top, but legacy files are a special case. Any time you develop a new feature or refactor an existing one, you should abide by the eslint rules. @@ -100,26 +100,44 @@ followed by any global declarations, then a blank newline prior to any imports o export default Foo; ``` -1. Relative paths: Unless you are writing a test, always reference other scripts using -relative paths instead of `~` - * In **app/assets/javascripts**: +1. Relative paths: when importing a module in the same directory, a child +directory, or an immediate parent directory prefer relative paths. When +importing a module which is two or more levels up, prefer either `~/` or `ee/` +. - ```javascript - // bad - import Foo from '~/foo' +In **app/assets/javascripts/my-feature/subdir**: - // good - import Foo from '../foo'; - ``` - * In **spec/javascripts**: +``` javascript +// bad +import Foo from '~/my-feature/foo'; +import Bar from '~/my-feature/subdir/bar'; +import Bin from '~/my-feature/subdir/lib/bin'; - ```javascript - // bad - import Foo from '../../app/assets/javascripts/foo' +// good +import Foo from '../foo'; +import Bar from './bar'; +import Bin from './lib/bin'; +``` - // good - import Foo from '~/foo'; - ``` +In **spec/javascripts**: + +``` javascript +// bad +import Foo from '../../app/assets/javascripts/my-feature/foo'; + +// good +import Foo from '~/my-feature/foo'; +``` + +When referencing an **EE component**: + +``` javascript +// bad +import Foo from '../../../../../ee/app/assets/javascripts/my-feature/ee-foo'; + +// good +import Foo from 'ee/my-feature/foo'; +``` 1. Avoid using IIFE. Although we have a lot of examples of files which wrap their contents in IIFEs (immediately-invoked function expressions), @@ -465,7 +483,7 @@ A forEach will cause side effects, it will be mutating the array being iterated. #### Vue and Boostrap 1. Tooltips: Do not rely on `has-tooltip` class name for Vue components - ``` javascript + ```javascript // bad <span class="has-tooltip" @@ -493,7 +511,24 @@ A forEach will cause side effects, it will be mutating the array being iterated. $('span').tooltip('fixTitle'); ``` +### The Javascript/Vue Accord +The goal of this accord is to make sure we are all on the same page. + +1. When writing Vue, you may not use jQuery in your application. +1.1 If you need to grab data from the DOM, you may query the DOM 1 time while bootstrapping your application to grab data attributes using `dataset`. You can do this without jQuery. +1.2 You may use a jQuery dependency in Vue.js following [this example from the docs](https://vuejs.org/v2/examples/select2.html). +1.3 If an outside jQuery Event needs to be listen to inside the Vue application, you may use jQuery event listeners. +1.4 We will avoid adding new jQuery events when they are not required. Instead of adding new jQuery events take a look at [different methods to do the same task](https://vuejs.org/v2/api/#vm-emit). + +1. You may query the `window` object 1 time, while bootstrapping your application for application specific data (e.g. `scrollTo` is ok to access anytime). Do this access during the bootstrapping of your application. + +1. You may have a temporary but immediate need to create technical debt by writing code that does not follow our standards, to be refactored later. Maintainers need to be ok with the tech debt in the first place. An issue should be created for that tech debt to evaluate it further and discuss. In the coming months you should fix that tech debt, with it's priority to be determined by maintainers. + +1. When creating tech debt you must write the tests for that code before hand and those tests may not be rewritten. e.g. jQuery tests rewritten to Vue tests. + +1. You may choose to use VueX as a centralized state management. If you choose not to use VueX, you must use the *store pattern* which can be found in the [Vue.js documentation](https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch). +1. Once you have chosen a centralized state management solution you must use it for your entire application. i.e. Don't mix and match your state management solutions. ## SCSS - [SCSS](style_guide_scss.md) diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 161d2544169..9b8ab5da74e 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -35,12 +35,11 @@ Please don't depend on GitLab-specific code since it can change in future versions. If needed copy-paste GitLab code into the migration to make it forward compatible. -## Commit Guidelines +## Schema Changes -Each migration **must** be added in its own commit with a descriptive commit -message. If a commit adds a migration it _should only_ include the migration and -any corresponding changes to `db/schema.rb`. This makes it easy to revert a -database migration without accidentally reverting other changes. +Migrations that make changes to the database schema (e.g. adding a column) can +only be added in the monthly release, patch releases may only contain data +migrations _unless_ schema changes are absolutely required to solve a problem. ## Downtime Tagging @@ -224,9 +223,9 @@ add_column(:projects, :foo, :integer, default: 10, limit: 8) ## Timestamp column type -By default, Rails uses the `timestamp` data type that stores timestamp data without timezone information. -The `timestamp` data type is used by calling either the `add_timestamps` or the `timestamps` method. -Also Rails converts the `:datetime` data type to the `timestamp` one. +By default, Rails uses the `timestamp` data type that stores timestamp data without timezone information. +The `timestamp` data type is used by calling either the `add_timestamps` or the `timestamps` method. +Also Rails converts the `:datetime` data type to the `timestamp` one. Example: diff --git a/doc/development/rake_tasks.md b/doc/development/rake_tasks.md index 42bb5e8619c..bfd80aab6a4 100644 --- a/doc/development/rake_tasks.md +++ b/doc/development/rake_tasks.md @@ -146,3 +146,20 @@ If new emoji are added, the spritesheet may change size. To compensate for such changes, first generate the `emoji.png` spritesheet with the above Rake task, then check the dimensions of the new spritesheet and update the `SPRITESHEET_WIDTH` and `SPRITESHEET_HEIGHT` constants accordingly. + +## Updating project templates + +Starting a project from a template needs this project to be exported. On a +up to date master branch with run: + +``` +gdk run db +# In a new terminal window +bundle exec rake gitlab:update_project_templates +git checkout -b update-project-templates +git add vendor/project_templates +git commit +git push -u origin update-project-templates +``` + +Now create a merge request and merge that to master. diff --git a/doc/development/testing.md b/doc/development/testing.md index e6aa4ae8f2f..efd56484b12 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -157,8 +157,9 @@ trade-off: - Unit tests are usually cheap, and you should consider them like the basement of your house: you need them to be confident that your code is behaving - correctly. However if you run only unit tests without integration / system tests, you might [miss] the [big] [picture]! -- Integration tests are a bit more expensive, but don't abuse them. A feature test + correctly. However if you run only unit tests without integration / system + tests, you might [miss] the [big] [picture]! +- Integration tests are a bit more expensive, but don't abuse them. A system test is often better than an integration test that is stubbing a lot of internals. - System tests are expensive (compared to unit tests), even more if they require a JavaScript driver. Make sure to follow the guidelines in the [Speed](#test-speed) @@ -188,24 +189,34 @@ Please consult the [dedicated "Frontend testing" guide](./fe_guide/testing.md). ### General Guidelines - Use a single, top-level `describe ClassName` block. -- Use `described_class` instead of repeating the class name being described - (_this is enforced by RuboCop_). - Use `.method` to describe class methods and `#method` to describe instance methods. - Use `context` to test branching logic. -- Use multi-line `do...end` blocks for `before` and `after`, even when it would - fit on a single line. - Don't assert against the absolute value of a sequence-generated attribute (see [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. -- Prefer `not_to` to `to_not` (_this is enforced by RuboCop_). - Try to match the ordering of tests to the ordering within the class. - Try to follow the [Four-Phase Test][four-phase-test] pattern, using newlines to separate phases. -- Try to use `Gitlab.config.gitlab.host` rather than hard coding `'localhost'` +- Use `Gitlab.config.gitlab.host` rather than hard coding `'localhost'` +- Don't assert against the absolute value of a sequence-generated attribute (see + [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` [four-phase-test]: https://robots.thoughtbot.com/four-phase-test +### Automatic retries and flaky tests detection + +On our CI, we use [rspec-retry] to automatically retry a failing example a few +times (see [`spec/spec_helper.rb`] for the precise retries count). + +We also use a home-made `RspecFlaky::Listener` listener which records flaky +examples in a JSON report file on `master` (`retrieve-tests-metadata` and `update-tests-metadata` jobs), and warns when a new flaky example +is detected in any other branch (`flaky-examples-check` job). In the future, the +`flaky-examples-check` job will not be allowed to fail. + +[rspec-retry]: https://github.com/NoRedInk/rspec-retry +[`spec/spec_helper.rb`]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/spec_helper.rb + ### `let` variables GitLab's RSpec suite has made extensive use of `let` variables to reduce @@ -268,6 +279,43 @@ end - Avoid scenario titles that add no information, such as "successfully". - Avoid scenario titles that repeat the feature title. +### Table-based / Parameterized tests + +This style of testing is used to exercise one piece of code with a comprehensive +range of inputs. By specifying the test case once, alongside a table of inputs +and the expected output for each, your tests can be made easier to read and more +compact. + +We use the [rspec-parameterized](https://github.com/tomykaira/rspec-parameterized) +gem. A short example, using the table syntax and checking Ruby equality for a +range of inputs, might look like this: + +```ruby +describe "#==" do + using Rspec::Parameterized::TableSyntax + + let(:project1) { create(:project) } + let(:project2) { create(:project) } + where(:a, :b, :result) do + 1 | 1 | true + 1 | 2 | false + true | true | true + true | false | false + project1 | project1 | true + project2 | project2 | true + project 1 | project2 | false + end + + with_them do + it { expect(a == b).to eq(result) } + + it 'is isomorphic' do + expect(b == a).to eq(result) + end + end +end +``` + ### Matchers Custom matchers should be created to clarify the intent and/or hide the @@ -276,6 +324,15 @@ complexity of RSpec expectations.They should be placed under a certain type of specs only (e.g. features, requests etc.) but shouldn't be if they apply to multiple type of specs. +#### have_gitlab_http_status + +Prefer `have_gitlab_http_status` over `have_http_status` because the former +could also show the response body whenever the status mismatched. This would +be very useful whenever some tests start breaking and we would love to know +why without editing the source and rerun the tests. + +This is especially useful whenever it's showing 500 internal server error. + ### Shared contexts All shared contexts should be be placed under `spec/support/shared_contexts/`. @@ -426,8 +483,6 @@ Here are some things to keep in mind regarding test performance: - `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 Git repository. Filesystem operations are 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 to be valid. Headless browser testing is slow! |