diff options
Diffstat (limited to 'doc/development')
| -rw-r--r-- | doc/development/README.md | 4 | ||||
| -rw-r--r-- | doc/development/api_styleguide.md | 56 | ||||
| -rw-r--r-- | doc/development/architecture.md | 2 | ||||
| -rw-r--r-- | doc/development/contributing/issue_workflow.md | 10 | ||||
| -rw-r--r-- | doc/development/go_guide/index.md | 18 | ||||
| -rw-r--r-- | doc/development/i18n/externalization.md | 59 | ||||
| -rw-r--r-- | doc/development/img/architecture_simplified.png | bin | 36325 -> 21239 bytes | |||
| -rw-r--r-- | doc/development/logging.md | 8 | ||||
| -rw-r--r-- | doc/development/pipelines.md | 18 | ||||
| -rw-r--r-- | doc/development/refactoring_guide/index.md | 77 | ||||
| -rw-r--r-- | doc/development/testing_guide/frontend_testing.md | 9 |
11 files changed, 244 insertions, 17 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index 16858b0c58e..b041e48f0c2 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -171,6 +171,10 @@ Complementary reads: - [Testing standards and style guidelines](testing_guide/index.md) - [Frontend testing standards and style guidelines](testing_guide/frontend_testing.md) +## Refactoring guides + +- [Refactoring guidelines](refactoring_guide/index.md) + ## Documentation guides - [Writing documentation](documentation/index.md) diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md index 37d8a677389..78e35023766 100644 --- a/doc/development/api_styleguide.md +++ b/doc/development/api_styleguide.md @@ -15,7 +15,7 @@ Always use an [Entity] to present the endpoint's payload. API endpoints must come with [documentation](documentation/styleguide.md#api), unless it is internal or behind a feature flag. The docs should be in the same merge request, or, if strictly necessary, -in a follow-up with the same milestone as the original merge request. +in a follow-up with the same milestone as the original merge request. ## Methods and parameters description @@ -125,6 +125,58 @@ different components are making use of. [validation, and coercion of the parameters]: https://github.com/ruby-grape/grape#parameter-validation-and-coercion [installing GitLab under a relative URL]: https://docs.gitlab.com/ee/install/relative_url.html +## Avoiding N+1 problems + +In order to avoid N+1 problems that are common when returning collections +of records in an API endpoint, we should use eager loading. + +A standard way to do this within the API is for models to implement a +scope called `with_api_entity_associations` that will preload the +associations and data returned in the API. An example of this scope can +be seen in +[the `Issue` model](https://gitlab.com/gitlab-org/gitlab/blob/2fedc47b97837ea08c3016cf2fb773a0300a4a25/app%2Fmodels%2Fissue.rb#L62). + +In situations where the same model has multiple entities in the API +(for instance, `UserBasic`, `User` and `UserPublic`) you should use your +discretion with applying this scope. It may be that you optimize for the +most basic entity, with successive entities building upon that scope. + +The `with_api_entity_associations` scope will also [automatically preload +data](https://gitlab.com/gitlab-org/gitlab/blob/19f74903240e209736c7668132e6a5a735954e7c/app%2Fmodels%2Ftodo.rb#L34) +for `Todo` _targets_ when returned in the Todos API. + +For more context and discussion about preloading see +[this merge request](https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/25711) +which introduced the scope. + +### Verifying with tests + +When an API endpoint returns collections, always add a test to verify +that the API endpoint does not have an N+1 problem, now and in the future. +We can do this using [`ActiveRecord::QueryRecorder`](query_recorder.md). + +Example: + +```ruby +def make_api_request + get api('/foo', personal_access_token: pat) +end + +it 'avoids N+1 queries', :request_store do + # Firstly, record how many PostgreSQL queries the endpoint will make + # when it returns a single record + create_record + + control = ActiveRecord::QueryRecorder.new { make_api_request } + + # Now create a second record and ensure that the API does not execute + # any more queries than before + create_record + + expect { make_api_request }.not_to exceed_query_limit(control) +end +``` + ## Testing When writing tests for new API endpoints, consider using a schema [fixture](./testing_guide/best_practices.md#fixtures) located in `/spec/fixtures/api/schemas`. You can `expect` a response to match a given schema: @@ -132,3 +184,5 @@ When writing tests for new API endpoints, consider using a schema [fixture](./te ```ruby expect(response).to match_response_schema('merge_requests') ``` + +Also see [verifying N+1 performance](#verifying-with-tests) in tests. diff --git a/doc/development/architecture.md b/doc/development/architecture.md index 436cb43199e..bbd5ca3c494 100644 --- a/doc/development/architecture.md +++ b/doc/development/architecture.md @@ -314,7 +314,7 @@ You can use it either for personal or business websites, such as portfolios, doc - Layer: Core Service (Processor) - GitLab.com: [Runner](../user/gitlab_com/index.md#shared-runners) -GitLab Runner runs tests and sends the results to GitLab. +GitLab Runner runs jobs and sends the results to GitLab. GitLab CI/CD is the open-source continuous integration service included with GitLab that coordinates the testing. The old name of this project was `GitLab CI Multi Runner` but please use `GitLab Runner` (without CI) from now on. diff --git a/doc/development/contributing/issue_workflow.md b/doc/development/contributing/issue_workflow.md index c8705a174af..13ff35ed65c 100644 --- a/doc/development/contributing/issue_workflow.md +++ b/doc/development/contributing/issue_workflow.md @@ -113,7 +113,7 @@ Stage labels respects the `devops::<stage_key>` naming convention. <https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/data/stages.yml> with `_` replaced with a space. -For instance, the "Manage" stage is represented by the ~"devops::manage" label in +For instance, the "Manage" stage is represented by the `~"devops::manage"` label in the `gitlab-org` group since its key under `stages` is `manage`. The current stage labels can be found by [searching the labels list for `devops::`](https://gitlab.com/groups/gitlab-org/-/labels?search=devops::). @@ -156,10 +156,10 @@ As a team needs some way to collect the work their members are planning to be as Normally there is a 1:1 relationship between Stage labels and Group labels. In the spirit of "Everyone can contribute", any issue can be picked up by any group, depending on current priorities. When picking up an issue belonging to a different -group, it should be relabelled. For example, if an issue labelled ~"devops::create" -and ~"group::knowledge" is picked up by someone in the Access group of the Plan stage, -the issue should be relabelled as ~"group::access" while keeping the original -~"devops::create" unchanged. +group, it should be relabelled. For example, if an issue labelled `~"devops::create"` +and `~"group::knowledge"` is picked up by someone in the Access group of the Plan stage, +the issue should be relabelled as `~"group::access"` while keeping the original +`~"devops::create"` unchanged. We also use stage and group labels to help quantify our [throughput](https://about.gitlab.com/handbook/engineering/management/throughput/). Please read [Stage and Group labels in Throughput](https://about.gitlab.com/handbook/engineering/management/throughput/#stage-and-group-labels-in-throughput) for more information on how the labels are used in this context. diff --git a/doc/development/go_guide/index.md b/doc/development/go_guide/index.md index 975b524c1df..c1dfb220df8 100644 --- a/doc/development/go_guide/index.md +++ b/doc/development/go_guide/index.md @@ -379,6 +379,24 @@ are: To reduce unnecessary differences between two distribution methods, Omnibus and CNG **should always use the same Go version**. +## Secure Team standards and style guidelines + +The following are some style guidelines that are specific to the Secure Team. + +### Code style and format + +Use `goimports -local gitlab.com/gitlab-org` before committing. +[goimports](https://godoc.org/golang.org/x/tools/cmd/goimports) +is a tool that automatically formats Go source code using +[Gofmt](https://golang.org/cmd/gofmt/), in addition to formatting import lines, +adding missing ones and removing unreferenced ones. +By using the `-local gitlab.com/gitlab-org` option, `goimports` will group locally referenced +packages separately from external ones. See +[the imports section](https://github.com/golang/go/wiki/CodeReviewComments#imports) +of the Code Review Comments page on the Go wiki for more details. +Most editors/IDEs will allow you to run commands before/after saving a file, you can set it +up to run `goimports -local gitlab.com/gitlab-org` so that it's applied to every file when saving. + --- [Return to Development documentation](../README.md). diff --git a/doc/development/i18n/externalization.md b/doc/development/i18n/externalization.md index 91ca6120db9..7ddcd426fd7 100644 --- a/doc/development/i18n/externalization.md +++ b/doc/development/i18n/externalization.md @@ -306,6 +306,65 @@ This makes use of [`Intl.DateTimeFormat`](https://developer.mozilla.org/en-US/do ## Best practices +### Keep translations dynamic + +There are cases when it makes sense to keep translations together within an array or a hash. + +Examples: + +- Mappings for a dropdown list +- Error messages + +To store these kinds of data, using a constant seems like the best choice, however this won't work for translations. + +Bad, avoid it: + +```ruby +class MyPresenter + MY_LIST = { + key_1: _('item 1'), + key_2: _('item 2'), + key_3: _('item 3') + } +end +``` + +The translation method (`_`) will be called when the class is loaded for the first time and translates the text to the default locale. Regardless of what's the user's locale, these values will not be translated again. + +Similar thing happens when using class methods with memoization. + +Bad, avoid it: + +```ruby +class MyModel + def self.list + @list ||= { + key_1: _('item 1'), + key_2: _('item 2'), + key_3: _('item 3') + } + end +end +``` + +This method will memoize the translations using the locale of the user, who first "called" this method. + +To avoid these problems, keep the translations dynamic. + +Good: + +```ruby +class MyPresenter + def self.my_list + { + key_1: _('item 1'), + key_2: _('item 2'), + key_3: _('item 3') + }.freeze + end +end +``` + ### Splitting sentences Please never split a sentence as that would assume the sentence grammar and diff --git a/doc/development/img/architecture_simplified.png b/doc/development/img/architecture_simplified.png Binary files differindex 4899993310f..46ae2b3c055 100644 --- a/doc/development/img/architecture_simplified.png +++ b/doc/development/img/architecture_simplified.png diff --git a/doc/development/logging.md b/doc/development/logging.md index f10737da766..ef2d2d7022d 100644 --- a/doc/development/logging.md +++ b/doc/development/logging.md @@ -164,6 +164,14 @@ Resources: - [Elasticsearch mapping - avoiding type gotchas](https://www.elastic.co/guide/en/elasticsearch/guide/current/mapping.html#_avoiding_type_gotchas) - [Elasticsearch mapping types]( https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-types.html) +#### Logging durations + +Similar to timezones, choosing the right time unit to log can impose avoidable overhead. So, whenever +challenged to choose between seconds, milliseconds or any other unit, lean towards _seconds_ as float. + +In order to make it easier to track timings in the logs, make sure the log key has `_s` as +suffix and `duration` within its name (e.g., `view_duration_s`). + ## Multi-destination Logging GitLab is transitioning from unstructured/plaintext logs to structured/JSON logs. During this transition period some logs will be recorded in multiple formats through multi-destination logging. diff --git a/doc/development/pipelines.md b/doc/development/pipelines.md index aecfe3af9c4..9ba6dfc110a 100644 --- a/doc/development/pipelines.md +++ b/doc/development/pipelines.md @@ -151,15 +151,15 @@ request, be sure to start the `dont-interrupt-me` job before pushing. ## PostgreSQL versions testing -We follow [the PostgreSQL versions Omnibus support policy](https://gitlab.com/groups/gitlab-org/-/epics/2184#proposal): - -| | 12.10 (April 2020) | 13.0 (May 2020) | 13.1 (June 2020) | 13.2 (July 2020) | 13.3 (August 2020) | 13.4, 13.5 | 13.6 (November 2020) | 14.0 (May 2021?) | -| ------ | ------------------ | --------------- | ---------------- | ---------------- | ------------------ | ------------ | -------------------- | -------------------- | -| PG9.6 | nightly | - | - | - | - | - | - | - | -| PG10 | `master` | - | - | - | - | - | - | - | -| PG11 | MRs/`master` | MRs/`master` | MRs/`master` | MRs/`master` | MRs/`master` | MRs/`master` | nightly | - | -| PG12 | - | - | - | - | `master` | `master` | MRs/`master` | `master` | -| PG13 | - | - | - | - | - | - | - | MRs/`master` | +We follow the [PostgreSQL versions shipped with Omnibus GitLab](https://docs.gitlab.com/omnibus/package-information/postgresql_versions.html): + +| | 12.10 (April 2020) | 13.0 (May 2020) | 13.1 (June 2020) | 13.2 (July 2020) | 13.3 (August 2020) | 13.4, 13.5 | 13.6 (November 2020) | 14.0 (May 2021?) | +| ------ | ------------------ | --------------- | ---------------- | ---------------- | ------------------ | ------------ | -------------------- | ---------------- | +| PG9.6 | nightly | - | - | - | - | - | - | - | +| PG10 | `master` | - | - | - | - | - | - | - | +| PG11 | MRs/`master` | MRs/`master` | MRs/`master` | MRs/`master` | MRs/`master` | MRs/`master` | nightly | - | +| PG12 | - | - | - | - | `master` | `master` | MRs/`master` | `master` | +| PG13 | - | - | - | - | - | - | - | MRs/`master` | ## Directed acyclic graph diff --git a/doc/development/refactoring_guide/index.md b/doc/development/refactoring_guide/index.md new file mode 100644 index 00000000000..4bd9d0e9c11 --- /dev/null +++ b/doc/development/refactoring_guide/index.md @@ -0,0 +1,77 @@ +# Refactoring guide + +This document is a collection of techniques and best practices to consider while performing a refactor. + +## Pinning tests + +Pinning tests help you ensure that you don't unintentionally change the output or behavior of the entity you're refactoring. This even includes preserving any existing *buggy* behavior, since consumers may rely on those bugs implicitly. + +### Example steps + +1. Identify all the possible inputs to the refactor subject (e.g. anything that's injected into the template or used in a conditional). +1. For each possible input, identify the significant possible values. +1. Create a test to save a full detailed snapshot for each helpful combination values per input. This should guarantee that we have "pinned down" the current behavior. The snapshot could be literally a screenshot, a dump of HTML, or even an ordered list of debugging statements. +1. Run all the pinning tests against the code, before you start refactoring (Oracle) +1. Perform the refactor (or checkout the commit with the work done) +1. Run again all the pinning test against the post refactor code (Pin) +1. Compare the Oracle with the Pin. If the Pin is different, you know the refactoring doesn't preserve existing behavior. +1. Repeat the previous three steps as necessary until the refactoring is complete. + +### Example commit history + +Leaving in the commits for adding and removing pins helps others checkout and verify the result of the test. + +```bash +AAAAAA Add pinning tests to funky_foo +BBBBBB Refactor funky_foo into nice_foo +CCCCCC Remove pinning tests for funky_foo +``` + +Then you can leave a reviewer instructions on how to run the pinning test in your MR. Example: + +> First revert the commit which removes the pin. +> +> ```bash +> git revert --no-commit $(git log -1 --grep="Remove pinning test for funky_foo" --pretty=format:"%H") +> ``` +> +> Then run the test +> +> ```bash +> yarn run jest path/to/funky_foo_pin_spec.js +> ``` + +### Try to keep pins green + +It's hard for a refactor to be 100% pure. This means that a pin which captures absolutely everything is bound to fail with +some trivial and expected differences. Try to keep the pins green by cleaning the pin with the expected changes. This helps +others quickly verify that a refactor was safe. + +[Example](https://gitlab.com/gitlab-org/gitlab/-/commit/7b73da4078a60cf18f5c10c712c66c302174f506?merge_request_iid=29528#a061e6835fd577ccf6802c8a476f4e9d47466d16_0_23): + +```javascript +// funky_foo_pin_spec.js + +const cleanForSnapshot = el => { + Array.from(rootEl.querySelectorAll('[data-deprecated-attribute]')).forEach(el => { + el.removeAttribute('data-deprecated-attribute'); + }); +}; + +// ... + +expect(cleanForSnapshot(wrapper.element)).toMatchSnapshot(); +``` + +### Resources + +[Unofficial wiki explanation](http://wiki.c2.com/?PinningTests) + +### Examples + +- [Pinning test in a haml to vue refactor](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/27691#pinning-tests) +- [Pinning test in isolating a bug](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/32198#note_212736225) +- [Pinning test in refactoring dropdown](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28173) +- [Pinning test in refactoring vulnerability_details.vue](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25830/commits) +- [Pinning test in refactoring notes_award_list.vue](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/29528#pinning-test) +- [Video of pair programming session on pinning tests](https://youtu.be/LrakPcspBK4) diff --git a/doc/development/testing_guide/frontend_testing.md b/doc/development/testing_guide/frontend_testing.md index 429ec262250..2a3fcf122a6 100644 --- a/doc/development/testing_guide/frontend_testing.md +++ b/doc/development/testing_guide/frontend_testing.md @@ -206,12 +206,19 @@ Following you'll find some general common practices you will find as part of our When it comes to querying DOM elements in your tests, it is best to uniquely target the element, without adding additional attributes specifically for testing purposes. Sometimes this cannot be done feasibly. In these cases, adding test attributes to simplify the selectors might be the best option. -Preferentially, in component testing with `@vue/test-utils`, you should query for child components using the component itself. Otherwise, try to use an existing attribute like `name` or a Vue `ref` (if using `@vue/test-utils`): +Preferentially, in component testing with `@vue/test-utils`, you should query for child components using the component itself. This helps enforce that specific behavior can be covered by that component's individual unit tests. Otherwise, try to use: + +- A behavioral attribute like `name` (also verifies that `name` was setup properly) +- A `data-testid` attribute ([recommended by maintainers of `@vue/test-utils`](https://github.com/vuejs/vue-test-utils/issues/1498#issuecomment-610133465)) +- a Vue `ref` (if using `@vue/test-utils`) + +Examples: ```javascript it('exists', () => { wrapper.find(FooComponent); wrapper.find('input[name=foo]'); + wrapper.find('[data-testid="foo"]'); wrapper.find({ ref: 'foo'}); wrapper.find('.js-foo'); }); |
