summaryrefslogtreecommitdiff
path: root/doc/development
diff options
context:
space:
mode:
Diffstat (limited to 'doc/development')
-rw-r--r--doc/development/code_review.md32
-rw-r--r--doc/development/database_debugging.md34
-rw-r--r--doc/development/diffs.md2
-rw-r--r--doc/development/documentation/feature-change-workflow.md30
-rw-r--r--doc/development/fe_guide/icons.md16
-rw-r--r--doc/development/file_storage.md4
-rw-r--r--doc/development/geo.md26
-rw-r--r--doc/development/git_object_deduplication.md110
-rw-r--r--doc/development/go_guide/index.md37
-rw-r--r--doc/development/new_fe_guide/style/javascript.md5
-rw-r--r--doc/development/testing_guide/end_to_end/page_objects.md38
-rw-r--r--doc/development/testing_guide/end_to_end/quick_start_guide.md33
-rw-r--r--doc/development/testing_guide/frontend_testing.md28
-rw-r--r--doc/development/testing_guide/review_apps.md8
14 files changed, 241 insertions, 162 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md
index 35ce6b8cbe4..e9c8b193309 100644
--- a/doc/development/code_review.md
+++ b/doc/development/code_review.md
@@ -45,9 +45,9 @@ page, with these behaviours:
1. It will not pick people whose [GitLab status](../user/profile/index.md#current-status)
contains the string 'OOO'.
-2. [Trainee maintainers](https://about.gitlab.com/handbook/engineering/workflow/code-review/#trainee-maintainer)
+1. [Trainee maintainers](https://about.gitlab.com/handbook/engineering/workflow/code-review/#trainee-maintainer)
are three times as likely to be picked as other reviewers.
-3. It always picks the same reviewers and maintainers for the same
+1. It always picks the same reviewers and maintainers for the same
branch name (unless their OOO status changes, as in point 1). It
removes leading `ce-` and `ee-`, and trailing `-ce` and `-ee`, so
that it can be stable for backport branches.
@@ -58,20 +58,20 @@ As described in the section on the responsibility of the maintainer below, you
are recommended to get your merge request approved and merged by maintainer(s)
from teams other than your own.
- 1. If your merge request includes backend changes [^1], it must be
- **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_backend)**.
- 1. If your merge request includes database migrations or changes to expensive queries [^2], it must be
- **approved by a [database maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_database)**.
- 1. If your merge request includes frontend changes [^1], it must be
- **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_frontend)**.
- 1. If your merge request includes UX changes [^1], it must be
- **approved by a [UX team member][team]**.
- 1. If your merge request includes adding a new JavaScript library [^1], it must be
- **approved by a [frontend lead][team]**.
- 1. If your merge request includes adding a new UI/UX paradigm [^1], it must be
- **approved by a [UX lead][team]**.
- 1. If your merge request includes a new dependency or a filesystem change, it must be
- **approved by a [Distribution team member][team]**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) for more details.
+1. If your merge request includes backend changes [^1], it must be
+ **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_backend)**.
+1. If your merge request includes database migrations or changes to expensive queries [^2], it must be
+ **approved by a [database maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_database)**.
+1. If your merge request includes frontend changes [^1], it must be
+ **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_frontend)**.
+1. If your merge request includes UX changes [^1], it must be
+ **approved by a [UX team member][team]**.
+1. If your merge request includes adding a new JavaScript library [^1], it must be
+ **approved by a [frontend lead][team]**.
+1. If your merge request includes adding a new UI/UX paradigm [^1], it must be
+ **approved by a [UX lead][team]**.
+1. If your merge request includes a new dependency or a filesystem change, it must be
+ **approved by a [Distribution team member][team]**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) for more details.
#### Security requirements
diff --git a/doc/development/database_debugging.md b/doc/development/database_debugging.md
index 0311eda1ff1..eb3b227473b 100644
--- a/doc/development/database_debugging.md
+++ b/doc/development/database_debugging.md
@@ -9,31 +9,31 @@ An easy first step is to search for your error in Slack or google "GitLab (my er
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)
+- `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)
## Nuke everything and start over
If you just want to delete everything and start over with an empty DB (~1 minute):
- - `bundle exec rake db:reset RAILS_ENV=development`
+- `bundle exec rake db:reset RAILS_ENV=development`
If you just want to delete everything and start over with dummy data (~40 minutes). This also does `db:reset` and runs DB-specific migrations:
- - `bundle exec rake dev:setup RAILS_ENV=development`
+- `bundle exec rake dev:setup RAILS_ENV=development`
If your test DB is giving you problems, it is safe to nuke it because it doesn't contain important data:
- - `bundle exec rake db:reset RAILS_ENV=test`
+- `bundle exec rake db:reset RAILS_ENV=test`
## Migration wrangling
- - `bundle exec rake db:migrate RAILS_ENV=development`: Execute any pending migrations that you may have picked up from a MR
- - `bundle exec rake db:migrate:status RAILS_ENV=development`: Check if all migrations are `up` or `down`
- - `bundle exec rake db:migrate:down VERSION=20170926203418 RAILS_ENV=development`: Tear down a migration
- - `bundle exec rake db:migrate:up VERSION=20170926203418 RAILS_ENV=development`: Set up a migration
- - `bundle exec rake db:migrate:redo VERSION=20170926203418 RAILS_ENV=development`: Re-run a specific migration
+- `bundle exec rake db:migrate RAILS_ENV=development`: Execute any pending migrations that you may have picked up from a MR
+- `bundle exec rake db:migrate:status RAILS_ENV=development`: Check if all migrations are `up` or `down`
+- `bundle exec rake db:migrate:down VERSION=20170926203418 RAILS_ENV=development`: Tear down a migration
+- `bundle exec rake db:migrate:up VERSION=20170926203418 RAILS_ENV=development`: Set up a migration
+- `bundle exec rake db:migrate:redo VERSION=20170926203418 RAILS_ENV=development`: Re-run a specific migration
## Manually access the database
@@ -45,12 +45,12 @@ bundle exec rails dbconsole RAILS_ENV=development
bundle exec rails db RAILS_ENV=development
```
- - `\q`: Quit/exit
- - `\dt`: List all tables
- - `\d+ issues`: List columns for `issues` table
- - `CREATE TABLE board_labels();`: Create a table called `board_labels`
- - `SELECT * FROM schema_migrations WHERE version = '20170926203418';`: Check if a migration was run
- - `DELETE FROM schema_migrations WHERE version = '20170926203418';`: Manually remove a migration
+- `\q`: Quit/exit
+- `\dt`: List all tables
+- `\d+ issues`: List columns for `issues` table
+- `CREATE TABLE board_labels();`: Create a table called `board_labels`
+- `SELECT * FROM schema_migrations WHERE version = '20170926203418';`: Check if a migration was run
+- `DELETE FROM schema_migrations WHERE version = '20170926203418';`: Manually remove a migration
## FAQ
diff --git a/doc/development/diffs.md b/doc/development/diffs.md
index 5655398c886..ac0b8555360 100644
--- a/doc/development/diffs.md
+++ b/doc/development/diffs.md
@@ -133,4 +133,4 @@ File diff will be suppressed (technically different from collapsed, but behaves
Diff Viewers, which can be found on `models/diff_viewer/*` are classes used to map metadata about each type of Diff File. It has information
whether it's a binary, which partial should be used to render it or which File extensions this class accounts for.
-`DiffViewer::Base` validates _blobs_ (old and new versions) content, extension and file type in order to check if it can be rendered. \ No newline at end of file
+`DiffViewer::Base` validates _blobs_ (old and new versions) content, extension and file type in order to check if it can be rendered.
diff --git a/doc/development/documentation/feature-change-workflow.md b/doc/development/documentation/feature-change-workflow.md
index ca29353ecbe..ac93ada5a4b 100644
--- a/doc/development/documentation/feature-change-workflow.md
+++ b/doc/development/documentation/feature-change-workflow.md
@@ -121,27 +121,27 @@ All reviewers can help ensure accuracy, clarity, completeness, and adherence to
- **Prior to merging**, documentation changes committed by the developer must be reviewed by:
- 1. **The code reviewer** for the MR, to confirm accuracy, clarity, and completeness.
- 1. Optionally: Others involved in the work, such as other devs or the PM.
- 1. Optionally: The technical writer for the DevOps stage. If not prior to merging, the technical writer will review after the merge.
- This helps us ensure that the developer has time to merge good content by the freeze, and that it can be further refined by the release, if needed.
- - To decide whether to request this review before the merge, consider the amount of time left before the code freeze, the size of the change,
- and your degree of confidence in having users of an RC use your docs as written.
- - Pre-merge tech writer reviews should be most common when the code is complete well in advance of the freeze and/or for larger documentation changes.
- - You can request a review and if there is not sufficient time to complete it prior to the freeze,
- the maintainer can merge the current doc changes (if complete) and create a follow-up doc review issue.
- - The technical writer can also help decide what docs to merge before the freeze and whether to work on further changes in a follow up MR.
- - **To request a pre-merge technical writer review**, assign the writer listed for the applicable [DevOps stage](https://about.gitlab.com/handbook/product/categories/#devops-stages).
- - **To request a post-merge technical writer review**, [create an issue for one using the Doc Review template](https://gitlab.com/gitlab-org/gitlab-ce/issues/new?issuable_template=Doc%20Review) and link it from the MR that makes the doc change.
- 1. **The maintainer** who is assigned to merge the MR, to verify clarity, completeness, and quality, to the best of their ability.
+ 1. **The code reviewer** for the MR, to confirm accuracy, clarity, and completeness.
+ 1. Optionally: Others involved in the work, such as other devs or the PM.
+ 1. Optionally: The technical writer for the DevOps stage. If not prior to merging, the technical writer will review after the merge.
+ This helps us ensure that the developer has time to merge good content by the freeze, and that it can be further refined by the release, if needed.
+ - To decide whether to request this review before the merge, consider the amount of time left before the code freeze, the size of the change,
+ and your degree of confidence in having users of an RC use your docs as written.
+ - Pre-merge tech writer reviews should be most common when the code is complete well in advance of the freeze and/or for larger documentation changes.
+ - You can request a review and if there is not sufficient time to complete it prior to the freeze,
+ the maintainer can merge the current doc changes (if complete) and create a follow-up doc review issue.
+ - The technical writer can also help decide what docs to merge before the freeze and whether to work on further changes in a follow up MR.
+ - **To request a pre-merge technical writer review**, assign the writer listed for the applicable [DevOps stage](https://about.gitlab.com/handbook/product/categories/#devops-stages).
+ - **To request a post-merge technical writer review**, [create an issue for one using the Doc Review template](https://gitlab.com/gitlab-org/gitlab-ce/issues/new?issuable_template=Doc%20Review) and link it from the MR that makes the doc change.
+ 1. **The maintainer** who is assigned to merge the MR, to verify clarity, completeness, and quality, to the best of their ability.
- Upon merging, if a technical writer review has not been performed and there is not yet a linked issue for a follow-up review, the maintainer should [create an issue using the Doc Review template](https://gitlab.com/gitlab-org/gitlab-ce/issues/new?issuable_template=Doc%20Review), link it from the MR, and
mention the original MR author in the new issue. Alternatively, the maintainer can ask the MR author to create and link this issue before the MR is merged.
- After merging, documentation changes are reviewed by:
- 1. The technical writer--**if** their review was not performed prior to the merge.
- 2. Optionally: by the PM (for accuracy and to ensure it's consistent with the vision for how the product will be used).
+ 1. The technical writer -- **if** their review was not performed prior to the merge.
+ 1. Optionally: by the PM (for accuracy and to ensure it's consistent with the vision for how the product will be used).
Any party can raise the item to the PM for review at any point: the dev, the technical writer, or the PM, who can request/plan a review at the outset.
### Technical Writer role
diff --git a/doc/development/fe_guide/icons.md b/doc/development/fe_guide/icons.md
index 533e2001300..4f687d8642e 100644
--- a/doc/development/fe_guide/icons.md
+++ b/doc/development/fe_guide/icons.md
@@ -21,10 +21,10 @@ To use a sprite Icon in HAML or Rails we use a specific helper function :
sprite_icon(icon_name, size: nil, css_class: '')
```
-- **icon_name** Use the icon_name that you can find in the SVG Sprite
- ([Overview is available here][svg-preview]).
-- **size (optional)** Use one of the following sizes : 16, 24, 32, 48, 72 (this will be translated into a `s16` class)
-- **css_class (optional)** If you want to add additional css classes
+- **icon_name** Use the icon_name that you can find in the SVG Sprite
+ ([Overview is available here][svg-preview]).
+- **size (optional)** Use one of the following sizes : 16, 24, 32, 48, 72 (this will be translated into a `s16` class)
+- **css_class (optional)** If you want to add additional css classes
**Example**
@@ -65,10 +65,10 @@ export default {
</template>
```
-- **name** Name of the Icon in the SVG Sprite ([Overview is available here][svg-preview]).
-- **size (optional)** Number value for the size which is then mapped to a specific CSS class
- (Available Sizes: 8, 12, 16, 18, 24, 32, 48, 72 are mapped to `sXX` css classes)
-- **css-classes (optional)** Additional CSS Classes to add to the svg tag.
+- **name** Name of the Icon in the SVG Sprite ([Overview is available here][svg-preview]).
+- **size (optional)** Number value for the size which is then mapped to a specific CSS class
+ (Available Sizes: 8, 12, 16, 18, 24, 32, 48, 72 are mapped to `sXX` css classes)
+- **css-classes (optional)** Additional CSS Classes to add to the svg tag.
### Usage in HTML/JS
diff --git a/doc/development/file_storage.md b/doc/development/file_storage.md
index 02874d18a30..475d1c1611e 100644
--- a/doc/development/file_storage.md
+++ b/doc/development/file_storage.md
@@ -92,8 +92,8 @@ in your uploader, you need to either 1) include `RecordsUpload::Concern` and pre
The `CarrierWave::Uploader#store_dir` is overridden to
- - `GitlabUploader.base_dir` + `GitlabUploader.dynamic_segment` when the store is LOCAL
- - `GitlabUploader.dynamic_segment` when the store is REMOTE (the bucket name is used to namespace)
+- `GitlabUploader.base_dir` + `GitlabUploader.dynamic_segment` when the store is LOCAL
+- `GitlabUploader.dynamic_segment` when the store is REMOTE (the bucket name is used to namespace)
### Using `ObjectStorage::Extension::RecordsUploads`
diff --git a/doc/development/geo.md b/doc/development/geo.md
index 685d4e44ad3..24f16eae9fa 100644
--- a/doc/development/geo.md
+++ b/doc/development/geo.md
@@ -341,9 +341,9 @@ not used, so sessions etc. aren't shared between nodes.
GitLab can optionally use Object Storage to store data it would
otherwise store on disk. These things can be:
- - LFS Objects
- - CI Job Artifacts
- - Uploads
+- LFS Objects
+- CI Job Artifacts
+- Uploads
Objects that are stored in object storage, are not handled by Geo. Geo
ignores items in object storage. Either:
@@ -412,15 +412,15 @@ The Geo **primary** stores events in the `geo_event_log` table. Each
entry in the log contains a specific type of event. These type of
events include:
- - Repository Deleted event
- - Repository Renamed event
- - Repositories Changed event
- - Repository Created event
- - Hashed Storage Migrated event
- - Lfs Object Deleted event
- - Hashed Storage Attachments event
- - Job Artifact Deleted event
- - Upload Deleted event
+- Repository Deleted event
+- Repository Renamed event
+- Repositories Changed event
+- Repository Created event
+- Hashed Storage Migrated event
+- Lfs Object Deleted event
+- Hashed Storage Attachments event
+- Job Artifact Deleted event
+- Upload Deleted event
### Geo Log Cursor
@@ -526,4 +526,4 @@ old method:
- Replication is synchronous and we preserve the order of events.
- Replication of the events happen at the same time as the changes in the
- database.
+ database.
diff --git a/doc/development/git_object_deduplication.md b/doc/development/git_object_deduplication.md
index c103a4527ff..5ce59891afa 100644
--- a/doc/development/git_object_deduplication.md
+++ b/doc/development/git_object_deduplication.md
@@ -79,11 +79,11 @@ at the Rails application level in SQL.
In conclusion, we need three things for effective object deduplication
across a collection of GitLab project repositories at the Git level:
-1. A pool repository must exist.
-2. The participating project repositories must be linked to the pool
- repository via their respective `objects/info/alternates` files.
-3. The pool repository must contain Git object data common to the
- participating project repositories.
+1. A pool repository must exist.
+1. The participating project repositories must be linked to the pool
+ repository via their respective `objects/info/alternates` files.
+1. The pool repository must contain Git object data common to the
+ participating project repositories.
### Deduplication factor
@@ -105,71 +105,71 @@ With pool repositories we made a fresh start. These live in their own
`pool_repositories` SQL table. The relations between these two tables
are as follows:
-- a `Project` belongs to at most one `PoolRepository`
- (`project.pool_repository`)
-- as an automatic consequence of the above, a `PoolRepository` has
- many `Project`s
-- a `PoolRepository` has exactly one "source `Project`"
- (`pool.source_project`)
+- a `Project` belongs to at most one `PoolRepository`
+ (`project.pool_repository`)
+- as an automatic consequence of the above, a `PoolRepository` has
+ many `Project`s
+- a `PoolRepository` has exactly one "source `Project`"
+ (`pool.source_project`)
> TODO Fix invalid SQL data for pools created prior to GitLab 11.11
> <https://gitlab.com/gitlab-org/gitaly/issues/1653>.
### Assumptions
-- All repositories in a pool must use [hashed
- storage](../administration/repository_storage_types.md). This is so
- that we don't have to ever worry about updating paths in
- `object/info/alternates` files.
-- All repositories in a pool must be on the same Gitaly storage shard.
- The Git alternates mechanism relies on direct disk access across
- multiple repositories, and we can only assume direct disk access to
- be possible within a Gitaly storage shard.
-- The only two ways to remove a member project from a pool are (1) to
- delete the project or (2) to move the project to another Gitaly
- storage shard.
+- All repositories in a pool must use [hashed
+ storage](../administration/repository_storage_types.md). This is so
+ that we don't have to ever worry about updating paths in
+ `object/info/alternates` files.
+- All repositories in a pool must be on the same Gitaly storage shard.
+ The Git alternates mechanism relies on direct disk access across
+ multiple repositories, and we can only assume direct disk access to
+ be possible within a Gitaly storage shard.
+- The only two ways to remove a member project from a pool are (1) to
+ delete the project or (2) to move the project to another Gitaly
+ storage shard.
### Creating pools and pool memberships
-- When a pool gets created, it must have a source project. The initial
- contents of the pool repository are a Git clone of the source
- project repository.
-- The occasion for creating a pool is when an existing eligible
- (public, hashed storage, non-forked) GitLab project gets forked and
- this project does not belong to a pool repository yet. The fork
- parent project becomes the source project of the new pool, and both
- the fork parent and the fork child project become members of the new
- pool.
-- Once project A has become the source project of a pool, all future
- eligible forks of A will become pool members.
-- If the fork source is itself a fork, the resulting repository will
- neither join the repository nor will a new pool repository be
- seeded.
-
- eg:
-
- Suppose fork A is part of a pool repository, any forks created off
- of fork A *will not* be a part of the pool repository that fork A is
- a part of.
-
- Suppose B is a fork of A, and A does not belong to an object pool.
- Now C gets created as a fork of B. C will not be part of a pool
- repository.
+- When a pool gets created, it must have a source project. The initial
+ contents of the pool repository are a Git clone of the source
+ project repository.
+- The occasion for creating a pool is when an existing eligible
+ (public, hashed storage, non-forked) GitLab project gets forked and
+ this project does not belong to a pool repository yet. The fork
+ parent project becomes the source project of the new pool, and both
+ the fork parent and the fork child project become members of the new
+ pool.
+- Once project A has become the source project of a pool, all future
+ eligible forks of A will become pool members.
+- If the fork source is itself a fork, the resulting repository will
+ neither join the repository nor will a new pool repository be
+ seeded.
+
+ eg:
+
+ Suppose fork A is part of a pool repository, any forks created off
+ of fork A *will not* be a part of the pool repository that fork A is
+ a part of.
+
+ Suppose B is a fork of A, and A does not belong to an object pool.
+ Now C gets created as a fork of B. C will not be part of a pool
+ repository.
> TODO should forks of forks be deduplicated?
> <https://gitlab.com/gitlab-org/gitaly/issues/1532>
### Consequences
-- If a normal Project participating in a pool gets moved to another
- Gitaly storage shard, its "belongs to PoolRepository" relation will
- be broken. Because of the way moving repositories between shard is
- implemented, we will automatically get a fresh self-contained copy
- of the project's repository on the new storage shard.
-- If the source project of a pool gets moved to another Gitaly storage
- shard or is deleted the "source project" relation is not broken.
- However, as of GitLab 12.0 a pool will not fetch from a source
- unless the source is on the same Gitaly shard.
+- If a normal Project participating in a pool gets moved to another
+ Gitaly storage shard, its "belongs to PoolRepository" relation will
+ be broken. Because of the way moving repositories between shard is
+ implemented, we will automatically get a fresh self-contained copy
+ of the project's repository on the new storage shard.
+- If the source project of a pool gets moved to another Gitaly storage
+ shard or is deleted the "source project" relation is not broken.
+ However, as of GitLab 12.0 a pool will not fetch from a source
+ unless the source is on the same Gitaly shard.
## Consistency between the SQL pool relation and Gitaly
diff --git a/doc/development/go_guide/index.md b/doc/development/go_guide/index.md
index f09339eb3a4..f827d240bf6 100644
--- a/doc/development/go_guide/index.md
+++ b/doc/development/go_guide/index.md
@@ -129,17 +129,50 @@ deploy a new pod, migrating the data automatically.
## Testing
+### Testing frameworks
+
We should not use any specific library or framework for testing, as the
[standard library](https://golang.org/pkg/) provides already everything to get
-started. For example, some external dependencies might be worth considering in
-case we decide to use a specific library or framework:
+started. If there is a need for more sophisticated testing tools, the following
+external dependencies might be worth considering in case we decide to use a specific
+library or framework:
- [Testify](https://github.com/stretchr/testify)
- [httpexpect](https://github.com/gavv/httpexpect)
+### Subtests
+
Use [subtests](https://blog.golang.org/subtests) whenever possible to improve
code readability and test output.
+### Better output in tests
+
+When comparing expected and actual values in tests, use
+[testify/require.Equal](https://godoc.org/github.com/stretchr/testify/require#Equal),
+[testify/require.EqualError](https://godoc.org/github.com/stretchr/testify/require#EqualError),
+[testify/require.EqualValues](https://godoc.org/github.com/stretchr/testify/require#EqualValues),
+and others to improve readability when comparing structs, errors,
+large portions of text, or JSON documents:
+
+```go
+type TestData struct {
+ // ...
+}
+
+func FuncUnderTest() TestData {
+ // ...
+}
+
+func Test(t *testing.T) {
+ t.Run("FuncUnderTest", func(t *testing.T) {
+ want := TestData{}
+ got := FuncUnderTest()
+
+ require.Equal(t, want, got) // note that expected value comes first, then comes the actual one ("diff" semantics)
+ })
+}
+```
+
### Benchmarks
Programs handling a lot of IO or complex operations should always include
diff --git a/doc/development/new_fe_guide/style/javascript.md b/doc/development/new_fe_guide/style/javascript.md
index 3019eaa089c..802ebd12d92 100644
--- a/doc/development/new_fe_guide/style/javascript.md
+++ b/doc/development/new_fe_guide/style/javascript.md
@@ -71,7 +71,6 @@ class myClass {
}
const instance = new myClass();
instance.makeRequest();
-
```
## Avoid classes to handle DOM events
@@ -189,8 +188,8 @@ disabled due to legacy compatibility reasons but they are in the process of bein
Do not disable specific ESLint rules. Due to technical debt, you may disable the following
rules only if you are invoking/instantiating existing code modules.
- - [no-new](http://eslint.org/docs/rules/no-new)
- - [class-method-use-this](http://eslint.org/docs/rules/class-methods-use-this)
+- [no-new](http://eslint.org/docs/rules/no-new)
+- [class-method-use-this](http://eslint.org/docs/rules/class-methods-use-this)
> Note: Disable these rules on a per line basis. This makes it easier to refactor
in the future. E.g. use `eslint-disable-next-line` or `eslint-disable-line`.
diff --git a/doc/development/testing_guide/end_to_end/page_objects.md b/doc/development/testing_guide/end_to_end/page_objects.md
index 05cb03eb4bd..29ad49403fe 100644
--- a/doc/development/testing_guide/end_to_end/page_objects.md
+++ b/doc/development/testing_guide/end_to_end/page_objects.md
@@ -92,20 +92,25 @@ end
The `view` DSL method will correspond to the rails View, partial, or vue component that renders the elements.
The `element` DSL method in turn declares an element for which a corresponding
-`qa-element-name-dasherized` CSS class will need to be added to the view file.
+`data-qa-selector=element_name_snaked` data attribute will need to be added to the view file.
You can also define a value (String or Regexp) to match to the actual view
code but **this is deprecated** in favor of the above method for two reasons:
- Consistency: there is only one way to define an element
-- Separation of concerns: QA uses dedicated CSS classes instead of reusing code
+- Separation of concerns: QA uses dedicated `data-qa-*` attributes instead of reusing code
or classes used by other components (e.g. `js-*` classes etc.)
```ruby
view 'app/views/my/view.html.haml' do
- # Implicitly require `.qa-logout-button` CSS class to be present in the view
+
+ ### Good ###
+
+ # Implicitly require the CSS selector `[data-qa-selector="logout_button"]` to be present in the view
element :logout_button
+ ### Bad ###
+
## This is deprecated and forbidden by the `QA/ElementWithPattern` RuboCop cop.
# Require `f.submit "Sign in"` to be present in `my/view.html.haml
element :my_button, 'f.submit "Sign in"' # rubocop:disable QA/ElementWithPattern
@@ -129,24 +134,39 @@ view 'app/views/my/view.html.haml' do
end
```
-To add these elements to the view, you must change the rails View, partial, or vue component by adding a `qa-element-descriptor` class
+To add these elements to the view, you must change the rails View, partial, or vue component by adding a `data-qa-selector` attribute
for each element defined.
-In our case, `qa-login-field`, `qa-password-field` and `qa-sign-in-button`
+In our case, `data-qa-selector="login_field"`, `data-qa-selector="password_field"` and `data-qa-selector="sign_in_button"`
**app/views/my/view.html.haml**
```haml
-= f.text_field :login, class: "form-control top qa-login-field", autofocus: "autofocus", autocapitalize: "off", autocorrect: "off", required: true, title: "This field is required."
-= f.password_field :password, class: "form-control bottom qa-password-field", required: true, title: "This field is required."
-= f.submit "Sign in", class: "btn btn-success qa-sign-in-button"
+= f.text_field :login, class: "form-control top", autofocus: "autofocus", autocapitalize: "off", autocorrect: "off", required: true, title: "This field is required.", data: { qa_selector: 'login_field' }
+= f.password_field :password, class: "form-control bottom", required: true, title: "This field is required.", data: { qa_selector: 'password_field' }
+= f.submit "Sign in", class: "btn btn-success", data: { qa_selector: 'sign_in_button' }
```
Things to note:
-- The CSS class must be `kebab-cased` (separated with hyphens "`-`")
+- The name of the element and the qa_selector must match and be snake_cased
- If the element appears on the page unconditionally, add `required: true` to the element. See
[Dynamic element validation](dynamic_element_validation.md)
+- You may see `.qa-selector` classes in existing Page Objects. We should prefer the [`data-qa-selector`](#data-qa-selector-vs-qa-selector)
+ method of definition over the `.qa-selector` CSS class
+
+
+### `data-qa-selector` vs `.qa-selector`
+
+> Introduced in GitLab 12.1
+
+There are two supported methods of defining elements within a view.
+
+1. `data-qa-selector` attribute
+1. `.qa-selector` class
+
+Any existing `.qa-selector` class should be considered deprecated
+and we should prefer the `data-qa-selector` method of definition.
## Running the test locally
diff --git a/doc/development/testing_guide/end_to_end/quick_start_guide.md b/doc/development/testing_guide/end_to_end/quick_start_guide.md
index efcfd44bc22..3bbf8feab39 100644
--- a/doc/development/testing_guide/end_to_end/quick_start_guide.md
+++ b/doc/development/testing_guide/end_to_end/quick_start_guide.md
@@ -101,7 +101,7 @@ it 'replaces an existing label if it has the same key' do
page.find('#content-body').click
page.refresh
- labels_block = page.find('.qa-labels-block')
+ labels_block = page.find(%q([data-qa-selector="labels_block"]))
expect(labels_block).to have_content('animal::dolphin')
expect(labels_block).not_to have_content('animal::fox')
@@ -130,7 +130,7 @@ it 'keeps both scoped labels when adding a label with a different key' do
page.find('#content-body').click
page.refresh
- labels_block = page.find('.qa-labels-block')
+ labels_block = page.find(%q([data-qa-selector="labels_block"]))
expect(labels_block).to have_content('animal::fox')
expect(labels_block).to have_content('plant::orchid')
@@ -139,7 +139,7 @@ it 'keeps both scoped labels when adding a label with a different key' do
end
```
-> Note that elements are always located using CSS selectors, and a good practice is to add test-specific selectors (this is called adding testability to the application and we will talk more about it later.) For example, the `labels_block` element uses the selector `.qa-labels-block`, which was added specifically for testing purposes.
+> Note that elements are always located using CSS selectors, and a good practice is to add test-specific selectors (this is called "testability"). For example, the `labels_block` element uses the CSS selector [`data-qa-selector="labels_block"`](page_objects.md#data-qa-selector-vs-qa-selector), which was added specifically for testing purposes.
Below are the steps that the test covers:
@@ -168,7 +168,7 @@ end
it 'replaces an existing label if it has the same key' do
select_label_and_refresh @new_label_same_scope
- labels_block = page.find('.qa-labels-block')
+ labels_block = page.find(%q([data-qa-selector="labels_block"]))
expect(labels_block).to have_content(@new_label_same_scope)
expect(labels_block).not_to have_content(@initial_label)
@@ -179,7 +179,7 @@ end
it 'keeps both scoped label when adding a label with a different key' do
select_label_and_refresh @new_label_different_scope
- labels_block = page.find('.qa-labels-block')
+ labels_block = page.find(%q([data-qa-selector="labels_block"]))
expect(labels_blocks).to have_content(@new_label_different_scope)
expect(labels_blocks).to have_content(@initial_label)
@@ -305,7 +305,7 @@ module QA
it 'correctly applies scoped labels depending on if they are from the same or a different scope' do
select_labels_and_refresh [@new_label_same_scope, @new_label_different_scope]
- labels_block = page.all('.qa-labels-block')
+ labels_block = page.all(%q([data-qa-selector="labels_block"]))
expect(labels_block).to have_content(@new_label_same_scope)
expect(labels_block).to have_content(@new_label_different_scope)
@@ -552,37 +552,36 @@ The `text_of_labels_block` method is a simple method that returns the `:labels_b
#### Updates in the view (*.html.haml) and `dropdowns_helper.rb` files
-Now let's change the view and the `dropdowns_helper` files to add the selectors that relate to the Page Object.
+Now let's change the view and the `dropdowns_helper` files to add the selectors that relate to the [Page Objects].
-In the [app/views/shared/issuable/_sidebar.html.haml](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/views/shared/issuable/_sidebar.html.haml) file, on [line 105 ](https://gitlab.com/gitlab-org/gitlab-ee/blob/84043fa72ca7f83ae9cde48ad670e6d5d16501a3/app/views/shared/issuable/_sidebar.html.haml#L105), add an extra class `qa-edit-link-labels`.
+In [`app/views/shared/issuable/_sidebar.html.haml:105`](https://gitlab.com/gitlab-org/gitlab-ee/blob/7ca12defc7a965987b162a6ebef302f95dc8867f/app/views/shared/issuable/_sidebar.html.haml#L105), add a `data: { qa_selector: 'edit_link_labels' }` data attribute.
The code should look like this:
```haml
-= link_to _('Edit'), '#', class: 'js-sidebar-dropdown-toggle edit-link float-right qa-edit-link-labels'
+= link_to _('Edit'), '#', class: 'js-sidebar-dropdown-toggle edit-link float-right', data: { qa_selector: 'edit_link_labels' }
```
-In the same file, on [line 121](https://gitlab.com/gitlab-org/gitlab-ee/blob/84043fa72ca7f83ae9cde48ad670e6d5d16501a3/app/views/shared/issuable/_sidebar.html.haml#L121), add an extra class `.qa-dropdown-menu-labels`.
+In the same file, on [line 121](https://gitlab.com/gitlab-org/gitlab-ee/blob/7ca12defc7a965987b162a6ebef302f95dc8867f/app/views/shared/issuable/_sidebar.html.haml#L121), add a `data: { qa_selector: 'dropdown_menu_labels' }` data attribute.
The code should look like this:
```haml
-.dropdown-menu.dropdown-select.dropdown-menu-paging.dropdown-menu-labels.dropdown-menu-selectable.qa-dropdown-menu-labels
+.dropdown-menu.dropdown-select.dropdown-menu-paging.dropdown-menu-labels.dropdown-menu-selectable.dropdown-extended-height{ data: { qa_selector: 'dropdown_menu_labels' } }
```
-In the [`dropdowns_helper.rb`](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/helpers/dropdowns_helper.rb) file, on [line 94](https://gitlab.com/gitlab-org/gitlab-ee/blob/99e51a374f2c20bee0989cac802e4b5621f72714/app/helpers/dropdowns_helper.rb#L94), add an extra class `qa-dropdown-input-field`.
+In [`app/helpers/dropdowns_helper.rb:94`](https://gitlab.com/gitlab-org/gitlab-ee/blob/7ca12defc7a965987b162a6ebef302f95dc8867f/app/helpers/dropdowns_helper.rb#L94), add a `data: { qa_selector: 'dropdown_input_field' }` data attribute.
The code should look like this:
```ruby
-filter_output = search_field_tag search_id, nil, class: "dropdown-input-field qa-dropdown-input-field", placeholder: placeholder, autocomplete: 'off'
+filter_output = search_field_tag search_id, nil, class: "dropdown-input-field", placeholder: placeholder, autocomplete: 'off', data: { qa_selector: 'dropdown_input_field' }
```
-> Classes starting with `qa-` are used for testing purposes only, and by defining such classes in the elements we add **testability** in the application.
+> `data-qa-*` data attributes and CSS classes starting with `qa-` are used solely for the purpose of QA and testing.
+> By defining these, we add **testability** to the application.
-> When defining a class like `qa-labels-block`, it is transformed into `:labels_block` for usage in the Page Objects. So, `qa-edit-link-labels` is transformed into `:edit_link_labels`, `qa-dropdown-menu-labels` is transformed into `:dropdown_menu_labels`, and `qa-dropdown-input-field` is transformed into `:dropdown_input_field`. Also, we use a [sanity test](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/qa/qa/page#how-did-we-solve-fragile-tests-problem) to check that defined elements have their respective `qa-` selectors in the specified views.
-
-> We did not define the `qa-labels-block` class in the `app/views/shared/issuable/_sidebar.html.haml` file because it was already there to be used.
+> When defining a data attribute like: `qa_selector: 'labels_block'`, it should match the element definition: `element :labels_block`. We use a [sanity test](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/qa/qa/page#how-did-we-solve-fragile-tests-problem) to check that defined elements have their respective selectors in the specified views.
#### Updates in the `QA::Page::Base` class
diff --git a/doc/development/testing_guide/frontend_testing.md b/doc/development/testing_guide/frontend_testing.md
index bb44cc595e9..c909745b1ab 100644
--- a/doc/development/testing_guide/frontend_testing.md
+++ b/doc/development/testing_guide/frontend_testing.md
@@ -79,6 +79,34 @@ describe('Component', () => {
Remember that the performance of each test depends on the environment.
+### Manual module mocks
+Jest supports [manual module mocks](https://jestjs.io/docs/en/manual-mocks) by placing a mock in a `__mocks__/` directory next to the source module. **Don't do this.** We want to keep all of our test-related code in one place (the `spec/` folder), and the logic that Jest uses to apply mocks from `__mocks__/` is rather inconsistent.
+
+Instead, our test runner detects manual mocks from `spec/frontend/mocks/`. Any mock placed here is automatically picked up and injected whenever you import its source module.
+
+- Files in `spec/frontend/mocks/ce` will mock the corresponding CE module from `app/assets/javascripts`, mirroring the source module's path.
+ - Example: `spec/frontend/mocks/ce/lib/utils/axios_utils` will mock the module `~/lib/utils/axios_utils`.
+- Files in `spec/frontend/mocks/node` will mock NPM packages of the same name or path.
+- We don't support mocking EE modules yet.
+
+If a mock is found for which a source module doesn't exist, the test suite will fail. 'Virtual' mocks, or mocks that don't have a 1-to-1 association with a source module, are not supported yet.
+
+#### Writing a mock
+Create a JS module in the appropriate place in `spec/frontend/mocks/`. That's it. It will automatically mock its source package in all tests.
+
+Make sure that your mock's export has the same format as the mocked module. So, if you're mocking a CommonJS module, you'll need to use `module.exports` instead of the ES6 `export`.
+
+It might be useful for a mock to expose a property that indicates if the mock was loaded. This way, tests can assert the presence of a mock without calling any logic and causing side-effects. The `~/lib/utils/axios_utils` module mock has such a property, `isMock`, that is `true` in the mock and undefined in the original class. Jest's mock functions also have a `mock` property that you can test.
+
+#### Bypassing mocks
+If you ever need to import the original module in your tests, use [`jest.requireActual()`](https://jestjs.io/docs/en/jest-object#jestrequireactualmodulename) (or `jest.requireActual().default` for the default export). The `jest.mock()` and `jest.unmock()` won't have an effect on modules that have a manual mock, because mocks are imported and cached before any tests are run.
+
+#### Keep mocks light
+Global mocks introduce magic and can affect how modules are imported in your tests. Try to keep them as light as possible and dependency-free. A global mock should be useful for any unit test. For example, the `axios_utils` and `jquery` module mocks throw an error when an HTTP request is attempted, since this is useful behaviour in &gt;99% of tests.
+
+When in doubt, construct mocks in your test file using [`jest.mock()`](https://jestjs.io/docs/en/jest-object#jestmockmodulename-factory-options), [`jest.spyOn()`](https://jestjs.io/docs/en/jest-object#jestspyonobject-methodname), etc.
+
+
## Karma test suite
GitLab uses the [Karma][karma] test runner with [Jasmine] as its test
diff --git a/doc/development/testing_guide/review_apps.md b/doc/development/testing_guide/review_apps.md
index ae40d628717..96761622cfe 100644
--- a/doc/development/testing_guide/review_apps.md
+++ b/doc/development/testing_guide/review_apps.md
@@ -137,8 +137,8 @@ secure note named **gitlab-{ce,ee} Review App's root password**.
### Run a Rails console
-1. [Filter Workloads by your Review App slug](https://console.cloud.google.com/kubernetes/workload?project=gitlab-review-apps)
- , e.g. `review-qa-raise-e-12chm0`.
+1. [Filter Workloads by your Review App slug](https://console.cloud.google.com/kubernetes/workload?project=gitlab-review-apps),
+ e.g. `review-qa-raise-e-12chm0`.
1. Find and open the `task-runner` Deployment, e.g. `review-qa-raise-e-12chm0-task-runner`.
1. Click on the Pod in the "Managed pods" section, e.g. `review-qa-raise-e-12chm0-task-runner-d5455cc8-2lsvz`.
1. Click on the `KUBECTL` dropdown, then `Exec` -> `task-runner`.
@@ -196,7 +196,7 @@ For the record, the debugging steps to find out this issue were:
1. `kubectl describe pod <pod name>` & confirm exact error message
1. Web search for exact error message, following rabbit hole to [a relevant kubernetes bug report](https://github.com/kubernetes/kubernetes/issues/57345)
1. Access the node over SSH via the GCP console (**Computer Engine > VM
- instances** then click the "SSH" button for the node where the `dns-gitlab-review-app-external-dns` pod runs)
+ instances** then click the "SSH" button for the node where the `dns-gitlab-review-app-external-dns` pod runs)
1. In the node: `systemctl --version` => systemd 232
1. Gather some more information:
- `mount | grep kube | wc -l` => e.g. 290
@@ -211,7 +211,7 @@ For the record, the debugging steps to find out this issue were:
To resolve the problem, we needed to (forcibly) drain some nodes:
1. Try a normal drain on the node where the `dns-gitlab-review-app-external-dns`
- pod runs so that Kubernetes automatically move it to another node: `kubectl drain NODE_NAME`
+ pod runs so that Kubernetes automatically move it to another node: `kubectl drain NODE_NAME`
1. If that doesn't work, you can also perform a forcible "drain" the node by removing all pods: `kubectl delete pods --field-selector=spec.nodeName=NODE_NAME`
1. In the node:
- Perform `systemctl daemon-reload` to remove the dead/inactive units
View Lorry Controller Status