diff options
Diffstat (limited to 'doc/development')
56 files changed, 2680 insertions, 1302 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index dd150421b65..0cafc112b6b 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -1,69 +1,96 @@ -# Development +--- +comments: false +--- -## Outside of docs +# GitLab development guides -- [CONTRIBUTING.md](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md) main contributing guide -- [PROCESS.md](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md) contributing process -- [GitLab Development Kit (GDK)](https://gitlab.com/gitlab-org/gitlab-development-kit/blob/master/doc/howto/README.md) to install a development version +## Get started! -## Styleguides - -- [API styleguide](api_styleguide.md) Use this styleguide if you are - contributing to the API. -- [Documentation styleguide](doc_styleguide.md) Use this styleguide if you are - contributing to documentation. -- [Writing documentation](writing_documentation.md) - - [Distinction between general documentation and technical articles](writing_documentation.md#distinction-between-general-documentation-and-technical-articles) -- [SQL Migration Style Guide](migration_style_guide.md) for creating safe SQL migrations -- [Testing standards and style guidelines](testing.md) -- [UX guide](ux_guide/index.md) for building GitLab with existing CSS styles and elements -- [Frontend guidelines](fe_guide/index.md) -- [SQL guidelines](sql.md) for working with SQL queries -- [Sidekiq guidelines](sidekiq_style_guide.md) for working with Sidekiq workers -- [`Gemfile` guidelines](gemfile.md) +- Setup GitLab's development environment with [GitLab Development Kit (GDK)](https://gitlab.com/gitlab-org/gitlab-development-kit/blob/master/doc/howto/README.md) +- [GitLab contributing guide](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md) +- [Architecture](architecture.md) of GitLab +- [Rake tasks](rake_tasks.md) for development -## Process +## Processes +- [GitLab core team & GitLab Inc. contribution process](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md) - [Generate a changelog entry with `bin/changelog`](changelog.md) -- [Limit conflicts with EE when developing on CE](limit_ee_conflicts.md) - [Code review guidelines](code_review.md) for reviewing code and having code reviewed. -- [Merge request performance guidelines](merge_request_performance_guidelines.md) - for ensuring merge requests do not negatively impact GitLab performance +- [Limit conflicts with EE when developing on CE](limit_ee_conflicts.md) -## Backend howtos +## UX and frontend guides -- [Architecture](architecture.md) of GitLab +- [UX guide](ux_guide/index.md) for building GitLab with existing CSS styles and elements +- [Frontend guidelines](fe_guide/index.md) + +## Backend guides + +- [API styleguide](api_styleguide.md) Use this styleguide if you are + contributing to the API. +- [Sidekiq guidelines](sidekiq_style_guide.md) for working with Sidekiq workers +- [Working with Gitaly](gitaly.md) +- [Manage feature flags](feature_flags.md) +- [View sent emails or preview mailers](emails.md) +- [Shell commands](shell_commands.md) in the GitLab codebase +- [`Gemfile` guidelines](gemfile.md) +- [Sidekiq debugging](sidekiq_debugging.md) - [Gotchas](gotchas.md) to avoid +- [Issue and merge requests state models](object_state_models.md) - [How to dump production data to staging](db_dump.md) + +## Performance guides + - [Instrumentation](instrumentation.md) - [Performance guidelines](performance.md) -- [Rake tasks](rake_tasks.md) for development -- [Shell commands](shell_commands.md) in the GitLab codebase -- [Sidekiq debugging](sidekiq_debugging.md) -- [Object state models](object_state_models.md) -- [Building a package for testing purposes](build_test_package.md) -- [Manage feature flags](feature_flags.md) +- [Merge request performance guidelines](merge_request_performance_guidelines.md) + for ensuring merge requests do not negatively impact GitLab performance + +## Databases guides -## Databases +### Migrations -- [Merge Request Checklist](database_merge_request_checklist.md) - [What requires downtime?](what_requires_downtime.md) +- [SQL guidelines](sql.md) for working with SQL queries +- [Migrations style guide](migration_style_guide.md) for creating safe SQL migrations +- [Post deployment migrations](post_deployment_migrations.md) +- [Background migrations](background_migrations.md) +- [Swapping tables](swapping_tables.md) + +### Best practices + +- [Merge Request checklist](database_merge_request_checklist.md) - [Adding database indexes](adding_database_indexes.md) -- [Post Deployment Migrations](post_deployment_migrations.md) -- [Foreign Keys & Associations](foreign_keys.md) -- [Serializing Data](serializing_data.md) -- [Polymorphic Associations](polymorphic_associations.md) -- [Single Table Inheritance](single_table_inheritance.md) -- [Background Migrations](background_migrations.md) -- [Storing SHA1 Hashes As Binary](sha1_as_binary.md) -- [Iterating Tables In Batches](iterating_tables_in_batches.md) -- [Ordering Table Columns](ordering_table_columns.md) -- [Verifying Database Capabilities](verifying_database_capabilities.md) -- [Hash Indexes](hash_indexes.md) - -## i18n - -- [Internationalization for GitLab](i18n_guide.md) +- [Foreign keys & associations](foreign_keys.md) +- [Single table inheritance](single_table_inheritance.md) +- [Polymorphic associations](polymorphic_associations.md) +- [Serializing data](serializing_data.md) +- [Hash indexes](hash_indexes.md) +- [Storing SHA1 hashes as binary](sha1_as_binary.md) +- [Iterating tables in batches](iterating_tables_in_batches.md) +- [Ordering table columns](ordering_table_columns.md) +- [Verifying database capabilities](verifying_database_capabilities.md) + +## Testing guides + +- [Testing standards and style guidelines](testing_guide/index.md) +- [Frontend testing standards and style guidelines](testing_guide/frontend_testing.md) + +## Documentation guides + +- [Documentation styleguide](doc_styleguide.md): Use this styleguide if you are + contributing to the documentation. +- [Writing documentation](writing_documentation.md) + - [Distinction between general documentation and technical articles](writing_documentation.md#distinction-between-general-documentation-and-technical-articles) + +## Internationalization (i18n) guides + +- [Introduction](i18n/index.md) +- [Externalization](i18n/externalization.md) +- [Translation](i18n/translation.md) + +## Build guides + +- [Building a package for testing purposes](build_test_package.md) ## Compliance diff --git a/doc/development/background_migrations.md b/doc/development/background_migrations.md index f83a60e49e8..5452b0e7a2f 100644 --- a/doc/development/background_migrations.md +++ b/doc/development/background_migrations.md @@ -215,14 +215,29 @@ same time will ensure that both existing and new data is migrated. In the next release we can remove the `after_commit` hooks and related code. We will also need to add a post-deployment migration that consumes any remaining -jobs. Such a migration would look like this: +jobs and manually run on any un-migrated rows. Such a migration would look like +this: ```ruby class ConsumeRemainingExtractServicesUrlJobs < ActiveRecord::Migration disable_ddl_transaction! + class Service < ActiveRecord::Base + include ::EachBatch + + self.table_name = 'services' + end + def up + # This must be included Gitlab::BackgroundMigration.steal('ExtractServicesUrl') + + # This should be included, but can be skipped - see below + Service.where(url: nil).each_batch(of: 50) do |batch| + range = batch.pluck('MIN(id)', 'MAX(id)').first + + Gitlab::BackgroundMigration::ExtractServicesUrl.new.perform(*range) + end end def down @@ -230,6 +245,15 @@ class ConsumeRemainingExtractServicesUrlJobs < ActiveRecord::Migration end ``` +The final step runs for any un-migrated rows after all of the jobs have been +processed. This is in case a Sidekiq process running the background migrations +received SIGKILL, leading to the jobs being lost. (See +[more reliable Sidekiq queue][reliable-sidekiq] for more information.) + +If the application does not depend on the data being 100% migrated (for +instance, the data is advisory, and not mission-critical), then this final step +can be skipped. + This migration will then process any jobs for the ExtractServicesUrl migration and continue once all jobs have been processed. Once done you can safely remove the `services.properties` column. @@ -254,6 +278,9 @@ for more details. 1. Make sure that background migration jobs are idempotent. 1. Make sure that tests you write are not false positives. +1. Make sure that if the data being migrated is critical and cannot be lost, the + clean-up migration also checks the final state of the data before completing. [migrations-readme]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/migrations/README.md [issue-rspec-hooks]: https://gitlab.com/gitlab-org/gitlab-ce/issues/35351 +[reliable-sidekiq]: https://gitlab.com/gitlab-org/gitlab-ce/issues/36791 diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 64a89976300..7165b8062a7 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -9,8 +9,18 @@ There are a few rules to get your merge request accepted: **approved by a [backend maintainer][projects]**. 1. If your merge request includes only frontend changes [^1], it must be **approved by a [frontend maintainer][projects]**. + 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 frontend and backend changes [^1], it must be **approved by a [frontend and a backend maintainer][projects]**. + 1. If your merge request includes UX and frontend changes [^1], it must + be **approved by a [UX team member and a frontend maintainer][team]**. + 1. If your merge request includes UX, frontend and backend changes [^1], it must + be **approved by a [UX team member, a frontend and a backend maintainer][team]**. 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 diff --git a/doc/development/doc_styleguide.md b/doc/development/doc_styleguide.md index 798f40eef3d..0e4ffbd7910 100644 --- a/doc/development/doc_styleguide.md +++ b/doc/development/doc_styleguide.md @@ -459,11 +459,11 @@ Rendered example: ### cURL commands - Use `https://gitlab.example.com/api/v4/` as an endpoint. -- Wherever needed use this private token: `9koXpg98eAheJpvBs5tK`. +- Wherever needed use this personal access token: `9koXpg98eAheJpvBs5tK`. - Always put the request first. `GET` is the default so you don't have to include it. - Use double quotes to the URL when it includes additional parameters. -- Prefer to use examples using the private token and don't pass data of +- Prefer to use examples using the personal access token and don't pass data of username and password. | Methods | Description | diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md new file mode 100644 index 00000000000..932a44f65e4 --- /dev/null +++ b/doc/development/ee_features.md @@ -0,0 +1,382 @@ +# Guidelines for implementing Enterprise Edition feature + +- **Write the code and the tests.**: As with any code, EE features should have + good test coverage to prevent regressions. +- **Write documentation.**: Add documentation to the `doc/` directory. Describe + the feature and include screenshots, if applicable. +- **Submit a MR to the `www-gitlab-com` project.**: Add the new feature to the + [EE features list][ee-features-list]. + +## Act as CE when unlicensed + +Since the implementation of [GitLab CE features to work with unlicensed EE instance][ee-as-ce] +GitLab Enterprise Edition should work like GitLab Community Edition +when no license is active. So EE features always should be guarded by +`project.feature_available?` or `group.feature_available?` (or +`License.feature_available?` if it is a system-wide feature). + +CE specs should remain untouched as much as possible and extra specs +should be added for EE. Licensed features can be stubbed using the +spec helper `stub_licensed_features` in `EE::LicenseHelpers`. + +[ee-as-ce]: https://gitlab.com/gitlab-org/gitlab-ee/issues/2500 + +## Separation of EE code + +We want a [single code base][] eventually, but before we reach the goal, +we still need to merge changes from GitLab CE to EE. To help us get there, +we should make sure that we no longer edit CE files in place in order to +implement EE features. + +Instead, all EE codes should be put inside the `ee/` top-level directory, and +tests should be put inside `spec/ee/`. We don't use `ee/spec` for now due to +technical limitation. The rest of codes should be as close as to the CE files. + +[single code base]: https://gitlab.com/gitlab-org/gitlab-ee/issues/2952#note_41016454 + +### EE-only features + +If the feature being developed is not present in any form in CE, we don't +need to put the codes under `EE` namespace. For example, an EE model could +go into: `ee/app/models/awesome.rb` using `Awesome` as the class name. This +is applied not only to models. Here's a list of other examples: + +- `ee/app/controllers/foos_controller.rb` +- `ee/app/finders/foos_finder.rb` +- `ee/app/helpers/foos_helper.rb` +- `ee/app/mailers/foos_mailer.rb` +- `ee/app/models/foo.rb` +- `ee/app/policies/foo_policy.rb` +- `ee/app/serializers/foo_entity.rb` +- `ee/app/serializers/foo_serializer.rb` +- `ee/app/services/foo/create_service.rb` +- `ee/app/validators/foo_attr_validator.rb` +- `ee/app/workers/foo_worker.rb` + +### EE features based on CE features + +For features that build on existing CE features, write a module in the +`EE` namespace and `prepend` it in the CE class. This makes conflicts +less likely to happen during CE to EE merges because only one line is +added to the CE class - the `prepend` line. + +Since the module would require an `EE` namespace, the file should also be +put in an `ee/` sub-directory. For example, we want to extend the user model +in EE, so we have a module called `::EE::User` put inside +`ee/app/models/ee/user.rb`. + +This is also not just applied to models. Here's a list of other examples: + +- `ee/app/controllers/ee/foos_controller.rb` +- `ee/app/finders/ee/foos_finder.rb` +- `ee/app/helpers/ee/foos_helper.rb` +- `ee/app/mailers/ee/foos_mailer.rb` +- `ee/app/models/ee/foo.rb` +- `ee/app/policies/ee/foo_policy.rb` +- `ee/app/serializers/ee/foo_entity.rb` +- `ee/app/serializers/ee/foo_serializer.rb` +- `ee/app/services/ee/foo/create_service.rb` +- `ee/app/validators/ee/foo_attr_validator.rb` +- `ee/app/workers/ee/foo_worker.rb` + +#### Overriding CE methods + +To override a method present in the CE codebase, use `prepend`. It +lets you override a method in a class with a method from a module, while +still having access the class's implementation with `super`. + +There are a few gotchas with it: + +- you should always add a `raise NotImplementedError unless defined?(super)` + guard clause in the "overrider" method to ensure that if the method gets + renamed in CE, the EE override won't be silently forgotten. +- when the "overrider" would add a line in the middle of the CE + implementation, you should refactor the CE method and split it in + smaller methods. Or create a "hook" method that is empty in CE, + and with the EE-specific implementation in EE. +- when the original implementation contains a guard clause (e.g. + `return unless condition`), we cannot easily extend the behaviour by + overriding the method, because we can't know when the overridden method + (i.e. calling `super` in the overriding method) would want to stop early. + In this case, we shouldn't just override it, but update the original method + to make it call the other method we want to extend, like a [template method + pattern](https://en.wikipedia.org/wiki/Template_method_pattern). + For example, given this base: + ``` ruby + class Base + def execute + return unless enabled? + + # ... + # ... + end + end + ``` + Instead of just overriding `Base#execute`, we should update it and extract + the behaviour into another method: + ``` ruby + class Base + def execute + return unless enabled? + + do_something + end + + private + + def do_something + # ... + # ... + end + end + ``` + Then we're free to override that `do_something` without worrying about the + guards: + ``` ruby + module EE::Base + def do_something + # Follow the above pattern to call super and extend it + end + end + ``` + This would require updating CE first, or make sure this is back ported to CE. + +When prepending, place them in the `ee/` specific sub-directory, and +wrap class or module in `module EE` to avoid naming conflicts. + +For example to override the CE implementation of +`ApplicationController#after_sign_out_path_for`: + +```ruby +def after_sign_out_path_for(resource) + current_application_settings.after_sign_out_path.presence || new_user_session_path +end +``` + +Instead of modifying the method in place, you should add `prepend` to +the existing file: + +```ruby +class ApplicationController < ActionController::Base + prepend EE::ApplicationController + # ... + + def after_sign_out_path_for(resource) + current_application_settings.after_sign_out_path.presence || new_user_session_path + end + + # ... +end +``` + +And create a new file in the `ee/` sub-directory with the altered +implementation: + +```ruby +module EE + class ApplicationController + def after_sign_out_path_for(resource) + raise NotImplementedError unless defined?(super) + + if Gitlab::Geo.secondary? + Gitlab::Geo.primary_node.oauth_logout_url(@geo_logout_state) + else + super + end + end + end +end +``` + +#### Use self-descriptive wrapper methods + +When it's not possible/logical to modify the implementation of a +method. Wrap it in a self-descriptive method and use that method. + +For example, in CE only an `admin` is allowed to access all private +projects/groups, but in EE also an `auditor` has full private +access. It would be incorrect to override the implementation of +`User#admin?`, so instead add a method `full_private_access?` to +`app/models/users.rb`. The implementation in CE will be: + +```ruby +def full_private_access? + admin? +end +``` + +In EE, the implementation `ee/app/models/ee/users.rb` would be: + +```ruby +def full_private_access? + raise NotImplementedError unless defined?(super) + super || auditor? +end +``` + +In `lib/gitlab/visibility_level.rb` this method is used to return the +allowed visibilty levels: + +```ruby +def levels_for_user(user = nil) + if user.full_private_access? + [PRIVATE, INTERNAL, PUBLIC] + elsif # ... +end +``` + +See [CE MR][ce-mr-full-private] and [EE MR][ee-mr-full-private] for +full implementation details. + +[ce-mr-full-private]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12373 +[ee-mr-full-private]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2199 + +### Code in `app/controllers/` + +In controllers, the most common type of conflict is with `before_action` that +has a list of actions in CE but EE adds some actions to that list. + +The same problem often occurs for `params.require` / `params.permit` calls. + +**Mitigations** + +Separate CE and EE actions/keywords. For instance for `params.require` in +`ProjectsController`: + +```ruby +def project_params + params.require(:project).permit(project_params_attributes) +end + +# Always returns an array of symbols, created however best fits the use case. +# It _should_ be sorted alphabetically. +def project_params_attributes + %i[ + description + name + path + ] +end + +``` + +In the `EE::ProjectsController` module: + +```ruby +def project_params_attributes + super + project_params_attributes_ee +end + +def project_params_attributes_ee + %i[ + approvals_before_merge + approver_group_ids + approver_ids + ... + ] +end +``` + +### Code in `app/models/` + +EE-specific models should `extend EE::Model`. + +For example, if EE has a specific `Tanuki` model, you would +place it in `ee/app/models/ee/tanuki.rb`. + +### Code in `app/views/` + +It's a very frequent problem that EE is adding some specific view code in a CE +view. For instance the approval code in the project's settings page. + +**Mitigations** + +Blocks of code that are EE-specific should be moved to partials. This +avoids conflicts with big chunks of HAML code that that are not fun to +resolve when you add the indentation to the equation. + +EE-specific views should be placed in `ee/app/views/ee/`, using extra +sub-directories if appropriate. + +### Code in `lib/` + +Place EE-specific logic in the top-level `EE` module namespace. Namespace the +class beneath the `EE` module just as you would normally. + +For example, if CE has LDAP classes in `lib/gitlab/ldap/` then you would place +EE-specific LDAP classes in `ee/lib/ee/gitlab/ldap`. + +### Code in `spec/` + +When you're testing EE-only features, avoid adding examples to the +existing CE specs. Also do no change existing CE examples, since they +should remain working as-is when EE is running without a license. + +Instead place EE specs in the `spec/ee/spec` folder. + +## JavaScript code in `assets/javascripts/` + +To separate EE-specific JS-files we can also move the files into an `ee` folder. + +For example there can be an +`app/assets/javascripts/protected_branches/protected_branches_bundle.js` and an +EE counterpart +`ee/app/assets/javascripts/protected_branches/protected_branches_bundle.js`. + +That way we can create a separate webpack bundle in `webpack.config.js`: + +```javascript + protected_branches: '~/protected_branches', + ee_protected_branches: 'ee/protected_branches/protected_branches_bundle.js', +``` + +With the separate bundle in place, we can decide which bundle to load inside the +view, using the `page_specific_javascript_bundle_tag` helper. + +```haml +- content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('protected_branches') +``` + +## SCSS code in `assets/stylesheets` + +To separate EE-specific styles in SCSS files, if a component you're adding styles for +is limited to only EE, it is better to have a separate SCSS file in appropriate directory +within `app/assets/stylesheets`. + +In some cases, this is not entirely possible or creating dedicated SCSS file is an overkill, +e.g. a text style of some component is different for EE. In such cases, +styles are usually kept in stylesheet that is common for both CE and EE, and it is wise +to isolate such ruleset from rest of CE rules (along with adding comment describing the same) +to avoid conflicts during CE to EE merge. + +#### Bad +```scss +.section-body { + .section-title { + background: $gl-header-color; + } + + &.ee-section-body { + .section-title { + background: $gl-header-color-cyan; + } + } +} +``` + +#### Good +```scss +.section-body { + .section-title { + background: $gl-header-color; + } +} + +/* EE-specific styles */ +.section-body.ee-section-body { + .section-title { + background: $gl-header-color-cyan; + } +} +``` diff --git a/doc/development/emails.md b/doc/development/emails.md new file mode 100644 index 00000000000..18f47f44cb5 --- /dev/null +++ b/doc/development/emails.md @@ -0,0 +1,23 @@ +# Dealing with email in development + +## Sent emails + +To view rendered emails "sent" in your development instance, visit +[`/rails/letter_opener`](http://localhost:3000/rails/letter_opener). + +## Mailer previews + +Rails provides a way to preview our mailer templates in HTML and plaintext using +dummy data. + +The previews live in [`spec/mailers/previews`][previews] and can be viewed at +[`/rails/mailers`](http://localhost:3000/rails/mailers). + +See the [Rails guides] for more info. + +[previews]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/spec/mailers/previews +[Rails guides]: http://guides.rubyonrails.org/action_mailer_basics.html#previewing-emails + +--- + +[Return to Development documentation](README.md) diff --git a/doc/development/fe_guide/icons.md b/doc/development/fe_guide/icons.md new file mode 100644 index 00000000000..a76e978bd26 --- /dev/null +++ b/doc/development/fe_guide/icons.md @@ -0,0 +1,40 @@ +# Icons + +We are using SVG Icons in GitLab with a SVG Sprite, due to this the icons are only loaded once and then referenced through an ID. The sprite SVG is located under `/assets/icons.svg`. Our goal is to replace one by one all inline SVG Icons (as those currently bloat the HTML) and also all Font Awesome usages. + +### Usage in HAML/Rails + +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 under `/assets/sprite.symbol.html`). +**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** + +`= sprite_icon('issues', size: 72, css_class: 'icon-danger')` + +**Output from example above** + +`<svg class="s72 icon-danger"><use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="/assets/icons.svg#issues"></use></svg>` + +### Usage in HTML/JS + +Please use the following function inside JS to render an icon : +`gl.utils.spriteIcon(iconName)` + +## Adding a new icon to the sprite + +All Icons and Illustrations are managed in the [gitlab-svgs](https://gitlab.com/gitlab-org/gitlab-svgs) repository which is added as a dev-dependency. + +To upgrade to a new SVG Sprite version run `yarn upgrade https://gitlab.com/gitlab-org/gitlab-svgs` and then run `yarn run svg`. This task will copy the svg sprite and all illustrations in the correct folders. + +# SVG Illustrations + +Please use from now on for any SVG based illustrations simple `img` tags to show an illustration by simply using either `image_tag` or `image_path` helpers. Please use the class `svg-content` around it to ensure nice rendering. The illustrations are also organised in the [gitlab-svgs](https://gitlab.com/gitlab-org/gitlab-svgs) repository (as they are then automatically optimised). + +**Example** + +`= image_tag 'illustrations/merge_requests.svg'` diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index 64bcb4a0257..8f956681693 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -29,34 +29,6 @@ For our currently-supported browsers, see our [requirements][requirements]. ## Development Process -When you are assigned an issue please follow the next steps: - -### Divide a big feature into small Merge Requests -1. Big Merge Request are painful to review. In order to make this process easier we -must break a big feature into smaller ones and create a Merge Request for each step. -1. First step is to create a branch from `master`, let's call it `new-feature`. This branch -will be the recipient of all the smaller Merge Requests. Only this one will be merged to master. -1. Don't do any work on this one, let's keep it synced with master. -1. Create a new branch from `new-feature`, let's call it `new-feature-step-1`. We advise you -to clearly identify which step the branch represents. -1. Do the first part of the modifications in this branch. The target branch of this Merge Request -should be `new-feature`. -1. Once `new-feature-step-1` gets merged into `new-feature` we can continue our work. Create a new -branch from `new-feature`, let's call it `new-feature-step-2` and repeat the process done before. - -```shell -master -└─ new-feature - ├─ new-feature-step-1 - ├─ new-feature-step-2 - └─ new-feature-step-3 -``` - -**Tips** -- Make sure `new-feature` branch is always synced with `master`: merge master frequently. -- Do the same for the feature branch you have opened. This can be accomplished by merging `master` into `new-feature` and `new-feature` into `new-feature-step-*` -- Avoid rewriting history. - ### Share your work early 1. Before writing code guarantee your vision of the architecture is aligned with GitLab's architecture. @@ -82,7 +54,8 @@ or make changes to our frontend development guidelines. --- -## [Testing](testing.md) +## [Testing](../testing_guide/frontend_testing.md) + How we write frontend tests, run the GitLab test suite, and debug test related issues. @@ -98,6 +71,14 @@ Vue specific design patterns and practices. --- +## [Vue Resource](vue_resource.md) +Vue resource specific practices and gotchas. + +## [Icons](icons.md) +How we use SVG for our Icons. + +--- + ## Style Guides ### [JavaScript Style Guide](style_guide_js.md) @@ -125,6 +106,10 @@ Frontend security practices. ## [Accessibility](accessibility.md) Our accessibility standards and resources. +## [Internationalization (i18n) and Translations](../i18n/externalization.md) +Frontend internationalization support is described in [this document](../i18n/). +The [externalization part of the guide](../i18n/externalization.md) explains the helpers/methods available. + [rails]: http://rubyonrails.org/ [haml]: http://haml.info/ diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 4f20aa070de..10f4c5a0902 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -88,16 +88,31 @@ followed by any global declarations, then a blank newline prior to any imports o 1. Use ES module syntax to import modules ```javascript // bad - require('foo'); + const SomeClass = require('some_class'); // good - import Foo from 'foo'; + import SomeClass from 'some_class'; // bad - module.exports = Foo; + module.exports = SomeClass; // good - export default Foo; + export default SomeClass; + ``` + + Import statements are following usual naming guidelines, for example object literals use camel case: + + ```javascript + // some_object file + export default { + key: 'value', + }; + + // bad + import ObjectLiteral from 'some_object'; + + // good + import objectLiteral from 'some_object'; ``` 1. Relative paths: when importing a module in the same directory, a child @@ -285,6 +300,13 @@ A forEach will cause side effects, it will be mutating the array being iterated. 1. **Extensions**: Use `.vue` extension for Vue components. 1. **Reference Naming**: Use camelCase for their instances: ```javascript + // bad + import CardBoard from 'cardBoard' + + components: { + CardBoard: + }; + // good import cardBoard from 'cardBoard' @@ -311,6 +333,7 @@ A forEach will cause side effects, it will be mutating the array being iterated. #### Alignment 1. Follow these alignment styles for the template method: + 1. With more than one attribute, all attributes should be on a new line: ```javascript // bad <component v-if="bar" @@ -327,9 +350,16 @@ A forEach will cause side effects, it will be mutating the array being iterated. <button class="btn"> Click me </button> + ``` + 1. The tag can be inline if there is only one attribute: + ```javascript + // good + <component bar="bar" /> - // if props fit in one line then keep it on the same line - <component bar="bar" /> + // good + <component + bar="bar" + /> ``` #### Quotes @@ -381,9 +411,12 @@ A forEach will cause side effects, it will be mutating the array being iterated. } ``` -1. Default key should always be provided if the prop is not required: +1. Default key should be provided if the prop is not required. +_Note:_ There are some scenarios where we need to check for the existence of the property. +On those a default key should not be provided. + ```javascript - // bad + // good props: { foo: { type: String, @@ -459,7 +492,25 @@ A forEach will cause side effects, it will be mutating the array being iterated. ``` #### Ordering -1. Order for a Vue Component: + +1. Tag order in `.vue` file + + ``` + <script> + // ... + </script> + + <template> + // ... + </template> + + // We don't use scoped styles but there are few instances of this + <style> + // ... + </style> + ``` + +1. Properties in a Vue Component: 1. `name` 1. `props` 1. `mixins` @@ -479,6 +530,7 @@ A forEach will cause side effects, it will be mutating the array being iterated. 1. `beforeDestroy` 1. `destroyed` + #### Vue and Bootstrap 1. Tooltips: Do not rely on `has-tooltip` class name for Vue components @@ -512,11 +564,11 @@ A forEach will cause side effects, it will be mutating the array being iterated. ``` ### The Javascript/Vue Accord -The goal of this accord is to make sure we are all on the same page. +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. When writing Vue, you may not use jQuery in your application. 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. You may use a jQuery dependency in Vue.js following [this example from the docs](https://vuejs.org/v2/examples/select2.html). + 1. You may use a jQuery dependency in Vue.js following [this example from the docs](https://vuejs.org/v2/examples/select2.html). 1. If an outside jQuery Event needs to be listen to inside the Vue application, you may use jQuery event listeners. 1. 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. diff --git a/doc/development/fe_guide/testing.md b/doc/development/fe_guide/testing.md index 867c83f1e72..98e499b8c0f 100644 --- a/doc/development/fe_guide/testing.md +++ b/doc/development/fe_guide/testing.md @@ -1,254 +1 @@ -# Frontend Testing - -There are two types of test suites you'll encounter while developing frontend code -at GitLab. We use Karma and Jasmine for JavaScript unit and integration testing, and RSpec -feature tests with Capybara for e2e (end-to-end) integration testing. - -Unit and feature tests need to be written for all new features. -Most of the time, you should use rspec for your feature tests. -There are cases where the behaviour you are testing is not worth the time spent running the full application, -for example, if you are testing styling, animation, edge cases or small actions that don't involve the backend, -you should write an integration test using Jasmine. - - - -_This diagram demonstrates the relative priority of each test type we use_ - -Regression tests should be written for bug fixes to prevent them from recurring in the future. - -See [the Testing Standards and Style Guidelines](../testing.md) -for more information on general testing practices at GitLab. - -## Karma test suite - -GitLab uses the [Karma][karma] test runner with [Jasmine][jasmine] as its test -framework for our JavaScript unit and integration tests. For integration tests, -we generate HTML files using RSpec (see `spec/javascripts/fixtures/*.rb` for examples). -Some fixtures are still HAML templates that are translated to HTML files using the same mechanism (see `static_fixtures.rb`). -Adding these static fixtures should be avoided as they are harder to keep up to date with real views. -The existing static fixtures will be migrated over time. -Please see [gitlab-org/gitlab-ce#24753](https://gitlab.com/gitlab-org/gitlab-ce/issues/24753) to track our progress. -Fixtures are served during testing by the [jasmine-jquery][jasmine-jquery] plugin. - -JavaScript tests live in `spec/javascripts/`, matching the folder structure -of `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js` -has a corresponding `spec/javascripts/behaviors/autosize_spec.js` file. - -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. - -### Best practice - -#### Naming unit tests - -When writing describe test blocks to test specific functions/methods, -please use the method name as the describe block name. - -```javascript -// Good -describe('methodName', () => { - it('passes', () => { - expect(true).toEqual(true); - }); -}); - -// Bad -describe('#methodName', () => { - it('passes', () => { - expect(true).toEqual(true); - }); -}); - -// Bad -describe('.methodName', () => { - it('passes', () => { - expect(true).toEqual(true); - }); -}); -``` -#### Testing Promises - -When testing Promises you should always make sure that the test is asynchronous and rejections are handled. -Your Promise chain should therefore end with a call of the `done` callback and `done.fail` in case an error occurred. - -```javascript -// Good -it('tests a promise', (done) => { - promise - .then((data) => { - expect(data).toBe(asExpected); - }) - .then(done) - .catch(done.fail); -}); - -// Good -it('tests a promise rejection', (done) => { - promise - .then(done.fail) - .catch((error) => { - expect(error).toBe(expectedError); - }) - .then(done) - .catch(done.fail); -}); - -// Bad (missing done callback) -it('tests a promise', () => { - promise - .then((data) => { - expect(data).toBe(asExpected); - }) -}); - -// Bad (missing catch) -it('tests a promise', (done) => { - promise - .then((data) => { - expect(data).toBe(asExpected); - }) - .then(done) -}); - -// Bad (use done.fail in asynchronous tests) -it('tests a promise', (done) => { - promise - .then((data) => { - expect(data).toBe(asExpected); - }) - .then(done) - .catch(fail) -}); - -// Bad (missing catch) -it('tests a promise rejection', (done) => { - promise - .catch((error) => { - expect(error).toBe(expectedError); - }) - .then(done) -}); -``` - -#### Stubbing - -For unit tests, you should stub methods that are unrelated to the current unit you are testing. -If you need to use a prototype method, instantiate an instance of the class and call it there instead of mocking the instance completely. - -For integration tests, you should stub methods that will effect the stability of the test if they -execute their original behaviour. i.e. Network requests. - -### Vue.js unit tests -See this [section][vue-test]. - -### Running frontend tests - -`rake karma` runs the frontend-only (JavaScript) tests. -It consists of two subtasks: - -- `rake karma:fixtures` (re-)generates fixtures -- `rake karma:tests` actually executes the tests - -As long as the fixtures don't change, `rake karma:tests` (or `yarn karma`) -is sufficient (and saves you some time). - -### Live testing and focused testing - -While developing locally, it may be helpful to keep karma running so that you -can get instant feedback on as you write tests and modify code. To do this -you can start karma with `npm run karma-start`. It will compile the javascript -assets and run a server at `http://localhost:9876/` where it will automatically -run the tests on any browser which connects to it. You can enter that url on -multiple browsers at once to have it run the tests on each in parallel. - -While karma is running, any changes you make will instantly trigger a recompile -and retest of the entire test suite, so you can see instantly if you've broken -a test with your changes. You can use [jasmine focused][jasmine-focus] or -excluded tests (with `fdescribe` or `xdescribe`) to get karma to run only the -tests you want while you're working on a specific feature, but make sure to -remove these directives when you commit your code. - -## RSpec Feature Integration Tests - -Information on setting up and running RSpec integration tests with -[Capybara][capybara] can be found in the -[general testing guide](../testing.md). - -## Gotchas - -### Errors due to use of unsupported JavaScript features - -Similar errors will be thrown if you're using JavaScript features not yet -supported by the PhantomJS test runner which is used for both Karma and RSpec -tests. We polyfill some JavaScript objects for older browsers, but some -features are still unavailable: - -- Array.from -- Array.first -- Async functions -- Generators -- Array destructuring -- For..Of -- Symbol/Symbol.iterator -- Spread - -Until these are polyfilled appropriately, they should not be used. Please -update this list with additional unsupported features. - -### RSpec errors due to JavaScript - -By default RSpec unit tests will not run JavaScript in the headless browser -and will simply rely on inspecting the HTML generated by rails. - -If an integration test depends on JavaScript to run correctly, you need to make -sure the spec is configured to enable JavaScript when the tests are run. If you -don't do this you'll see vague error messages from the spec runner. - -To enable a JavaScript driver in an `rspec` test, add `:js` to the -individual spec or the context block containing multiple specs that need -JavaScript enabled: - -```ruby -# For one spec -it 'presents information about abuse report', :js do - # assertions... -end - -describe "Admin::AbuseReports", :js do - it 'presents information about abuse report' do - # assertions... - end - it 'shows buttons for adding to abuse report' do - # assertions... - end -end -``` - -### Spinach errors due to missing JavaScript - -> **Note:** Since we are discouraging the use of Spinach when writing new -> feature tests, you shouldn't ever need to use this. This information is kept -> available for legacy purposes only. - -In Spinach, the JavaScript driver is enabled differently. In the `*.feature` -file for the failing spec, add the `@javascript` flag above the Scenario: - -``` -@javascript -Scenario: Developer can approve merge request - Given I am a "Shop" developer - And I visit project "Shop" merge requests page - And merge request 'Bug NS-04' must be approved - And I click link "Bug NS-04" - When I click link "Approve" - Then I should see approved merge request "Bug NS-04" -``` - -[capybara]: http://teamcapybara.github.io/capybara/ -[jasmine]: https://jasmine.github.io/ -[jasmine-focus]: https://jasmine.github.io/2.5/focused_specs.html -[jasmine-jquery]: https://github.com/velesin/jasmine-jquery -[karma]: http://karma-runner.github.io/ -[vue-test]:https://docs.gitlab.com/ce/development/fe_guide/vue.html#testing-vue-components +This document was moved to [../testing_guide/frontend_testing.md](../testing_guide/frontend_testing.md). diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index 2607353782a..f88f0753687 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -179,6 +179,7 @@ itself, please read this guide: [State Management][state-management] The Service is a class used only to communicate with the server. It does not store or manipulate any data. It is not aware of the store or the components. We use [vue-resource][vue-resource-repo] to communicate with the server. +Refer to [vue resource](vue_resource.md) for more details. Vue Resource should only be imported in the service file. @@ -189,55 +190,6 @@ Vue Resource should only be imported in the service file. Vue.use(VueResource); ``` -#### Vue-resource gotchas -#### Headers -Headers are being parsed into a plain object in an interceptor. -In Vue-resource 1.x `headers` object was changed into an `Headers` object. In order to not change all old code, an interceptor was added. - -If you need to write a unit test that takes the headers in consideration, you need to include an interceptor to parse the headers after your test interceptor. -You can see an example in `spec/javascripts/environments/environment_spec.js`: - ```javascript - import { headersInterceptor } from './helpers/vue_resource_helper'; - - beforeEach(() => { - Vue.http.interceptors.push(myInterceptor); - Vue.http.interceptors.push(headersInterceptor); - }); - - afterEach(() => { - Vue.http.interceptors = _.without(Vue.http.interceptors, myInterceptor); - Vue.http.interceptors = _.without(Vue.http.interceptors, headersInterceptor); - }); - ``` - -#### `.json()` -When making a request to the server, you will most likely need to access the body of the response. -Use `.json()` to convert. Because `.json()` returns a Promise the follwoing structure should be used: - - ```javascript - service.get('url') - .then(resp => resp.json()) - .then((data) => { - this.store.storeData(data); - }) - .catch(() => new Flash('Something went wrong')); - ``` - -When using `Poll` (`app/assets/javascripts/lib/utils/poll.js`), the `successCallback` needs to handle `.json()` as a Promise: - ```javascript - successCallback: (response) => { - return response.json().then((data) => { - // handle the response - }); - } - ``` - -#### CSRF token -We use a Vue Resource interceptor to manage the CSRF token. -`app/assets/javascripts/vue_shared/vue_resource_interceptor.js` holds all our common interceptors. -Note: You don't need to load `app/assets/javascripts/vue_shared/vue_resource_interceptor.js` -since it's already being loaded by `common_vue.js`. - ### End Result The following example shows an application: @@ -428,7 +380,7 @@ is a good example of this pattern. ## Style guide -Please refer to the Vue section of our [style guide](style_guide_js.md#vuejs) +Please refer to the Vue section of our [style guide](style_guide_js.md#vue-js) for best practices while writing your Vue components and templates. ## Testing Vue Components @@ -769,7 +721,6 @@ describe('component', () => { [component-system]: https://vuejs.org/v2/guide/#Composing-with-Components [state-management]: https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch [one-way-data-flow]: https://vuejs.org/v2/guide/components.html#One-Way-Data-Flow -[vue-resource-repo]: https://github.com/pagekit/vue-resource [vue-resource-interceptor]: https://github.com/pagekit/vue-resource/blob/develop/docs/http.md#interceptors [vue-test]: https://vuejs.org/v2/guide/unit-testing.html [issue-boards-service]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/boards/services/board_service.js.es6 diff --git a/doc/development/fe_guide/vue_resource.md b/doc/development/fe_guide/vue_resource.md new file mode 100644 index 00000000000..c376c5c32bf --- /dev/null +++ b/doc/development/fe_guide/vue_resource.md @@ -0,0 +1,72 @@ +# Vue Resouce +In Vue applications we use [vue-resource][vue-resource-repo] to communicate with the server. + +## HTTP Status Codes + +### `.json()` +When making a request to the server, you will most likely need to access the body of the response. +Use `.json()` to convert. Because `.json()` returns a Promise the follwoing structure should be used: + + ```javascript + service.get('url') + .then(resp => resp.json()) + .then((data) => { + this.store.storeData(data); + }) + .catch(() => new Flash('Something went wrong')); + ``` + + +When using `Poll` (`app/assets/javascripts/lib/utils/poll.js`), the `successCallback` needs to handle `.json()` as a Promise: + ```javascript + successCallback: (response) => { + return response.json().then((data) => { + // handle the response + }); + } + ``` + +### 204 +Some endpoints - usually `delete` endpoints - return `204` as the success response. +When handling `204 - No Content` responses, we cannot use `.json()` since it tries to parse the non-existant body content. + +When handling `204` responses, do not use `.json`, otherwise the promise will throw an error and will enter the `catch` statement: + +```javascript + Vue.http.delete('path') + .then(() => { + // success! + }) + .catch(() => { + // handle error + }) +``` + +## Headers +Headers are being parsed into a plain object in an interceptor. +In Vue-resource 1.x `headers` object was changed into an `Headers` object. In order to not change all old code, an interceptor was added. + +If you need to write a unit test that takes the headers in consideration, you need to include an interceptor to parse the headers after your test interceptor. +You can see an example in `spec/javascripts/environments/environment_spec.js`: + ```javascript + import { headersInterceptor } from './helpers/vue_resource_helper'; + + beforeEach(() => { + Vue.http.interceptors.push(myInterceptor); + Vue.http.interceptors.push(headersInterceptor); + }); + + afterEach(() => { + Vue.http.interceptors = _.without(Vue.http.interceptors, myInterceptor); + Vue.http.interceptors = _.without(Vue.http.interceptors, headersInterceptor); + }); + ``` + +## CSRF token +We use a Vue Resource interceptor to manage the CSRF token. +`app/assets/javascripts/vue_shared/vue_resource_interceptor.js` holds all our common interceptors. +Note: You don't need to load `app/assets/javascripts/vue_shared/vue_resource_interceptor.js` +since it's already being loaded by `common_vue.js`. + + +[vue-resource-repo]: https://github.com/pagekit/vue-resource diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md new file mode 100644 index 00000000000..ca2048c7019 --- /dev/null +++ b/doc/development/gitaly.md @@ -0,0 +1,102 @@ +# GitLab Developers Guide to Working with Gitaly + +[Gitaly](https://gitlab.com/gitlab-org/gitaly) is a high-level Git RPC service used by GitLab CE/EE, +Workhorse and GitLab-Shell. All Rugged operations in GitLab CE/EE are currently being phased out to +be replaced by Gitaly API calls. + +Visit the [Gitaly Migration Board](https://gitlab.com/gitlab-org/gitaly/boards/331341) for current +status of the migration. + +## Feature Flags + +Gitaly makes heavy use of [feature flags](feature_flags.md). + +Each Rugged-to-Gitaly migration goes through a [series of phases](https://gitlab.com/gitlab-org/gitaly/blob/master/doc/MIGRATION_PROCESS.md): + +* **Opt-In**: by default the Rugged implementation is used. + * Production instances can choose to enable the Gitaly endpoint by enabling the feature flag. + * For testing purposes, you may wish to enable all feature flags by default. This can be done by exporting the following + environment variable: `GITALY_FEATURE_DEFAULT_ON=1`. + * On developer instances (ie, when `Rails.env.development?` is true), the Gitaly endpoint + is enabled by default, but can be _disabled_ using feature flags. +* **Opt-Out**: by default, the Gitaly endpoint is used, but the feature can be explicitly disabled using the feature flag. +* **Mandatory**: The migration is complete and cannot be disabled. The old codepath is removed. + +### Enabling and Disabling Feature + +In the Rails console, type: + +```ruby +Feature.enable(:gitaly_feature_name) +Feature.disable(:gitaly_feature_name) +``` + +Where `gitaly_feature_name` is the name of the Gitaly feature. This can be determined by finding the appropriate +`gitaly_migrate` code block, for example: + +```ruby +gitaly_migrate(:tag_names) do +... +end +``` + +Since Gitaly features are always prefixed with `gitaly_`, the name of the feature flag in this case would be `gitaly_tag_names`. + +## Gitaly-Related Test Failures + +If your test-suite is failing with Gitaly issues, as a first step, try running: + +```shell +rm -rf tmp/tests/gitaly +``` + +## `TooManyInvocationsError` errors + +During development and testing, you may experience `Gitlab::GitalyClient::TooManyInvocationsError` failures. +The `GitalyClient` will attempt to block against potential n+1 issues by raising this error +when Gitaly is called more than 30 times in a single Rails request or Sidekiq execution. + +As a temporary measure, export `GITALY_DISABLE_REQUEST_LIMITS=1` to suppress the error. This will disable the n+1 detection +in your development environment. + +Please raise an issue in the GitLab CE or EE repositories to report the issue. Include the labels ~Gitaly +~performance ~"technical debt". Please ensure that the issue contains the full stack trace and error message of the +`TooManyInvocationsError`. Also include any known failing tests if possible. + +Isolate the source of the n+1 problem. This will normally be a loop that results in Gitaly being called for each +element in an array. If you are unable to isolate the problem, please contact a member +of the [Gitaly Team](https://gitlab.com/groups/gl-gitaly/group_members) for assistance. + +Once the source has been found, wrap it in an `allow_n_plus_1_calls` block, as follows: + +```ruby +# n+1: link to n+1 issue +Gitlab::GitalyClient.allow_n_plus_1_calls do + # original code + commits.each { |commit| ... } +end +``` + +Once the code is wrapped in this block, this code-path will be excluded from n+1 detection. + +## Request counts + +Commits and other git data, is now fetched through Gitaly. These fetches can, +much like with a database, be batched. This improves performance for the client +and for Gitaly itself and therefore for the users too. To keep performance stable +and guard performance regressions, Gitaly calls can be counted and the call count +can be tested against. This requires the `:request_store` flag to be set. + +```ruby +describe 'Gitaly Request count tests' do + context 'when the request store is activated', :request_store do + it 'correctly counts the gitaly requests made' do + expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(10) + end + end +end +``` + +--- + +[Return to Development documentation](README.md) diff --git a/doc/development/i18n/externalization.md b/doc/development/i18n/externalization.md new file mode 100644 index 00000000000..7c38260406d --- /dev/null +++ b/doc/development/i18n/externalization.md @@ -0,0 +1,316 @@ +# Internationalization for GitLab + +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10669) in GitLab 9.2. + +For working with internationalization (i18n), +[GNU gettext](https://www.gnu.org/software/gettext/) is used given it's the most +used tool for this task and there are a lot of applications that will help us to +work with it. + +## Setting up GitLab Development Kit (GDK) + +In order to be able to work on the [GitLab Community Edition](https://gitlab.com/gitlab-org/gitlab-ce) +project you must download and configure it through [GDK](https://gitlab.com/gitlab-org/gitlab-development-kit/blob/master/doc/set-up-gdk.md). + +Once you have the GitLab project ready, you can start working on the translation. + +## Tools + +The following tools are used: + +1. [`gettext_i18n_rails`](https://github.com/grosser/gettext_i18n_rails): this + gem allow us to translate content from models, views and controllers. Also + it gives us access to the following raketasks: + - `rake gettext:find`: Parses almost all the files from the + Rails application looking for content that has been marked for + translation. Finally, it updates the PO files with the new content that + it has found. + - `rake gettext:pack`: Processes the PO files and generates the + MO files that are binary and are finally used by the application. + +1. [`gettext_i18n_rails_js`](https://github.com/webhippie/gettext_i18n_rails_js): + this gem is useful to make the translations available in JavaScript. It + provides the following raketask: + - `rake gettext:po_to_json`: Reads the contents from the PO files and + generates JSON files containing all the available translations. + +1. PO editor: there are multiple applications that can help us to work with PO + files, a good option is [Poedit](https://poedit.net/download) which is + available for macOS, GNU/Linux and Windows. + +## Preparing a page for translation + +We basically have 4 types of files: + +1. Ruby files: basically Models and Controllers. +1. HAML files: these are the view files. +1. ERB files: used for email templates. +1. JavaScript files: we mostly need to work with VUE JS templates. + +### Ruby files + +If there is a method or variable that works with a raw string, for instance: + +```ruby +def hello + "Hello world!" +end +``` + +Or: + +```ruby +hello = "Hello world!" +``` + +You can easily mark that content for translation with: + +```ruby +def hello + _("Hello world!") +end +``` + +Or: + +```ruby +hello = _("Hello world!") +``` + +### HAML files + +Given the following content in HAML: + +```haml +%h1 Hello world! +``` + +You can mark that content for translation with: + +```haml +%h1= _("Hello world!") +``` + +### ERB files + +Given the following content in ERB: + +```erb +<h1>Hello world!</h1> +``` + +You can mark that content for translation with: + +```erb +<h1><%= _("Hello world!") %></h1> +``` + +### JavaScript files + +In JavaScript we added the `__()` (double underscore parenthesis) function +for translations. + +### Updating the PO files with the new content + +Now that the new content is marked for translation, we need to update the PO +files with the following command: + +```sh +bundle exec rake gettext:find +``` + +This command will update the `locale/**/gitlab.edit.po` file with the +new content that the parser has found. + +New translations will be added with their default content and will be marked +fuzzy. To use the translation, look for the `#, fuzzy` mention in `gitlab.edit.po` +and remove it. + +We need to make sure we remove the `fuzzy` translations before generating the +`locale/**/gitlab.po` file. When they aren't removed, the resulting `.po` will +be treated as a binary file which could overwrite translations that were merged +before the new translations. + +When we are just preparing a page to be translated, but not actually adding any +translations. There's no need to generate `.po` files. + +Translations that aren't used in the source code anymore will be marked with +`~#`; these can be removed to keep our translation files clutter-free. + +### Validating PO files + +To make sure we keep our translation files up to date, there's a linter that is +running on CI as part of the `static-analysis` job. + +To lint the adjustments in PO files locally you can run `rake gettext:lint`. + +The linter will take the following into account: + +- Valid PO-file syntax +- Variable usage + - Only one unnamed (`%d`) variable, since the order of variables might change + in different languages + - All variables used in the message-id are used in the translation + - There should be no variables used in a translation that aren't in the + message-id +- Errors during translation. + +The errors are grouped per file, and per message ID: + +``` +Errors in `locale/zh_HK/gitlab.po`: + PO-syntax errors + SimplePoParser::ParserErrorSyntax error in lines + Syntax error in msgctxt + Syntax error in msgid + Syntax error in msgstr + Syntax error in message_line + There should be only whitespace until the end of line after the double quote character of a message text. + Parseing result before error: '{:msgid=>["", "You are going to remove %{project_name_with_namespace}.\\n", "Removed project CANNOT be restored!\\n", "Are you ABSOLUTELY sure?"]}' + SimplePoParser filtered backtrace: SimplePoParser::ParserError +Errors in `locale/zh_TW/gitlab.po`: + 1 pipeline + <%d 條流水線> is using unknown variables: [%d] + Failure translating to zh_TW with []: too few arguments +``` + +In this output the `locale/zh_HK/gitlab.po` has syntax errors. +The `locale/zh_TW/gitlab.po` has variables that are used in the translation that +aren't in the message with id `1 pipeline`. + +## Working with special content + + +### Just marking content for parsing + +- In Ruby/HAML: + + ```ruby + _('Subscribe') + ``` + +- In JavaScript: + + ```js + import { __ } from '../../../locale'; + const label = __('Subscribe'); + ``` + + +Sometimes there are some dynamic translations that can't be found by the +parser when running `bundle exec rake gettext:find`. For these scenarios you can +use the [`_N` method](https://github.com/grosser/gettext_i18n_rails/blob/c09e38d481e0899ca7d3fc01786834fa8e7aab97/Readme.md#unfound-translations-with-rake-gettextfind). + +There is also and alternative method to [translate messages from validation errors](https://github.com/grosser/gettext_i18n_rails/blob/c09e38d481e0899ca7d3fc01786834fa8e7aab97/Readme.md#option-a). + +### Interpolation + +- In Ruby/HAML: + + ```ruby + _("Hello %{name}") % { name: 'Joe' } => 'Hello Joe' + ``` + +- In JavaScript: + + ```js + import { __, sprintf } from '../../../locale'; + sprintf(__('Hello %{username}'), { username: 'Joe' }) => 'Hello Joe' + ``` + +### Plurals + +- In Ruby/HAML: + + ```ruby + n_('Apple', 'Apples', 3) => 'Apples' + ``` + + Using interpolation: + ```ruby + n_("There is a mouse.", "There are %d mice.", size) % size + ``` + +- In JavaScript: + + ```js + n__('Apple', 'Apples', 3) => 'Apples' + ``` + + Using interpolation: + + ```js + n__('Last day', 'Last %d days', 30) => 'Last 30 days' + ``` + +### Namespaces + +Sometimes you need to add some context to the text that you want to translate +(if the word occurs in a sentence and/or the word is ambiguous). + +- In Ruby/HAML: + + ```ruby + s_('OpenedNDaysAgo|Opened') + ``` + + In case the translation is not found it will return `Opened`. + +- In JavaScript: + + ```js + s__('OpenedNDaysAgo|Opened') + ``` + +## Adding a new language + +Let's suppose you want to add translations for a new language, let's say French. + +1. The first step is to register the new language in `lib/gitlab/i18n.rb`: + + ```ruby + ... + AVAILABLE_LANGUAGES = { + ..., + 'fr' => 'Français' + }.freeze + ... + ``` + +1. Next, you need to add the language: + + ```sh + bundle exec rake gettext:add_language[fr] + ``` + + If you want to add a new language for a specific region, the command is similar, + you just need to separate the region with an underscore (`_`). For example: + + ```sh + bundle exec rake gettext:add_language[en_GB] + ``` + + Please note that you need to specify the region part in capitals. + +1. Now that the language is added, a new directory has been created under the + path: `locale/fr/`. You can now start using your PO editor to edit the PO file + located in: `locale/fr/gitlab.edit.po`. + +1. After you're done updating the translations, you need to process the PO files + in order to generate the binary MO files and finally update the JSON files + containing the translations: + + ```sh + bundle exec rake gettext:compile + ``` + +1. In order to see the translated content we need to change our preferred language + which can be found under the user's **Settings** (`/profile`). + +1. After checking that the changes are ok, you can proceed to commit the new files. + For example: + + ```sh + git add locale/fr/ app/assets/javascripts/locale/fr/ + git commit -m "Add French translations for Cycle Analytics page" + ``` diff --git a/doc/development/i18n/img/crowdin-editor.png b/doc/development/i18n/img/crowdin-editor.png Binary files differnew file mode 100644 index 00000000000..5c31d8f0cec --- /dev/null +++ b/doc/development/i18n/img/crowdin-editor.png diff --git a/doc/development/i18n/index.md b/doc/development/i18n/index.md new file mode 100644 index 00000000000..4cb2624c098 --- /dev/null +++ b/doc/development/i18n/index.md @@ -0,0 +1,76 @@ +# Translate GitLab to your language + +The text in GitLab's user interface is in American English by default. +Each string can be translated to other languages. +As each string is translated, it is added to the languages translation file, +and will be available in future releases of GitLab. + +Contributions to translations are always needed. +Many strings are not yet available for translation because they have not been externalized. +Helping externalize strings benefits all languages. +Some translations are incomplete or inconsistent. +Translating strings will help complete and improve each language. + +## How to contribute + +There are many ways you can contribute in translating GitLab. + +### Externalize strings + +Before a string can be translated, it must be externalized. +This is the process where English strings in the GitLab source code are wrapped in a function that +retrieves the translated string for the user's language. + +As new features are added and existing features are updated, the surrounding strings are being +externalized, however, there are many parts of GitLab that still need more work to externalize all +strings. + +See [Externalization for GitLab](externalization.md). + +### Translate strings + +The translation process is managed at [translate.gitlab.com](https://translate.gitlab.com) +using [Crowdin](https://crowdin.com/). +You will need to create an account before you can submit translations. +Once you are signed in, select the language you wish to contribute translations to. + +Voting for translations is also valuable, helping to confirm good and flag inaccurate translations. + +See [Translation guidelines](translation.md). + +### Proof reading + +Proof reading helps ensure the accuracy and consistency of translations. +All translations are proof read before being accepted. +If a translations requires changes, you will be notified with a comment explaining why. + +Community assistance proof reading translations is encouraged and appreciated. +Requests to become a proof reader will be considered on the merits of previous translations. + +- Bulgarian +- Chinese Simplified + - [Huang Tao](https://crowdin.com/profile/htve) +- Chinese Traditional + - [Huang Tao](https://crowdin.com/profile/htve) +- Chinese Traditional, Hong Kong + - [Huang Tao](https://crowdin.com/profile/htve) +- Dutch +- Esperanto +- French +- German +- Italian +- Japanese +- Korean + - [Huang Tao](https://crowdin.com/profile/htve) +- Portuguese, Brazilian +- Russian + - [Alexy Lustin](https://crowdin.com/profile/lustin) + - [Nikita Grylov](https://crowdin.com/profile/nixel2007) +- Spanish +- Ukrainian + +If you would like to be added as a proof reader, please [open an issue](https://gitlab.com/gitlab-org/gitlab-ce/issues). + +## Release + +Translations are typically included in the next major or minor release. diff --git a/doc/development/i18n/translation.md b/doc/development/i18n/translation.md new file mode 100644 index 00000000000..b34ec754742 --- /dev/null +++ b/doc/development/i18n/translation.md @@ -0,0 +1,76 @@ +# Translating GitLab + +For managing the translation process we use [Crowdin](https://crowdin.com). + +## Using Crowdin + +The first step is to get familiar with Crowdin. + +### Sign In + +To contribute translations at [translate.gitlab.com](https://translate.gitlab.com) +you must create a Crowdin account. +You may create a new account or use any of their supported sign in services. + +### Language Selections + +GitLab is being translated into many languages. + +1. Select the language you would like to contribute translations to by clicking the flag +1. You will see a list of files and folders. + Click `gitlab.pot` to open the translation editor. + +### Translation Editor + +The online translation editor is the easiest way to contribute translations. + + + +1. Strings for translation are listed in the left panel +1. Translations are entered into the central panel. + Multiple translations will be required for strings that contains plurals. + The string to be translated is shown above with glossary terms highlighted. + If the string to be translated is not clear, you can 'Request Context' + +A glossary of common terms is available in the right panel by clicking Terms. +Comments can be added to discuss a translation with the community. + +Remember to **Save** each translation. + +## Translation Guidelines + +Be sure to check the following guidelines before you translate any strings. + +### Technical terms + +Technical terms should be treated like proper nouns and not be translated. +This helps maintain a logical connection and consistency between tools (e.g. `git` client) and +GitLab. + +Technical terms that should always be in English are noted in the glossary when using +[translate.gitlab.com](https://translate.gitlab.com). + +### Formality + +The level of formality used in software varies by language. +For example, in French we translate `you` as the informal `tu`. + +You can refer to other translated strings and notes in the glossary to assist determining a +suitable level of formality. + +### Inclusive language + +[Diversity] is one of GitLab's values. +We ask you to avoid translations which exclude people based on their gender or ethnicity. +In languages which distinguish between a male and female form, +use both or choose a neutral formulation. + +For example in German, the word "user" can be translated into "Benutzer" (male) or "Benutzerin" (female). +Therefore "create a new user" would translate into "Benutzer(in) anlegen". + +[Diversity]: https://about.gitlab.com/handbook/values/#diversity + +### Updating the glossary + +To propose additions to the glossary please +[open an issue](https://gitlab.com/gitlab-org/gitlab-ce/issues). diff --git a/doc/development/i18n_guide.md b/doc/development/i18n_guide.md index bd0ef39ca62..f6e949b5fd8 100644 --- a/doc/development/i18n_guide.md +++ b/doc/development/i18n_guide.md @@ -1,297 +1 @@ -# Internationalization for GitLab - -> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10669) in GitLab 9.2. - -For working with internationalization (i18n) we use -[GNU gettext](https://www.gnu.org/software/gettext/) given it's the most used -tool for this task and we have a lot of applications that will help us to work -with it. - -## Setting up GitLab Development Kit (GDK) - -In order to be able to work on the [GitLab Community Edition](https://gitlab.com/gitlab-org/gitlab-ce) project we must download and -configure it through [GDK](https://gitlab.com/gitlab-org/gitlab-development-kit), we can do it by following this [guide](https://gitlab.com/gitlab-org/gitlab-development-kit/blob/master/doc/set-up-gdk.md). - -Once we have the GitLab project ready we can start working on the -translation of the project. - -## Tools - -We use a couple of gems: - -1. [`gettext_i18n_rails`](https://github.com/grosser/gettext_i18n_rails): this - gem allow us to translate content from models, views and controllers. Also - it gives us access to the following raketasks: - - `rake gettext:find`: Parses almost all the files from the - Rails application looking for content that has been marked for - translation. Finally, it updates the PO files with the new content that - it has found. - - `rake gettext:pack`: Processes the PO files and generates the - MO files that are binary and are finally used by the application. - -1. [`gettext_i18n_rails_js`](https://github.com/webhippie/gettext_i18n_rails_js): - this gem is useful to make the translations available in JavaScript. It - provides the following raketask: - - `rake gettext:po_to_json`: Reads the contents from the PO files and - generates JSON files containing all the available translations. - -1. PO editor: there are multiple applications that can help us to work with PO - files, a good option is [Poedit](https://poedit.net/download) which is - available for macOS, GNU/Linux and Windows. - -## Preparing a page for translation - -We basically have 4 types of files: - -1. Ruby files: basically Models and Controllers. -1. HAML files: these are the view files. -1. ERB files: used for email templates. -1. JavaScript files: we mostly need to work with VUE JS templates. - -### Ruby files - -If there is a method or variable that works with a raw string, for instance: - -```ruby -def hello - "Hello world!" -end -``` - -Or: - -```ruby -hello = "Hello world!" -``` - -You can easily mark that content for translation with: - -```ruby -def hello - _("Hello world!") -end -``` - -Or: - -```ruby -hello = _("Hello world!") -``` - -### HAML files - -Given the following content in HAML: - -```haml -%h1 Hello world! -``` - -You can mark that content for translation with: - -```haml -%h1= _("Hello world!") -``` - -### ERB files - -Given the following content in ERB: - -```erb -<h1>Hello world!</h1> -``` - -You can mark that content for translation with: - -```erb -<h1><%= _("Hello world!") %></h1> -``` - -### JavaScript files - -In JavaScript we added the `__()` (double underscore parenthesis) function -for translations. - -### Updating the PO files with the new content - -Now that the new content is marked for translation, we need to update the PO -files with the following command: - -```sh -bundle exec rake gettext:find -``` - -This command will update the `locale/**/gitlab.edit.po` file with the -new content that the parser has found. - -New translations will be added with their default content and will be marked -fuzzy. To use the translation, look for the `#, fuzzy` mention in `gitlab.edit.po` -and remove it. - -We need to make sure we remove the `fuzzy` translations before generating the -`locale/**/gitlab.po` file. When they aren't removed, the resulting `.po` will -be treated as a binary file which could overwrite translations that were merged -before the new translations. - -When we are just preparing a page to be translated, but not actually adding any -translations. There's no need to generate `.po` files. - -Translations that aren't used in the source code anymore will be marked with -`~#`; these can be removed to keep our translation files clutter-free. - -### Validating PO files - -To make sure we keep our translation files up to date, there's a linter that is -running on CI as part of the `static-analysis` job. - -To lint the adjustments in PO files locally you can run `rake gettext:lint`. - -The linter will take the following into account: - -- Valid PO-file syntax -- Variable usage - - Only one unnamed (`%d`) variable, since the order of variables might change - in different languages - - All variables used in the message-id are used in the translation - - There should be no variables used in a translation that aren't in the - message-id -- Errors during translation. - -The errors are grouped per file, and per message ID: - -``` -Errors in `locale/zh_HK/gitlab.po`: - PO-syntax errors - SimplePoParser::ParserErrorSyntax error in lines - Syntax error in msgctxt - Syntax error in msgid - Syntax error in msgstr - Syntax error in message_line - There should be only whitespace until the end of line after the double quote character of a message text. - Parseing result before error: '{:msgid=>["", "You are going to remove %{project_name_with_namespace}.\\n", "Removed project CANNOT be restored!\\n", "Are you ABSOLUTELY sure?"]}' - SimplePoParser filtered backtrace: SimplePoParser::ParserError -Errors in `locale/zh_TW/gitlab.po`: - 1 pipeline - <%d 條流水線> is using unknown variables: [%d] - Failure translating to zh_TW with []: too few arguments -``` - -In this output the `locale/zh_HK/gitlab.po` has syntax errors. -The `locale/zh_TW/gitlab.po` has variables that are used in the translation that -aren't in the message with id `1 pipeline`. - -## Working with special content - -### Interpolation - -- In Ruby/HAML: - - ```ruby - _("Hello %{name}") % { name: 'Joe' } - ``` - -- In JavaScript: Not supported at this moment. - -### Plurals - -- In Ruby/HAML: - - ```ruby - n_('Apple', 'Apples', 3) => 'Apples' - ``` - - Using interpolation: - ```ruby - n_("There is a mouse.", "There are %d mice.", size) % size - ``` - -- In JavaScript: - - ```js - n__('Apple', 'Apples', 3) => 'Apples' - ``` - - Using interpolation: - - ```js - n__('Last day', 'Last %d days', 30) => 'Last 30 days' - ``` - -### Namespaces - -Sometimes you need to add some context to the text that you want to translate -(if the word occurs in a sentence and/or the word is ambiguous). - -- In Ruby/HAML: - - ```ruby - s_('OpenedNDaysAgo|Opened') - ``` - - In case the translation is not found it will return `Opened`. - -- In JavaScript: - - ```js - s__('OpenedNDaysAgo|Opened') - ``` - -### Just marking content for parsing - -Sometimes there are some dynamic translations that can't be found by the -parser when running `bundle exec rake gettext:find`. For these scenarios you can -use the [`_N` method](https://github.com/grosser/gettext_i18n_rails/blob/c09e38d481e0899ca7d3fc01786834fa8e7aab97/Readme.md#unfound-translations-with-rake-gettextfind). - -There is also and alternative method to [translate messages from validation errors](https://github.com/grosser/gettext_i18n_rails/blob/c09e38d481e0899ca7d3fc01786834fa8e7aab97/Readme.md#option-a). - -## Adding a new language - -Let's suppose you want to add translations for a new language, let's say French. - -1. The first step is to register the new language in `lib/gitlab/i18n.rb`: - - ```ruby - ... - AVAILABLE_LANGUAGES = { - ..., - 'fr' => 'Français' - }.freeze - ... - ``` - -1. Next, you need to add the language: - - ```sh - bundle exec rake gettext:add_language[fr] - ``` - - If you want to add a new language for a specific region, the command is similar, - you just need to separate the region with an underscore (`_`). For example: - - ```sh - bundle exec rake gettext:add_language[en_GB] - ``` - - Please note that you need to specify the region part in capitals. - -1. Now that the language is added, a new directory has been created under the - path: `locale/fr/`. You can now start using your PO editor to edit the PO file - located in: `locale/fr/gitlab.edit.po`. - -1. After you're done updating the translations, you need to process the PO files - in order to generate the binary MO files and finally update the JSON files - containing the translations: - - ```sh - bundle exec rake gettext:compile - ``` - -1. In order to see the translated content we need to change our preferred language - which can be found under the user's **Settings** (`/profile`). - -1. After checking that the changes are ok, you can proceed to commit the new files. - For example: - - ```sh - git add locale/fr/ app/assets/javascripts/locale/fr/ - git commit -m "Add French translations for Cycle Analytics page" - ``` +This document was moved to [a new location](i18n/index.md). diff --git a/doc/development/img/manual_build_docs.png b/doc/development/img/manual_build_docs.png Binary files differnew file mode 100644 index 00000000000..615facabb5f --- /dev/null +++ b/doc/development/img/manual_build_docs.png diff --git a/doc/development/licensing.md b/doc/development/licensing.md index 9a5811d8474..902b1c74a42 100644 --- a/doc/development/licensing.md +++ b/doc/development/licensing.md @@ -56,6 +56,7 @@ Libraries with the following licenses are acceptable for use: - [ISC License][ISC] (also known as the OpenBSD License): A permissive (non-copyleft) license as defined by the Open Source Initiative. - [Creative Commons Zero (CC0)][CC0]: A public domain dedication, recommended as a way to disclaim copyright on your work to the maximum extent possible. - [Unlicense][UNLICENSE]: Another public domain dedication. +- [OWFa 1.0][OWFa1]: An open-source license and patent grant designed for specifications. ## Unacceptable Licenses @@ -65,6 +66,7 @@ Libraries with the following licenses are unacceptable for use: - [GNU AGPLv3][AGPLv3]: AGPL-licensed libraries cannot be linked to from non-GPL projects. - [Open Software License (OSL)][OSL]: is a copyleft license. In addition, the FSF [recommend against its use][OSL-GNU]. - [Facebook BSD + PATENTS][Facebook]: is a 3-clause BSD license with a patent grant that has been deemed [Category X][x-list] by the Apache foundation. +- [WTFPL][WTFPL]: is a public domain dedication [rejected by the OSI (3.2)][WTFPL-OSI]. Also has a strong language which is not in accordance with our diversity policy. ## Requesting Approval for Licenses @@ -104,7 +106,10 @@ Gems which are included only in the "development" or "test" groups by Bundler ar [OSL-GNU]: https://www.gnu.org/licenses/license-list.en.html#OSL [Org-Repo]: https://gitlab.com/gitlab-com/organization [UNLICENSE]: https://unlicense.org +[OWFa1]: http://www.openwebfoundation.org/legal/the-owf-1-0-agreements/owfa-1-0 [Facebook]: https://code.facebook.com/pages/850928938376556 [x-list]: https://www.apache.org/legal/resolved.html#category-x [Acceptable-Licenses]: #acceptable-licenses [Unacceptable-Licenses]: #unacceptable-licenses +[WTFPL]: https://wtfpl.net +[WTFPL-OSI]: https://opensource.org/minutes20090304 diff --git a/doc/development/profiling.md b/doc/development/profiling.md index 933033a09e0..af79353b721 100644 --- a/doc/development/profiling.md +++ b/doc/development/profiling.md @@ -27,3 +27,13 @@ Bullet will log query problems to both the Rails log as well as the Chrome console. As a follow up to finding `N+1` queries with Bullet, consider writing a [QueryRecoder test](query_recorder.md) to prevent a regression. + +## GitLab Profiler + + +[Gitlab-Profiler](https://gitlab.com/gitlab-com/gitlab-profiler) was built to +help developers understand why specific URLs of their application may be slow +and to provide hard data that can help reduce load times. + +For GitLab.com, you can find the latest results here: +<http://redash.gitlab.com/dashboard/gitlab-profiler-statistics> diff --git a/doc/development/swapping_tables.md b/doc/development/swapping_tables.md new file mode 100644 index 00000000000..6b990ece72c --- /dev/null +++ b/doc/development/swapping_tables.md @@ -0,0 +1,53 @@ +# Swapping Tables + +Sometimes you need to replace one table with another. For example, when +migrating data in a very large table it's often better to create a copy of the +table and insert & migrate the data into this new table in the background. + +Let's say you want to swap the table "events" with "events_for_migration". In +this case you need to follow 3 steps: + +1. Rename "events" to "events_temporary" +2. Rename "events_for_migration" to "events" +3. Rename "events_temporary" to "events_for_migration" + +Rails allows you to do this using the `rename_table` method: + +```ruby +rename_table :events, :events_temporary +rename_table :events_for_migration, :events +rename_table :events_temporary, :events_for_migration +``` + +This does not require any downtime as long as the 3 `rename_table` calls are +executed in the _same_ database transaction. Rails by default uses database +transactions for migrations, but if it doesn't you'll need to start one +manually: + +```ruby +Event.transaction do + rename_table :events, :events_temporary + rename_table :events_for_migration, :events + rename_table :events_temporary, :events_for_migration +end +``` + +Once swapped you _have to_ reset the primary key of the new table. For +PostgreSQL you can use the `reset_pk_sequence!` method like so: + +```ruby +reset_pk_sequence!('events') +``` + +For MySQL however you need to do run the following: + +```ruby +amount = Event.pluck('COALESCE(MAX(id), 1)').first + +execute "ALTER TABLE events AUTO_INCREMENT = #{amount}" +``` + +Failure to reset the primary keys will result in newly created rows starting +with an ID value of 1. Depending on the existing data this can then lead to +duplicate key constraints from popping up, preventing users from creating new +data. diff --git a/doc/development/testing.md b/doc/development/testing.md index 83269303005..45b1519ece8 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -1,556 +1 @@ -# 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. - -## Definitions - -### Unit tests - -Formal definition: https://en.wikipedia.org/wiki/Unit_testing - -These kind of tests ensure that a single unit of code (a method) works as -expected (given an input, it has a predictable output). These tests should be -isolated as much as possible. For example, model methods that don't do anything -with the database shouldn't need a DB record. Classes that don't need database -records should use stubs/doubles as much as possible. - -| Code path | Tests path | Testing engine | Notes | -| --------- | ---------- | -------------- | ----- | -| `app/finders/` | `spec/finders/` | RSpec | | -| `app/helpers/` | `spec/helpers/` | RSpec | | -| `app/db/{post_,}migrate/` | `spec/migrations/` | RSpec | More details at [`spec/migrations/README.md`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/migrations/README.md). | -| `app/policies/` | `spec/policies/` | RSpec | | -| `app/presenters/` | `spec/presenters/` | RSpec | | -| `app/routing/` | `spec/routing/` | RSpec | | -| `app/serializers/` | `spec/serializers/` | RSpec | | -| `app/services/` | `spec/services/` | RSpec | | -| `app/tasks/` | `spec/tasks/` | RSpec | | -| `app/uploaders/` | `spec/uploaders/` | RSpec | | -| `app/views/` | `spec/views/` | RSpec | | -| `app/workers/` | `spec/workers/` | RSpec | | -| `app/assets/javascripts/` | `spec/javascripts/` | Karma | More details in the [JavaScript](#javascript) section. | - -### Integration tests - -Formal definition: https://en.wikipedia.org/wiki/Integration_testing - -These kind of tests ensure that individual parts of the application work well together, without the overhead of the actual app environment (i.e. the browser). These tests should assert at the request/response level: status code, headers, body. They're useful to test permissions, redirections, what view is rendered etc. - -| Code path | Tests path | Testing engine | Notes | -| --------- | ---------- | -------------- | ----- | -| `app/controllers/` | `spec/controllers/` | RSpec | | -| `app/mailers/` | `spec/mailers/` | RSpec | | -| `lib/api/` | `spec/requests/api/` | RSpec | | -| `lib/ci/api/` | `spec/requests/ci/api/` | RSpec | | -| `app/assets/javascripts/` | `spec/javascripts/` | Karma | More details in the [JavaScript](#javascript) section. | - -#### About controller tests - -In an ideal world, controllers should be thin. However, when this is not the -case, it's acceptable to write a system/feature test without JavaScript instead -of a controller test. The reason is that testing a fat controller usually -involves a lot of stubbing, things like: - -```ruby -controller.instance_variable_set(:@user, user) -``` - -and use methods which are deprecated in Rails 5 ([#23768]). - -[#23768]: https://gitlab.com/gitlab-org/gitlab-ce/issues/23768 - -#### About Karma - -As you may have noticed, Karma is both in the Unit tests and the Integration -tests category. That's because Karma is a tool that provides an environment to -run JavaScript tests, so you can either run unit tests (e.g. test a single -JavaScript method), or integration tests (e.g. test a component that is composed -of multiple components). - -### System tests or Feature tests - -Formal definition: https://en.wikipedia.org/wiki/System_testing. - -These kind of tests ensure the application works as expected from a user point -of view (aka black-box testing). These tests should test a happy path for a -given page or set of pages, and a test case should be added for any regression -that couldn't have been caught at lower levels with better tests (i.e. if a -regression is found, regression tests should be added at the lowest-level -possible). - -| Tests path | Testing engine | Notes | -| ---------- | -------------- | ----- | -| `spec/features/` | [Capybara] + [RSpec] | If your spec has the `:js` metadata, the browser driver will be [Poltergeist], otherwise it's using [RackTest]. | -| `features/` | Spinach | Spinach tests are deprecated, [you shouldn't add new Spinach tests](#spinach-feature-tests). | - -[Capybara]: https://github.com/teamcapybara/capybara -[RSpec]: https://github.com/rspec/rspec-rails#feature-specs -[Poltergeist]: https://github.com/teamcapybara/capybara#poltergeist -[RackTest]: https://github.com/teamcapybara/capybara#racktest - -#### Best practices - -- Create only the necessary records in the database -- Test a happy path and a less happy path but that's it -- Every other possible path should be tested with Unit or Integration tests -- Test what's displayed on the page, not the internals of ActiveRecord models. - For instance, if you want to verify that a record was created, add - expectations that its attributes are displayed on the page, not that - `Model.count` increased by one. -- It's ok to look for DOM elements but don't abuse it since it makes the tests - more brittle - -If we're confident that the low-level components work well (and we should be if -we have enough Unit & Integration tests), we shouldn't need to duplicate their -thorough testing at the System test level. - -It's very easy to add tests, but a lot harder to remove or improve tests, so one -should take care of not introducing too many (slow and duplicated) specs. - -The reasons why we should follow these best practices are as follows: - -- System tests are slow to run since they spin up the entire application stack - in a headless browser, and even slower when they integrate a JS driver -- When system tests run with a JavaScript driver, the tests are run in a - different thread than the application. This means it does not share a - database connection and your test will have to commit the transactions in - order for the running application to see the data (and vice-versa). In that - case we need to truncate the database after each spec instead of simply - rolling back a transaction (the faster strategy that's in use for other kind - of tests). This is slower than transactions, however, so we want to use - truncation only when necessary. - -### Black-box tests or End-to-end tests - -GitLab consists of [multiple pieces] such as [GitLab Shell], [GitLab Workhorse], -[Gitaly], [GitLab Pages], [GitLab Runner], and GitLab Rails. All theses pieces -are configured and packaged by [GitLab Omnibus]. - -[GitLab QA] is a tool that allows to test that all these pieces integrate well -together by building a Docker image for a given version of GitLab Rails and -running feature tests (i.e. using Capybara) against it. - -The actual test scenarios and steps are [part of GitLab Rails] so that they're -always in-sync with the codebase. - -[multiple pieces]: ./architecture.md#components -[GitLab Shell]: https://gitlab.com/gitlab-org/gitlab-shell -[GitLab Workhorse]: https://gitlab.com/gitlab-org/gitlab-workhorse -[Gitaly]: https://gitlab.com/gitlab-org/gitaly -[GitLab Pages]: https://gitlab.com/gitlab-org/gitlab-pages -[GitLab Runner]: https://gitlab.com/gitlab-org/gitlab-ci-multi-runner -[GitLab Omnibus]: https://gitlab.com/gitlab-org/omnibus-gitlab -[GitLab QA]: https://gitlab.com/gitlab-org/gitlab-qa -[part of GitLab Rails]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/qa - -## How to test at the correct level? - -As many things in life, deciding what to test at each level of testing is a -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 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) - section. - -Another way to see it is to think about the "cost of tests", this is well -explained [in this article][tests-cost] and the basic idea is that the cost of a -test includes: - -- The time it takes to write the test -- The time it takes to run the test every time the suite runs -- The time it takes to understand the test -- The time it takes to fix the test if it breaks and the underlying code is OK -- Maybe, the time it takes to change the code to make the code testable. - -[miss]: https://twitter.com/ThePracticalDev/status/850748070698651649 -[big]: https://twitter.com/timbray/status/822470746773409794 -[picture]: https://twitter.com/withzombies/status/829716565834752000 -[tests-cost]: https://medium.com/table-xi/high-cost-tests-and-high-value-tests-a86e27a54df#.2ulyh3a4e - -## Frontend testing - -Please consult the [dedicated "Frontend testing" guide](./fe_guide/testing.md). - -## RSpec - -### General Guidelines - -- Use a single, top-level `describe ClassName` block. -- Use `.method` to describe class methods and `#method` to describe instance - methods. -- Use `context` to test branching logic. -- 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)). -- 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. -- 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 -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 - -#### `set` variables - -In some cases there is no need to recreate the same object for tests again for -each example. For example, a project is needed to test issues on the same -project, one project will do for the entire file. This can be achieved by using -`set` in the same way you would use `let`. - -`rspec-set` only works on ActiveRecord objects, and before new examples it -reloads or recreates the model, _only_ if needed. That is, when you changed -properties or destroyed the object. - -There is one gotcha; you can't reference a model defined in a `let` block in a -`set` block. - -### Time-sensitive tests - -[Timecop](https://github.com/travisjeffery/timecop) is available in our -Ruby-based tests for verifying things that are time-sensitive. Any test that -exercises or verifies something time-sensitive should make use of Timecop to -prevent transient test failures. - -Example: - -```ruby -it 'is overdue' do - issue = build(:issue, due_date: Date.tomorrow) - - Timecop.freeze(3.days.from_now) do - expect(issue).to be_overdue - end -end -``` - -### System / Feature tests - -- Feature specs 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. - -### 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 -complexity of RSpec expectations.They should be placed under -`spec/support/matchers/`. Matchers can be placed in subfolder if they apply to -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/`. -Shared contexts can be placed in subfolder if they apply to a certain type of -specs only (e.g. features, requests etc.) but shouldn't be if they apply to -multiple type of specs. - -Each file should include only one context and have a descriptive name, e.g. -`spec/support/shared_contexts/controllers/githubish_import_controller_shared_context.rb`. - -### Shared examples - -All shared examples should be be placed under `spec/support/shared_examples/`. -Shared examples can be placed in subfolder if they apply to a certain type of -specs only (e.g. features, requests etc.) but shouldn't be if they apply to -multiple type of specs. - -Each file should include only one context and have a descriptive name, e.g. -`spec/support/shared_examples/controllers/githubish_import_controller_shared_example.rb`. - -### Helpers - -Helpers are usually modules that provide some methods to hide the complexity of -specific RSpec examples. You can define helpers in RSpec files if they're not -intended to be shared with other specs. Otherwise, they should be be placed -under `spec/support/helpers/`. Helpers can be placed in subfolder if they apply -to a certain type of specs only (e.g. features, requests etc.) but shouldn't be -if they apply to multiple type of specs. - -Helpers should follow the Rails naming / namespacing convention. For instance -`spec/support/helpers/cycle_analytics_helpers.rb` should define: - -```ruby -module Spec - module Support - module Helpers - module CycleAnalyticsHelpers - def create_commit_referencing_issue(issue, branch_name: random_git_name) - project.repository.add_branch(user, branch_name, 'master') - create_commit("Commit for ##{issue.iid}", issue.project, user, branch_name) - end - end - end - end -end -``` - -Helpers should not change the RSpec config. For instance, the helpers module -described above should not include: - -```ruby -RSpec.configure do |config| - config.include Spec::Support::Helpers::CycleAnalyticsHelpers -end -``` - -### 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. -- 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 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 - -### Fixtures - -All fixtures should be be placed under `spec/fixtures/`. - -### Config - -RSpec config files are files that change the RSpec config (i.e. -`RSpec.configure do |config|` blocks). They should be placed under -`spec/support/config/`. - -Each file should be related to a specific domain, e.g. -`spec/support/config/capybara.rb`, `spec/support/config/carrierwave.rb`, etc. - -Helpers can be included in the `spec/support/config/rspec.rb` file. If a -helpers module applies only to a certain kind of specs, it should add modifiers -to the `config.include` call. For instance if -`spec/support/helpers/cycle_analytics_helpers.rb` applies to `:lib` and -`type: :model` specs only, you would write the following: - -```ruby -RSpec.configure do |config| - config.include Spec::Support::Helpers::CycleAnalyticsHelpers, :lib - config.include Spec::Support::Helpers::CycleAnalyticsHelpers, type: :model -end -``` - -## Testing Rake Tasks - -To make testing Rake tasks a little easier, there is a helper that can be included -in lieu of the standard Spec helper. Instead of `require 'spec_helper'`, use -`require 'rake_helper'`. The helper includes `spec_helper` for you, and configures -a few other things to make testing Rake tasks easier. - -At a minimum, requiring the Rake helper will redirect `stdout`, include the -runtime task helpers, and include the `RakeHelpers` Spec support module. - -The `RakeHelpers` module exposes a `run_rake_task(<task>)` method to make -executing tasks simple. See `spec/support/rake_helpers.rb` for all available -methods. - -Example: - -```ruby -require 'rake_helper' - -describe 'gitlab:shell rake tasks' do - before do - Rake.application.rake_require 'tasks/gitlab/shell' - - stub_warn_user_is_not_gitlab - end - - describe 'install task' do - it 'invokes create_hooks task' do - expect(Rake::Task['gitlab:shell:create_hooks']).to receive(:invoke) - - run_rake_task('gitlab:shell:install') - end - end -end -``` - -## Test speed - -GitLab has a massive test suite that, without [parallelization], can take hours -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! -- 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! - -[parallelization]: #test-suite-parallelization-on-the-ci - -### Test suite parallelization on the CI - -Our current CI parallelization setup is as follows: - -1. The `knapsack` job in the prepare stage that is supposed to ensure we have a - `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file: - - The `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file is fetched - from S3, if it's not here we initialize the file with `{}`. -1. Each `rspec x y` job are run with `knapsack rspec` and should have an evenly - distributed share of tests: - - It works because the jobs have access to the - `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` since the "artifacts - from all previous stages are passed by default". [^1] - - the jobs set their own report path to - `KNAPSACK_REPORT_PATH=knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json`. - - if knapsack is doing its job, test files that are run should be listed under - `Report specs`, not under `Leftover specs`. -1. The `update-knapsack` job takes all the - `knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json` - files from the `rspec x y` jobs and merge them all together into a single - `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file that is then - uploaded to S3. - -After that, the next pipeline will use the up-to-date -`knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file. The same strategy -is used for Spinach tests as well. - -### Monitoring - -The GitLab test suite is [monitored] for the `master` branch, and any branch -that includes `rspec-profile` in their name. - -A [public dashboard] is available for everyone to see. Feel free to look at the -slowest test files and try to improve them. - -[monitored]: ./performance.md#rspec-profiling -[public dashboard]: https://redash.gitlab.com/public/dashboards/l1WhHXaxrCWM5Ai9D7YDqHKehq6OU3bx5gssaiWe?org_slug=default - -## CI setup - -- On CE and EE, the test suite runs both PostgreSQL and MySQL. -- Rails logging to `log/test.log` is disabled by default in CI [for - performance reasons][logging]. To override this setting, provide the - `RAILS_ENABLE_TEST_LOG` environment variable. - -[logging]: https://jtway.co/speed-up-your-rails-test-suite-by-6-in-1-line-13fedb869ec4 - -## 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. - ---- - -[Return to Development documentation](README.md) - -[^1]: /ci/yaml/README.html#dependencies +This document was moved to [testing_guide/index.md](testing_guide/index.md). diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md new file mode 100644 index 00000000000..8b7b015427f --- /dev/null +++ b/doc/development/testing_guide/best_practices.md @@ -0,0 +1,303 @@ +# Testing best practices + +## Test speed + +GitLab has a massive test suite that, without [parallelization], can take hours +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! +- 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! + +[parallelization]: ci.md#test-suite-parallelization-on-the-ci + +## RSpec + +### General guidelines + +- Use a single, top-level `describe ClassName` block. +- Use `.method` to describe class methods and `#method` to describe instance + methods. +- Use `context` to test branching logic. +- 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)). +- 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. +- 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` +- When using `evaluate_script("$('.js-foo').testSomething()")` (or `execute_script`) which acts on a given element, + use a Capyabara matcher beforehand (e.g. `find('.js-foo')`) to ensure the element actually exists. + +[four-phase-test]: https://robots.thoughtbot.com/four-phase-test + +### System / Feature tests + +NOTE: **Note:** Before writing a new system test, [please consider **not** +writing one](testing_levels.md#consider-not-writing-a-system-test)! + +- Feature specs should be named `ROLE_ACTION_spec.rb`, such as + `user_changes_password_spec.rb`. +- 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. +- Create only the necessary records in the database +- Test a happy path and a less happy path but that's it +- Every other possible path should be tested with Unit or Integration tests +- Test what's displayed on the page, not the internals of ActiveRecord models. + For instance, if you want to verify that a record was created, add + expectations that its attributes are displayed on the page, not that + `Model.count` increased by one. +- It's ok to look for DOM elements but don't abuse it since it makes the tests + more brittle + +#### Debugging Capybara + +Sometimes you may need to debug Capybara tests by observing browser behavior. + +You can pause Capybara and view the website on the browser by using the +`live_debug` method in your spec. The current page will be automatically opened +in your default browser. +You may need to sign in first (the current user's credentials are displayed in +the terminal). + +To resume the test run, press any key. + +For example: + +``` +$ bin/rspec spec/features/auto_deploy_spec.rb:34 +Running via Spring preloader in process 8999 +Run options: include {:locations=>{"./spec/features/auto_deploy_spec.rb"=>[34]}} + +Current example is paused for live debugging +The current user credentials are: user2 / 12345678 +Press any key to resume the execution of the example! +Back to the example! +. + +Finished in 34.51 seconds (files took 0.76702 seconds to load) +1 example, 0 failures +``` + +### `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 + +### `set` variables + +In some cases there is no need to recreate the same object for tests again for +each example. For example, a project is needed to test issues on the same +project, one project will do for the entire file. This can be achieved by using +`set` in the same way you would use `let`. + +`rspec-set` only works on ActiveRecord objects, and before new examples it +reloads or recreates the model, _only_ if needed. That is, when you changed +properties or destroyed the object. + +There is one gotcha; you can't reference a model defined in a `let` block in a +`set` block. + +### Time-sensitive tests + +[Timecop](https://github.com/travisjeffery/timecop) is available in our +Ruby-based tests for verifying things that are time-sensitive. Any test that +exercises or verifies something time-sensitive should make use of Timecop to +prevent transient test failures. + +Example: + +```ruby +it 'is overdue' do + issue = build(:issue, due_date: Date.tomorrow) + + Timecop.freeze(3.days.from_now) do + expect(issue).to be_overdue + end +end +``` + +### 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 +complexity of RSpec expectations.They should be placed under +`spec/support/matchers/`. Matchers can be placed in subfolder if they apply to +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/`. +Shared contexts can be placed in subfolder if they apply to a certain type of +specs only (e.g. features, requests etc.) but shouldn't be if they apply to +multiple type of specs. + +Each file should include only one context and have a descriptive name, e.g. +`spec/support/shared_contexts/controllers/githubish_import_controller_shared_context.rb`. + +### Shared examples + +All shared examples should be be placed under `spec/support/shared_examples/`. +Shared examples can be placed in subfolder if they apply to a certain type of +specs only (e.g. features, requests etc.) but shouldn't be if they apply to +multiple type of specs. + +Each file should include only one context and have a descriptive name, e.g. +`spec/support/shared_examples/controllers/githubish_import_controller_shared_example.rb`. + +### Helpers + +Helpers are usually modules that provide some methods to hide the complexity of +specific RSpec examples. You can define helpers in RSpec files if they're not +intended to be shared with other specs. Otherwise, they should be be placed +under `spec/support/helpers/`. Helpers can be placed in subfolder if they apply +to a certain type of specs only (e.g. features, requests etc.) but shouldn't be +if they apply to multiple type of specs. + +Helpers should follow the Rails naming / namespacing convention. For instance +`spec/support/helpers/cycle_analytics_helpers.rb` should define: + +```ruby +module Spec + module Support + module Helpers + module CycleAnalyticsHelpers + def create_commit_referencing_issue(issue, branch_name: random_git_name) + project.repository.add_branch(user, branch_name, 'master') + create_commit("Commit for ##{issue.iid}", issue.project, user, branch_name) + end + end + end + end +end +``` + +Helpers should not change the RSpec config. For instance, the helpers module +described above should not include: + +```ruby +RSpec.configure do |config| + config.include Spec::Support::Helpers::CycleAnalyticsHelpers +end +``` + +### 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. +- 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 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 + +### Fixtures + +All fixtures should be be placed under `spec/fixtures/`. + +### Config + +RSpec config files are files that change the RSpec config (i.e. +`RSpec.configure do |config|` blocks). They should be placed under +`spec/support/config/`. + +Each file should be related to a specific domain, e.g. +`spec/support/config/capybara.rb`, `spec/support/config/carrierwave.rb`, etc. + +Helpers can be included in the `spec/support/config/rspec.rb` file. If a +helpers module applies only to a certain kind of specs, it should add modifiers +to the `config.include` call. For instance if +`spec/support/helpers/cycle_analytics_helpers.rb` applies to `:lib` and +`type: :model` specs only, you would write the following: + +```ruby +RSpec.configure do |config| + config.include Spec::Support::Helpers::CycleAnalyticsHelpers, :lib + config.include Spec::Support::Helpers::CycleAnalyticsHelpers, type: :model +end +``` + +--- + +[Return to Testing documentation](index.md) diff --git a/doc/development/testing_guide/ci.md b/doc/development/testing_guide/ci.md new file mode 100644 index 00000000000..e90de55068d --- /dev/null +++ b/doc/development/testing_guide/ci.md @@ -0,0 +1,52 @@ +# GitLab tests in the Continuous Integration (CI) context + +### Test suite parallelization on the CI + +Our current CI parallelization setup is as follows: + +1. The `knapsack` job in the prepare stage that is supposed to ensure we have a + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file: + - The `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file is fetched + from S3, if it's not here we initialize the file with `{}`. +1. Each `rspec x y` job are run with `knapsack rspec` and should have an evenly + distributed share of tests: + - It works because the jobs have access to the + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` since the "artifacts + from all previous stages are passed by default". [^1] + - the jobs set their own report path to + `KNAPSACK_REPORT_PATH=knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json`. + - if knapsack is doing its job, test files that are run should be listed under + `Report specs`, not under `Leftover specs`. +1. The `update-knapsack` job takes all the + `knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json` + files from the `rspec x y` jobs and merge them all together into a single + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file that is then + uploaded to S3. + +After that, the next pipeline will use the up-to-date +`knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file. The same strategy +is used for Spinach tests as well. + +### Monitoring + +The GitLab test suite is [monitored] for the `master` branch, and any branch +that includes `rspec-profile` in their name. + +A [public dashboard] is available for everyone to see. Feel free to look at the +slowest test files and try to improve them. + +[monitored]: ../performance.md#rspec-profiling +[public dashboard]: https://redash.gitlab.com/public/dashboards/l1WhHXaxrCWM5Ai9D7YDqHKehq6OU3bx5gssaiWe?org_slug=default + +## CI setup + +- On CE and EE, the test suite runs both PostgreSQL and MySQL. +- Rails logging to `log/test.log` is disabled by default in CI [for + performance reasons][logging]. To override this setting, provide the + `RAILS_ENABLE_TEST_LOG` environment variable. + +[logging]: https://jtway.co/speed-up-your-rails-test-suite-by-6-in-1-line-13fedb869ec4 + +--- + +[Return to Testing documentation](index.md) diff --git a/doc/development/testing_guide/flaky_tests.md b/doc/development/testing_guide/flaky_tests.md new file mode 100644 index 00000000000..bbb2313ea7b --- /dev/null +++ b/doc/development/testing_guide/flaky_tests.md @@ -0,0 +1,74 @@ +# Flaky tests + +## What's a flaky test? + +It's a test that sometimes fails, but if you retry it enough times, it passes, +eventually. + +## 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. + +This was originally implemented in: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13021. + +[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 + +## Problems we had in the past at GitLab + +- [`rspec-retry` is bitting us when some API specs fail](https://gitlab.com/gitlab-org/gitlab-ce/issues/29242): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9825 +- [Sporadic RSpec failures due to `PG::UniqueViolation`](https://gitlab.com/gitlab-org/gitlab-ce/issues/28307#note_24958837): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9846 + - Follow-up: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10688 + - [Capybara.reset_session! should be called before requests are blocked](https://gitlab.com/gitlab-org/gitlab-ce/issues/33779): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12224 +- FFaker generates funky data that tests are not ready to handle (and tests should be predictable so that's bad!): + - [Make `spec/mailers/notify_spec.rb` more robust](https://gitlab.com/gitlab-org/gitlab-ce/issues/20121): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10015 + - [Transient failure in spec/requests/api/commits_spec.rb](https://gitlab.com/gitlab-org/gitlab-ce/issues/27988#note_25342521): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9944 + - [Replace FFaker factory data with sequences](https://gitlab.com/gitlab-org/gitlab-ce/issues/29643): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10184 + - [Transient failure in spec/finders/issues_finder_spec.rb](https://gitlab.com/gitlab-org/gitlab-ce/issues/30211#note_26707685): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10404 + +### Time-sensitive flaky tests + +- https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10046 +- https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10306 + +### Array order expectation + +- https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10148 + +### Feature tests + +- [Be sure to create all the data the test need before starting exercize](https://gitlab.com/gitlab-org/gitlab-ce/issues/32622#note_31128195): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12059 +- [Bis](https://gitlab.com/gitlab-org/gitlab-ce/issues/34609#note_34048715): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12604 +- [Bis](https://gitlab.com/gitlab-org/gitlab-ce/issues/34698#note_34276286): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12664 +- [Assert against the underlying database state instead of against a page's content](https://gitlab.com/gitlab-org/gitlab-ce/issues/31437): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10934 + +#### Capybara viewport size related issues + +- [Transient failure of spec/features/issues/filtered_search/filter_issues_spec.rb](https://gitlab.com/gitlab-org/gitlab-ce/issues/29241#note_26743936): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10411 + +#### Capybara JS driver related issues + +- [Don't wait for AJAX when no AJAX request is fired](https://gitlab.com/gitlab-org/gitlab-ce/issues/30461): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10454 +- [Bis](https://gitlab.com/gitlab-org/gitlab-ce/issues/34647): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12626 + +#### PhantomJS / WebKit related issues + +- Memory is through the roof! (TL;DR: Load images but block images requests!): https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12003 + +## Resources + +- [Flaky Tests: Are You Sure You Want to Rerun Them?](http://semaphoreci.com/blog/2017/04/20/flaky-tests.html) +- [How to Deal With and Eliminate Flaky Tests](https://semaphoreci.com/community/tutorials/how-to-deal-with-and-eliminate-flaky-tests) +- [Tips on Treating Flakiness in your Rails Test Suite](http://semaphoreci.com/blog/2017/08/03/tips-on-treating-flakiness-in-your-test-suite.html) +- ['Flaky' tests: a short story](https://www.ombulabs.com/blog/rspec/continuous-integration/how-to-track-down-a-flaky-test.html) +- [Using Insights to Discover Flaky, Slow, and Failed Tests](https://circleci.com/blog/using-insights-to-discover-flaky-slow-and-failed-tests/) + +--- + +[Return to Testing documentation](index.md) diff --git a/doc/development/testing_guide/frontend_testing.md b/doc/development/testing_guide/frontend_testing.md new file mode 100644 index 00000000000..0c63f51cb45 --- /dev/null +++ b/doc/development/testing_guide/frontend_testing.md @@ -0,0 +1,254 @@ +# Frontend testing standards and style guidelines + +There are two types of test suites you'll encounter while developing frontend code +at GitLab. We use Karma and Jasmine for JavaScript unit and integration testing, +and RSpec feature tests with Capybara for e2e (end-to-end) integration testing. + +Unit and feature tests need to be written for all new features. +Most of the time, you should use [RSpec] for your feature tests. + +Regression tests should be written for bug fixes to prevent them from recurring +in the future. + +See the [Testing Standards and Style Guidelines](index.md) page for more +information on general testing practices at GitLab. + +## Karma test suite + +GitLab uses the [Karma][karma] test runner with [Jasmine] as its test +framework for our JavaScript unit and integration tests. For integration tests, +we generate HTML files using RSpec (see `spec/javascripts/fixtures/*.rb` for examples). +Some fixtures are still HAML templates that are translated to HTML files using the same mechanism (see `static_fixtures.rb`). +Adding these static fixtures should be avoided as they are harder to keep up to date with real views. +The existing static fixtures will be migrated over time. +Please see [gitlab-org/gitlab-ce#24753](https://gitlab.com/gitlab-org/gitlab-ce/issues/24753) to track our progress. +Fixtures are served during testing by the [jasmine-jquery][jasmine-jquery] plugin. + +JavaScript tests live in `spec/javascripts/`, matching the folder structure +of `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js` +has a corresponding `spec/javascripts/behaviors/autosize_spec.js` file. + +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. + +### Best practices + +#### Naming unit tests + +When writing describe test blocks to test specific functions/methods, +please use the method name as the describe block name. + +```javascript +// Good +describe('methodName', () => { + it('passes', () => { + expect(true).toEqual(true); + }); +}); + +// Bad +describe('#methodName', () => { + it('passes', () => { + expect(true).toEqual(true); + }); +}); + +// Bad +describe('.methodName', () => { + it('passes', () => { + expect(true).toEqual(true); + }); +}); +``` +#### Testing promises + +When testing Promises you should always make sure that the test is asynchronous and rejections are handled. +Your Promise chain should therefore end with a call of the `done` callback and `done.fail` in case an error occurred. + +```javascript +// Good +it('tests a promise', (done) => { + promise + .then((data) => { + expect(data).toBe(asExpected); + }) + .then(done) + .catch(done.fail); +}); + +// Good +it('tests a promise rejection', (done) => { + promise + .then(done.fail) + .catch((error) => { + expect(error).toBe(expectedError); + }) + .then(done) + .catch(done.fail); +}); + +// Bad (missing done callback) +it('tests a promise', () => { + promise + .then((data) => { + expect(data).toBe(asExpected); + }) +}); + +// Bad (missing catch) +it('tests a promise', (done) => { + promise + .then((data) => { + expect(data).toBe(asExpected); + }) + .then(done) +}); + +// Bad (use done.fail in asynchronous tests) +it('tests a promise', (done) => { + promise + .then((data) => { + expect(data).toBe(asExpected); + }) + .then(done) + .catch(fail) +}); + +// Bad (missing catch) +it('tests a promise rejection', (done) => { + promise + .catch((error) => { + expect(error).toBe(expectedError); + }) + .then(done) +}); +``` + +#### Stubbing + +For unit tests, you should stub methods that are unrelated to the current unit you are testing. +If you need to use a prototype method, instantiate an instance of the class and call it there instead of mocking the instance completely. + +For integration tests, you should stub methods that will effect the stability of the test if they +execute their original behaviour. i.e. Network requests. + +### Vue.js unit tests + +See this [section][vue-test]. + +### Running frontend tests + +`rake karma` runs the frontend-only (JavaScript) tests. +It consists of two subtasks: + +- `rake karma:fixtures` (re-)generates fixtures +- `rake karma:tests` actually executes the tests + +As long as the fixtures don't change, `rake karma:tests` (or `yarn karma`) +is sufficient (and saves you some time). + +### Live testing and focused testing + +While developing locally, it may be helpful to keep karma running so that you +can get instant feedback on as you write tests and modify code. To do this +you can start karma with `npm run karma-start`. It will compile the javascript +assets and run a server at `http://localhost:9876/` where it will automatically +run the tests on any browser which connects to it. You can enter that url on +multiple browsers at once to have it run the tests on each in parallel. + +While karma is running, any changes you make will instantly trigger a recompile +and retest of the entire test suite, so you can see instantly if you've broken +a test with your changes. You can use [jasmine focused][jasmine-focus] or +excluded tests (with `fdescribe` or `xdescribe`) to get karma to run only the +tests you want while you're working on a specific feature, but make sure to +remove these directives when you commit your code. + +## RSpec feature integration tests + +Information on setting up and running RSpec integration tests with +[Capybara] can be found in the [Testing Best Practices](best_practices.md). + +## Gotchas + +### Errors due to use of unsupported JavaScript features + +Similar errors will be thrown if you're using JavaScript features not yet +supported by the PhantomJS test runner which is used for both Karma and RSpec +tests. We polyfill some JavaScript objects for older browsers, but some +features are still unavailable: + +- Array.from +- Array.first +- Async functions +- Generators +- Array destructuring +- For..Of +- Symbol/Symbol.iterator +- Spread + +Until these are polyfilled appropriately, they should not be used. Please +update this list with additional unsupported features. + +### RSpec errors due to JavaScript + +By default RSpec unit tests will not run JavaScript in the headless browser +and will simply rely on inspecting the HTML generated by rails. + +If an integration test depends on JavaScript to run correctly, you need to make +sure the spec is configured to enable JavaScript when the tests are run. If you +don't do this you'll see vague error messages from the spec runner. + +To enable a JavaScript driver in an `rspec` test, add `:js` to the +individual spec or the context block containing multiple specs that need +JavaScript enabled: + +```ruby +# For one spec +it 'presents information about abuse report', :js do + # assertions... +end + +describe "Admin::AbuseReports", :js do + it 'presents information about abuse report' do + # assertions... + end + it 'shows buttons for adding to abuse report' do + # assertions... + end +end +``` + +### Spinach errors due to missing JavaScript + +NOTE: **Note:** Since we are discouraging the use of Spinach when writing new +feature tests, you shouldn't ever need to use this. This information is kept +available for legacy purposes only. + +In Spinach, the JavaScript driver is enabled differently. In the `*.feature` +file for the failing spec, add the `@javascript` flag above the Scenario: + +``` +@javascript +Scenario: Developer can approve merge request + Given I am a "Shop" developer + And I visit project "Shop" merge requests page + And merge request 'Bug NS-04' must be approved + And I click link "Bug NS-04" + When I click link "Approve" + Then I should see approved merge request "Bug NS-04" +``` + +[jasmine-focus]: https://jasmine.github.io/2.5/focused_specs.html +[jasmine-jquery]: https://github.com/velesin/jasmine-jquery +[karma]: http://karma-runner.github.io/ +[vue-test]:https://docs.gitlab.com/ce/development/fe_guide/vue.html#testing-vue-components +[RSpec]: https://github.com/rspec/rspec-rails#feature-specs +[Capybara]: https://github.com/teamcapybara/capybara +[Karma]: http://karma-runner.github.io/ +[Jasmine]: https://jasmine.github.io/ + +--- + +[Return to Testing documentation](index.md) diff --git a/doc/development/fe_guide/img/testing_triangle.png b/doc/development/testing_guide/img/testing_triangle.png Binary files differindex 7a9a848c2ee..7a9a848c2ee 100644 --- a/doc/development/fe_guide/img/testing_triangle.png +++ b/doc/development/testing_guide/img/testing_triangle.png diff --git a/doc/development/testing_guide/index.md b/doc/development/testing_guide/index.md new file mode 100644 index 00000000000..8045bbad7ba --- /dev/null +++ b/doc/development/testing_guide/index.md @@ -0,0 +1,91 @@ +# Testing standards and style guidelines + +This document describes various guidelines and best practices for automated +testing of the GitLab project. + +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. + +## Overview + +GitLab is built on top of [Ruby on Rails][rails], and we're using [RSpec] for all +the backend tests, with [Capybara] for end-to-end integration testing. +On the frontend side, we're using [Karma] and [Jasmine] for JavaScript unit and +integration testing. + +Following are two great articles that everyone should read to understand what +automated testing means, and what are its principles: + +- [Five Factor Testing](https://www.devmynd.com/blog/five-factor-testing): Why do we need tests? +- [Principles of Automated Testing](http://www.lihaoyi.com/post/PrinciplesofAutomatedTesting.html): Levels of testing. Prioritize tests. Cost of tests. + +--- + +## [Testing levels](testing_levels.md) + +Learn about the different testing levels, and how to decide at what level your +changes should be tested. + +--- + +## [Testing best practices](best_practices.md) + +Everything you should know about how to write good tests: RSpec, FactoryGirl, +system tests, parameterized tests etc. + +--- + +## [Frontend testing standards and style guidelines](frontend_testing.md) + +Everything you should know about how to write good Frontend tests: Karma, +testing promises, stubbing etc. + +--- + +## [Flaky tests](flaky_tests.md) + +What are flaky tests, the different kind of flaky tests we encountered, and what +we do about them. + +--- + +## [GitLab tests in the Continuous Integration (CI) context](ci.md) + +How GitLab test suite is run in the CI context: setup, caches, artifacts, +parallelization, monitoring. + +--- + +## [Testing Rake tasks](testing_rake_tasks.md) + +Everything you should know about how to test Rake tasks. + +--- + +## 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. + +--- + +[Return to Development documentation](../README.md) + +[^1]: /ci/yaml/README.html#dependencies + +[rails]: http://rubyonrails.org/ +[RSpec]: https://github.com/rspec/rspec-rails#feature-specs +[Capybara]: https://github.com/teamcapybara/capybara +[Karma]: http://karma-runner.github.io/ +[Jasmine]: https://jasmine.github.io/ diff --git a/doc/development/testing_guide/testing_levels.md b/doc/development/testing_guide/testing_levels.md new file mode 100644 index 00000000000..1cbd4350284 --- /dev/null +++ b/doc/development/testing_guide/testing_levels.md @@ -0,0 +1,173 @@ +# Testing levels + + + +_This diagram demonstrates the relative priority of each test type we use. `e2e` stands for end-to-end._ + +## Unit tests + +Formal definition: https://en.wikipedia.org/wiki/Unit_testing + +These kind of tests ensure that a single unit of code (a method) works as +expected (given an input, it has a predictable output). These tests should be +isolated as much as possible. For example, model methods that don't do anything +with the database shouldn't need a DB record. Classes that don't need database +records should use stubs/doubles as much as possible. + +| Code path | Tests path | Testing engine | Notes | +| --------- | ---------- | -------------- | ----- | +| `app/finders/` | `spec/finders/` | RSpec | | +| `app/helpers/` | `spec/helpers/` | RSpec | | +| `app/db/{post_,}migrate/` | `spec/migrations/` | RSpec | More details at [`spec/migrations/README.md`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/migrations/README.md). | +| `app/policies/` | `spec/policies/` | RSpec | | +| `app/presenters/` | `spec/presenters/` | RSpec | | +| `app/routing/` | `spec/routing/` | RSpec | | +| `app/serializers/` | `spec/serializers/` | RSpec | | +| `app/services/` | `spec/services/` | RSpec | | +| `app/tasks/` | `spec/tasks/` | RSpec | | +| `app/uploaders/` | `spec/uploaders/` | RSpec | | +| `app/views/` | `spec/views/` | RSpec | | +| `app/workers/` | `spec/workers/` | RSpec | | +| `app/assets/javascripts/` | `spec/javascripts/` | Karma | More details in the [Frontent Testing guide](frontend_testing.md) section. | + +## Integration tests + +Formal definition: https://en.wikipedia.org/wiki/Integration_testing + +These kind of tests ensure that individual parts of the application work well together, without the overhead of the actual app environment (i.e. the browser). These tests should assert at the request/response level: status code, headers, body. They're useful to test permissions, redirections, what view is rendered etc. + +| Code path | Tests path | Testing engine | Notes | +| --------- | ---------- | -------------- | ----- | +| `app/controllers/` | `spec/controllers/` | RSpec | | +| `app/mailers/` | `spec/mailers/` | RSpec | | +| `lib/api/` | `spec/requests/api/` | RSpec | | +| `lib/ci/api/` | `spec/requests/ci/api/` | RSpec | | +| `app/assets/javascripts/` | `spec/javascripts/` | Karma | More details in the [JavaScript](#javascript) section. | + +### About controller tests + +In an ideal world, controllers should be thin. However, when this is not the +case, it's acceptable to write a system/feature test without JavaScript instead +of a controller test. The reason is that testing a fat controller usually +involves a lot of stubbing, things like: + +```ruby +controller.instance_variable_set(:@user, user) +``` + +and use methods which are deprecated in Rails 5 ([#23768]). + +[#23768]: https://gitlab.com/gitlab-org/gitlab-ce/issues/23768 + +### About Karma + +As you may have noticed, Karma is both in the Unit tests and the Integration +tests category. That's because Karma is a tool that provides an environment to +run JavaScript tests, so you can either run unit tests (e.g. test a single +JavaScript method), or integration tests (e.g. test a component that is composed +of multiple components). + +## System tests or feature tests + +Formal definition: https://en.wikipedia.org/wiki/System_testing. + +These kind of tests ensure the application works as expected from a user point +of view (aka black-box testing). These tests should test a happy path for a +given page or set of pages, and a test case should be added for any regression +that couldn't have been caught at lower levels with better tests (i.e. if a +regression is found, regression tests should be added at the lowest-level +possible). + +| Tests path | Testing engine | Notes | +| ---------- | -------------- | ----- | +| `spec/features/` | [Capybara] + [RSpec] | If your spec has the `:js` metadata, the browser driver will be [Poltergeist], otherwise it's using [RackTest]. | +| `features/` | Spinach | Spinach tests are deprecated, [you shouldn't add new Spinach tests](#spinach-feature-tests). | + +### Consider **not** writing a system test! + +If we're confident that the low-level components work well (and we should be if +we have enough Unit & Integration tests), we shouldn't need to duplicate their +thorough testing at the System test level. + +It's very easy to add tests, but a lot harder to remove or improve tests, so one +should take care of not introducing too many (slow and duplicated) specs. + +The reasons why we should follow these best practices are as follows: + +- System tests are slow to run since they spin up the entire application stack + in a headless browser, and even slower when they integrate a JS driver +- When system tests run with a JavaScript driver, the tests are run in a + different thread than the application. This means it does not share a + database connection and your test will have to commit the transactions in + order for the running application to see the data (and vice-versa). In that + case we need to truncate the database after each spec instead of simply + rolling back a transaction (the faster strategy that's in use for other kind + of tests). This is slower than transactions, however, so we want to use + truncation only when necessary. + +[Poltergeist]: https://github.com/teamcapybara/capybara#poltergeist +[RackTest]: https://github.com/teamcapybara/capybara#racktest + +## Black-box tests or end-to-end tests + +GitLab consists of [multiple pieces] such as [GitLab Shell], [GitLab Workhorse], +[Gitaly], [GitLab Pages], [GitLab Runner], and GitLab Rails. All theses pieces +are configured and packaged by [GitLab Omnibus]. + +[GitLab QA] is a tool that allows to test that all these pieces integrate well +together by building a Docker image for a given version of GitLab Rails and +running feature tests (i.e. using Capybara) against it. + +The actual test scenarios and steps are [part of GitLab Rails] so that they're +always in-sync with the codebase. + +[multiple pieces]: ../architecture.md#components +[GitLab Shell]: https://gitlab.com/gitlab-org/gitlab-shell +[GitLab Workhorse]: https://gitlab.com/gitlab-org/gitlab-workhorse +[Gitaly]: https://gitlab.com/gitlab-org/gitaly +[GitLab Pages]: https://gitlab.com/gitlab-org/gitlab-pages +[GitLab Runner]: https://gitlab.com/gitlab-org/gitlab-runner +[GitLab Omnibus]: https://gitlab.com/gitlab-org/omnibus-gitlab +[GitLab QA]: https://gitlab.com/gitlab-org/gitlab-qa +[part of GitLab Rails]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/qa + +## How to test at the correct level? + +As many things in life, deciding what to test at each level of testing is a +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 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) + section. + +Another way to see it is to think about the "cost of tests", this is well +explained [in this article][tests-cost] and the basic idea is that the cost of a +test includes: + +- The time it takes to write the test +- The time it takes to run the test every time the suite runs +- The time it takes to understand the test +- The time it takes to fix the test if it breaks and the underlying code is OK +- Maybe, the time it takes to change the code to make the code testable. + +### Frontend-related tests + +There are cases where the behaviour you are testing is not worth the time spent +running the full application, for example, if you are testing styling, animation, +edge cases or small actions that don't involve the backend, +you should write an integration test using Jasmine. + +[miss]: https://twitter.com/ThePracticalDev/status/850748070698651649 +[big]: https://twitter.com/timbray/status/822470746773409794 +[picture]: https://twitter.com/withzombies/status/829716565834752000 +[tests-cost]: https://medium.com/table-xi/high-cost-tests-and-high-value-tests-a86e27a54df#.2ulyh3a4e + +--- + +[Return to Testing documentation](index.md) diff --git a/doc/development/testing_guide/testing_rake_tasks.md b/doc/development/testing_guide/testing_rake_tasks.md new file mode 100644 index 00000000000..60163f1a230 --- /dev/null +++ b/doc/development/testing_guide/testing_rake_tasks.md @@ -0,0 +1,39 @@ +# Testing Rake tasks + +To make testing Rake tasks a little easier, there is a helper that can be included +in lieu of the standard Spec helper. Instead of `require 'spec_helper'`, use +`require 'rake_helper'`. The helper includes `spec_helper` for you, and configures +a few other things to make testing Rake tasks easier. + +At a minimum, requiring the Rake helper will redirect `stdout`, include the +runtime task helpers, and include the `RakeHelpers` Spec support module. + +The `RakeHelpers` module exposes a `run_rake_task(<task>)` method to make +executing tasks simple. See `spec/support/rake_helpers.rb` for all available +methods. + +Example: + +```ruby +require 'rake_helper' + +describe 'gitlab:shell rake tasks' do + before do + Rake.application.rake_require 'tasks/gitlab/shell' + + stub_warn_user_is_not_gitlab + end + + describe 'install task' do + it 'invokes create_hooks task' do + expect(Rake::Task['gitlab:shell:create_hooks']).to receive(:invoke) + + run_rake_task('gitlab:shell:install') + end + end +end +``` + +--- + +[Return to Testing documentation](index.md) diff --git a/doc/development/ux_guide/animation.md b/doc/development/ux_guide/animation.md index 5dae4bcc905..d190ee1b0ff 100644 --- a/doc/development/ux_guide/animation.md +++ b/doc/development/ux_guide/animation.md @@ -39,6 +39,12 @@ When information is updating in place, a quick, subtle animation is needed. The  +### Skeleton loading + +Skeleton loading is explained in the [component section](components.html#skeleton-loading) of the UX guide. It includes a horizontally pulsating animation that shows motion as if it's growing. It's timing is a slower `linear 1s`. + + + ### Moving transitions When elements move on screen, there should be a quick animation so it is clear to users what moved where. The timing of this animation differs based on the amount of movement and change. Consider animations between `200ms` and `400ms`. @@ -51,7 +57,9 @@ View the [interactive example](http://codepen.io/awhildy/full/ALyKPE/) here.  #### Autoscroll the page + Another example of a moving transition is when you have to autoscroll the page to keep an active element visible. View the [interactive example](http://codepen.io/awhildy/full/PbxgVo/) here. -
\ No newline at end of file + + diff --git a/doc/development/ux_guide/basics.md b/doc/development/ux_guide/basics.md index a436e9b1948..e215026bcca 100644 --- a/doc/development/ux_guide/basics.md +++ b/doc/development/ux_guide/basics.md @@ -32,19 +32,17 @@ This is the typeface used for code blocks and references to commits, branches, a --- ## Icons -GitLab uses Font Awesome icons throughout our interface. -| | | -| :-----------: | :---- | -|  | The trash icon is used for destructive actions that deletes information. | -|  | The pencil icon is used for editing content such as comments.| -|  | The bell icon is for notifications, such as Todos. | -|  | The eye icon is for subscribing to updates. For example, you can subscribe to a label and get updated on issues with that label. | -|  | The standard RSS icon is used for linking to RSS/atom feeds. | -|  | An 'x' is used for closing UI elements such as dropdowns. | -|  | A plus is used when creating new objects, such as issues, projects, etc. | - -> TODO: update this section, add more general guidance to icon usage and personality, etc. +GitLab has a strong, unique personality. When you look at any screen, you should know immediately that it is GitLab. +Iconography is a powerful visual cue to the user and is a great way for us to reflect our particular sense of style. + +- **Standard size:** 16px * 16px +- **Border thickness:** 2px +- **Border radius:** 3px + + + +> TODO: List all icons, proper usage, hover, and active states. --- diff --git a/doc/development/ux_guide/components.md b/doc/development/ux_guide/components.md index ac7c1b6207d..fa31c496b30 100644 --- a/doc/development/ux_guide/components.md +++ b/doc/development/ux_guide/components.md @@ -42,6 +42,37 @@ By default, tooltips should be placed below the referring element. However, if t --- +## Popovers + +Popovers provide additional, useful, unique information about the referring elements and can provide one or multiple actionable elements. They inform the user of additional information within the context of their original view, but without forcing the user to act upon it like a modal. Popovers are different from tooltips, which do not provide rich markup and actionable items. A popover can contain a header section with a different background color. + +Popovers are summoned: + +* Upon hover or touch on an element + +### Usage +A popover should be used: +* When you don't want to let the user lose context, but still want to provide additional useful unique information about referring elements +* When it isn’t critical for the user to act upon the information +* When you want to give a user a summary of extended information and the option to switch context if they want to dive in deeper. + +### Styling + +A popover can contain a header section with a different background color if that improves readability and separation of content within. + + + +This example shows two sections, where each section includes an actionable element. The first section shows a summary of the content shown when clicking the "read more" link. With this information the user can decide to dive deeper or start their GitLab Enterprise Edition trial immediately. + +### Placement +By default, tooltips should be placed below the referring element. However, if there isn’t enough space in the viewport or it blocks related content, the tooltip should be moved to the side or above as needed. + + + +In this example we let the user know more about the setting they are deciding over, without loosing context. If they want to know even more they can do so, but with the expectation of opening that content in a new view. + +--- + ## Anchor links Anchor links are used for navigational actions and lone, secondary commands (such as 'Reset filters' on the Issues List) when deemed appropriate by the UX team. @@ -204,6 +235,25 @@ Cover blocks are generally used to create a heading element for a page, such as --- +## Skeleton loading + +Skeleton loading is a way to convey to the user what kind of content is currently being loaded. It's a paradigm with which content can independently and asynchronously be loaded, while still adhering to the structure and look of the completely loaded view. + +### Requirements + +* A skeleton should represent an organism in a recognisable way +* Atom elements within organisms (for reference see this article on [atomic design methodology](http://atomicdesign.bradfrost.com/chapter-2/)) may be represented in a maximum of 3 repetitions, if applicable. +* Skeletons should only be presented in grayscale using the HEX colors: `#fafafa` or `#ffffff` (except for shadows) +* Animate the grey atoms in a pulsating way to show motion, as if "loading". The pulse animation transitions colors horizontally from left to right, starting with `#f2f2f2` to `#fafafa`. + + + +### Usage + +Skeleton loading can replace any existing UI elements for the period in which they are loaded and should aim for maintaining a similar structure visually. + +--- + ## Panels > TODO: Catalog how we are currently using panels and rationalize how they relate to alerts diff --git a/doc/development/ux_guide/illustrations.md b/doc/development/ux_guide/illustrations.md new file mode 100644 index 00000000000..7e16c300921 --- /dev/null +++ b/doc/development/ux_guide/illustrations.md @@ -0,0 +1,86 @@ +# Illustrations + +The illustrations should always align with topics and goals in specific context. + +## Principles + +#### Be simple. +- For clarity, we use simple and specific elements to create our illustrations. + +#### Be optimistic. +- We are an open-minded, optimistic, and friendly team. We should reflect those values in our illustrations to connect with our brand experience. + +#### Be gentle. +- Our illustrations assist users in understanding context and guide users in the right direction. Illustrations are supportive, so they should be obvious but not aggressive. + + +## Style + +#### Shapes +- All illustrations are geometric rather than organic. +- The illustrations are made by circles, rectangles, squares, and triangles. + +<img src="img/illustrations-geometric.png" width=224px alt="Example for geometric" /> + +#### Stroke +- Standard border thickness: **4px** +- Depending on the situation, border thickness can be changed to **3px**. For example, when the illustration size is small, an illustration with 4px border thickness would look tight. In this case, the border thickness can be changed to 3px. +- We use **rounded caps** and **rounded corner**. + +| Do | Don't | +| -------- | -------- | +| <img src="img/illustrations-caps-do.png" width= 133px alt="Do: caps and corner" /> | <img src="img/illustrations-caps-don't.png" width= 133px alt="Don't: caps and corner"/> | + +#### Radius +- Standard corner radius: **10px** +- Depending on the situation, corner radius can be changed to **5px**. For example, when the illustration size is small, an illustration with 10px corner radius would be over-rounded. In this case, the corner radius can be changed to 5px. + +<img src="img/illustrations-border-radius.png" width= 464px alt="Example for border radius"/> + +#### Size +Depends on the situation, the illustration size can be the 3 types below: + +**Large** +* Use case: Empty states, error pages(e.g. 404, 403) +* For vertical layout, the illustration should not larger than **430*300 px**. +* For horizontal layout, the illustration should not larger than **430*380 px**. + +| Vertical layout | Horizontal layout | +| --------------- | ----------------- | +| <img src="img/illustration-size-large-vertical.png" /> | <img src="img/illustration-size-large-horizontal.png" /> + +**Medium** +* Use case: Banner +* The illustration should not larger than **240*160 px** +* The illustration should keep simple and clear. We recommend not including too many elements in the medium size illustration. + +<img src="img/illustration-size-medium.png" width=983px /> + +**Small** +* Use case: Graphics for explanatory text, graphics for status. +* The illustration should not larger than **160*90 px**. +* The illustration should keep simple and clear. We recommend not including too many elements in the small size illustration. + +<img src="img/illustration-size-small.png" width=983px /> + +**Illustration on mobile** +- Keep the proportions in original ratio. + +#### Colors palette + +For consistency, we recommend choosing colors from our color palette. + +| Orange | Purple | Grey | +| -------- | -------- | -------- | +| <img src="img/illustrations-color-orange.png" width= 160px alt="Orange" /> | <img src="img/illustrations-color-purple.png" width= 160px alt="Purple" /> | <img src="img/illustrations-color-grey.png" width= 160px alt="Grey" /> | +| #FC6D26 | #6B4FBB | #EEEEEE | + +#### Don't +- Don't include the typography in the illustration. +- Don't include tanuki in the illustration. If necessary, we recommend having tanuki monochromatic. + +--- + +| Orange | Purple | +| -------- | -------- | +| <img src="img/illustrations-palette-oragne.png" width= 160px alt="Palette - Orange" /> | <img src="img/illustrations-palette-purple.png" width= 160px alt="Palette - Purple" /> | diff --git a/doc/development/ux_guide/img/icon-spec.png b/doc/development/ux_guide/img/icon-spec.png Binary files differnew file mode 100644 index 00000000000..56b19610dc1 --- /dev/null +++ b/doc/development/ux_guide/img/icon-spec.png diff --git a/doc/development/ux_guide/img/illustration-size-large-horizontal.png b/doc/development/ux_guide/img/illustration-size-large-horizontal.png Binary files differnew file mode 100644 index 00000000000..8aa835adccc --- /dev/null +++ b/doc/development/ux_guide/img/illustration-size-large-horizontal.png diff --git a/doc/development/ux_guide/img/illustration-size-large-vertical.png b/doc/development/ux_guide/img/illustration-size-large-vertical.png Binary files differnew file mode 100644 index 00000000000..813b6a065e5 --- /dev/null +++ b/doc/development/ux_guide/img/illustration-size-large-vertical.png diff --git a/doc/development/ux_guide/img/illustration-size-medium.png b/doc/development/ux_guide/img/illustration-size-medium.png Binary files differnew file mode 100644 index 00000000000..55cfe1dcb91 --- /dev/null +++ b/doc/development/ux_guide/img/illustration-size-medium.png diff --git a/doc/development/ux_guide/img/illustration-size-small.png b/doc/development/ux_guide/img/illustration-size-small.png Binary files differnew file mode 100644 index 00000000000..0124f58f48e --- /dev/null +++ b/doc/development/ux_guide/img/illustration-size-small.png diff --git a/doc/development/ux_guide/img/illustrations-border-radius.png b/doc/development/ux_guide/img/illustrations-border-radius.png Binary files differnew file mode 100644 index 00000000000..4e2fef5c7f5 --- /dev/null +++ b/doc/development/ux_guide/img/illustrations-border-radius.png diff --git a/doc/development/ux_guide/img/illustrations-caps-do.png b/doc/development/ux_guide/img/illustrations-caps-do.png Binary files differnew file mode 100644 index 00000000000..7a2c74382f6 --- /dev/null +++ b/doc/development/ux_guide/img/illustrations-caps-do.png diff --git a/doc/development/ux_guide/img/illustrations-caps-don't.png b/doc/development/ux_guide/img/illustrations-caps-don't.png Binary files differnew file mode 100644 index 00000000000..848f72dbe30 --- /dev/null +++ b/doc/development/ux_guide/img/illustrations-caps-don't.png diff --git a/doc/development/ux_guide/img/illustrations-color-grey.png b/doc/development/ux_guide/img/illustrations-color-grey.png Binary files differnew file mode 100644 index 00000000000..63855026c2b --- /dev/null +++ b/doc/development/ux_guide/img/illustrations-color-grey.png diff --git a/doc/development/ux_guide/img/illustrations-color-orange.png b/doc/development/ux_guide/img/illustrations-color-orange.png Binary files differnew file mode 100644 index 00000000000..96765c8c28c --- /dev/null +++ b/doc/development/ux_guide/img/illustrations-color-orange.png diff --git a/doc/development/ux_guide/img/illustrations-color-purple.png b/doc/development/ux_guide/img/illustrations-color-purple.png Binary files differnew file mode 100644 index 00000000000..745d2c853ba --- /dev/null +++ b/doc/development/ux_guide/img/illustrations-color-purple.png diff --git a/doc/development/ux_guide/img/illustrations-geometric.png b/doc/development/ux_guide/img/illustrations-geometric.png Binary files differnew file mode 100644 index 00000000000..33f05547bac --- /dev/null +++ b/doc/development/ux_guide/img/illustrations-geometric.png diff --git a/doc/development/ux_guide/img/illustrations-palette-oragne.png b/doc/development/ux_guide/img/illustrations-palette-oragne.png Binary files differnew file mode 100644 index 00000000000..15f35912646 --- /dev/null +++ b/doc/development/ux_guide/img/illustrations-palette-oragne.png diff --git a/doc/development/ux_guide/img/illustrations-palette-purple.png b/doc/development/ux_guide/img/illustrations-palette-purple.png Binary files differnew file mode 100644 index 00000000000..e0f5839705e --- /dev/null +++ b/doc/development/ux_guide/img/illustrations-palette-purple.png diff --git a/doc/development/ux_guide/img/popover-placement-above.png b/doc/development/ux_guide/img/popover-placement-above.png Binary files differnew file mode 100644 index 00000000000..1aa044bfc9c --- /dev/null +++ b/doc/development/ux_guide/img/popover-placement-above.png diff --git a/doc/development/ux_guide/img/popover-placement-below.png b/doc/development/ux_guide/img/popover-placement-below.png Binary files differnew file mode 100644 index 00000000000..2d6ab8a1618 --- /dev/null +++ b/doc/development/ux_guide/img/popover-placement-below.png diff --git a/doc/development/ux_guide/img/skeleton-loading.gif b/doc/development/ux_guide/img/skeleton-loading.gif Binary files differnew file mode 100644 index 00000000000..5877139171d --- /dev/null +++ b/doc/development/ux_guide/img/skeleton-loading.gif diff --git a/doc/development/ux_guide/index.md b/doc/development/ux_guide/index.md index 8a849f239dc..42bcf234e12 100644 --- a/doc/development/ux_guide/index.md +++ b/doc/development/ux_guide/index.md @@ -21,6 +21,11 @@ Guidance on the timing, curving and motion for GitLab. --- +### [Illustrations](illustrations.md) +Guidelines for principals and styles related to illustrations for GitLab. + +--- + ### [Copy](copy.md) Conventions on text and messaging within labels, buttons, and other components. diff --git a/doc/development/ux_guide/users.md b/doc/development/ux_guide/users.md index cbd7c17de41..fce882a45f1 100644 --- a/doc/development/ux_guide/users.md +++ b/doc/development/ux_guide/users.md @@ -1,4 +1,5 @@ -## UX Personas +# UX Personas + * [Nazim Ramesh](#nazim-ramesh) - Small to medium size organisations using GitLab CE * [James Mackey](#james-mackey) @@ -7,16 +8,16 @@ * [Karolina Plaskaty](#karolina-plaskaty) - Using GitLab.com for personal/hobby projects - Would like to use GitLab at work - - Working for a medium to large size organisation + - Working for a medium to large size organisation -<hr> +--- -### Nazim Ramesh +## Nazim Ramesh - Small to medium size organisations using GitLab CE <img src="img/nazim-ramesh.png" width="300px"> -#### Demographics +### Demographics - **Age**<br>32 years old - **Location**<br>Germany @@ -26,7 +27,7 @@ - **Frequently used programming languages**<br>JavaScript, SQL, PHP - **Hobbies / interests**<br>Functional programming, open source, gaming, web development and web security. -#### Motivations +### Motivations Nazim works for a software development company which currently hires around 80 people. When Nazim first joined the company, the engineering team were using Subversion (SVN) as their primary form of source control. However, Nazim felt SVN was not flexible enough to work with many feature branches and noticed that developers with less experience of source control struggled with the central-repository nature of SVN. Armed with a wishlist of features, Nazim began comparing source control tools. A search for “self-hosted Git server repository management” returned GitLab. In his own words, Nazim explains why he wanted the engineering team to start using GitLab: > @@ -39,48 +40,48 @@ In his role as a full-stack web developer, Nazim could recommend products that h “The biggest challenge...why should we change anything at all from the status quo? We needed to switch from SVN to Git. They knew they needed to learn Git and a Git workflow...using Git was scary to my colleagues...they thought it was more complex than SVN to use.” > -Undeterred, Nazim decided to migrate a couple of projects across to GitLab. +Undeterred, Nazim decided to migrate a couple of projects across to GitLab. > “Old SVN users couldn’t see the benefits of Git at first. It took a month or two to convince them.” > -Slowly, by showing his colleagues how easy it was to use Git, the majority of the team’s projects were migrated to GitLab. +Slowly, by showing his colleagues how easy it was to use Git, the majority of the team’s projects were migrated to GitLab. -The engineering team have been using GitLab CE for around 2 years now. Nazim credits himself as being entirely responsible for his company’s decision to move to GitLab. +The engineering team have been using GitLab CE for around 2 years now. Nazim credits himself as being entirely responsible for his company’s decision to move to GitLab. -#### Frustrations -##### Adoption to GitLab has been slow +### Frustrations +#### Adoption to GitLab has been slow Not only has the engineering team had to get to grips with Git, they’ve also had to adapt to using GitLab. Due to lack of training and existing skills in other tools, the full feature set of GitLab CE is not being utilised. Nazim sold GitLab to his manager as an ‘all in one’ tool which would replace multiple tools used within the company, thus saving costs. Nazim hasn’t had the time to integrate the legacy tools to GitLab and he’s struggling to convince his peers to change their habits. -##### Missing Features +#### Missing Features Nazim’s company want GitLab to be able to do everything. There isn’t a large budget for software, so they’re selective about what tools are implemented. It needs to add real value to the company. In order for GitLab to be widely adopted and to meet the requirements of different roles within the company, it needs a host of features. When an individual within Nazim’s company wants to know if GitLab has a specific feature or does a particular thing, Nazim is the person to ask. He becomes the point of contact to investigate, build or sometimes just raise the feature request. Nazim gets frustrated when GitLab isn’t able to do what he or his colleagues need it to do. -##### Regressions and bugs +#### Regressions and bugs Nazim often has to calm down his colleagues, when a release contains regressions or new bugs. As he puts it “every new version adds something awesome, but breaks something”. He feels that “old issues for "minor" annoyances get quickly buried in the mass of open issues and linger for a very long time. More generally, I have the feeling that GitLab focus on adding new functionalities, but overlook a bunch of annoying minor regressions or introduced bugs.” Due to limited resource and expertise within the team, not only is it difficult to remain up-to-date with the frequent release cycle, it’s also counterproductive to fix workflows every month. -##### Uses too much RAM and CPU +#### Uses too much RAM and CPU > “Memory usages mean that if we host it from a cloud based host like AWS, we spend almost as much on the instance as what we would pay GitHub” > -##### UI/UX +#### UI/UX GitLab’s interface initially attracted Nazim when he was comparing version control software. He thought it would help his less technical colleagues to adapt to using Git and perhaps, GitLab could be rolled out to other areas of the business, beyond engineering. However, using GitLab’s interface daily has left him frustrated at the lack of personalisation / control over his user experience. He’s also regularly lost in a maze of navigation. Whilst he acknowledges that GitLab listens to its users and that the interface is improving, he becomes annoyed when the changes are too progressive. “Too frequent UI changes. Most of them tend to turn out great after a few cycles of fixes, but the frequency is still far too high for me to feel comfortable to always stay on the current release.” -#### Goals +### Goals * To convince his colleagues to fully adopt GitLab CE, thus improving workflow and collaboration. * To use a feature rich version control platform that covers all stages of the development lifecycle, in order to reduce dependencies on other tools. * To use an intuitive and stable product, so he can spend more time on his core job responsibilities and less time bug-fixing, guiding colleagues, etc. -<hr> +--- -### James Mackey +## James Mackey - Medium to large size organisations using CE or EE - Small organisations using EE <img src="img/james-mackey.png" width="300px"> -#### Demographics +### Demographics - **Age**<br>36 years old - **Location**<br>US @@ -90,7 +91,7 @@ GitLab’s interface initially attracted Nazim when he was comparing version con - **Frequently used programming languages**<br>JavaScript, SQL, Node.js, Java, PHP, Python - **Hobbies / interests**<br>DevOps, open source, web development, science, automation and electronics. -#### Motivations +### Motivations James works for a research company which currently hires around 800 staff. He began using GitLab.com back in 2013 for his own open source, hobby projects and loved “the simplicity of installation, administration and use”. After using GitLab for over a year, he began to wonder about using it at work. James explains: > @@ -99,7 +100,7 @@ James works for a research company which currently hires around 800 staff. He be James and his colleagues also reviewed competitor products including GitHub Enterprise, but they found it “less innovative and with considerable costs...GitLab had the features we wanted at a much lower cost per head than GitHub”. -The company James works for provides employees with a discretionary budget to spend how they want on software, so James and his team decided to upgrade to EE. +The company James works for provides employees with a discretionary budget to spend how they want on software, so James and his team decided to upgrade to EE. James feels partially responsible for his organisation’s decision to start using GitLab. @@ -107,33 +108,33 @@ James feels partially responsible for his organisation’s decision to start usi “It's still up to the teams themselves [to decide] which tools to use. We just had a great experience moving our daily development to GitLab, so other teams have followed the path or are thinking about switching.” > -#### Frustrations -##### Third Party Integration +### Frustrations +#### Third Party Integration Some of GitLab EE’s features are too basic, in particular, issues boards which do not have the level of reporting that James and his team need. Subsequently, they still need to use GitLab EE in conjunction with other tools, such as JIRA. Whilst James feels it isn’t essential for GitLab to meet all his needs (his company are happy for him to use, and pay for, multiple tools), he sometimes isn’t sure what is/isn’t possible with plugins and what level of custom development he and his team will need to do. -##### UX/UI +#### UX/UI James and his team use CI quite heavily for several projects. Whilst they’ve welcomed improvements to the builds and pipelines interface, they still have some difficulty following build process on the different tabs under Pipelines. Some confusion has arisen from not knowing where to find different pieces of information or how to get to the next stages logs from the current stage’s log output screen. They feel more intuitive linking and flow may alleviate the problem. Generally, they feel GitLab’s navigation needs to reviewed and optimised. -##### Permissions +#### Permissions > “There is no granular control over user or group permissions. The permissions for a project are too tightly coupled to the permissions for Gitlab CI/build pipelines.” > -#### Goals +### Goals * To be able to integrate third party tools easily with GitLab EE and to create custom integrations and patches where needed. * To use GitLab EE primarily for code hosting, merge requests, continuous integration and issue management. James and his team want to be able to understand and use these particular features easily. * To able to share one instance of GitLab EE with multiple teams across the business. Advanced user management, the ability to separate permissions on different parts of the source code, etc are important to James. -<hr> +--- -### Karolina Plaskaty +## Karolina Plaskaty - Using GitLab.com for personal/hobby projects - Would like to use GitLab at work -- Working for a medium to large size organisation +- Working for a medium to large size organisation <img src="img/karolina-plaskaty.png" width="300px"> -#### Demographics +### Demographics - **Age**<br>26 years old - **Location**<br>UK @@ -143,22 +144,22 @@ James and his team use CI quite heavily for several projects. Whilst they’ve w - **Frequently used programming languages**<br>JavaScript and SQL - **Hobbies / interests**<br>Web development, mobile development, UX, open source, gaming and travel. -#### Motivations +### Motivations Karolina has been using GitLab.com for around a year. She roughly spends 8 hours every week programming, of that, 2 hours is spent contributing to open source projects. Karolina contributes to open source projects to gain programming experience and to give back to the community. She likes GitLab.com for its free private repositories and range of features which provide her with everything she needs for her personal projects. Karolina is also a massive fan of GitLab’s values and the fact that it isn’t a “behemoth of a company”. She explains that “displaying every single thing (doc, culture, assumptions, development...) in the open gives me greater confidence to choose Gitlab personally and to recommend it at work.” She’s also an avid reader of GitLab’s blog. Karolina works for a software development company which currently hires around 500 people. Karolina would love to use GitLab at work but the company has used GitHub Enterprise for a number of years. She describes management at her company as “old fashioned” and explains that it’s “less of a technical issue and more of a cultural issue” to convince upper management to move to GitLab. Karolina is also relatively new to the company so she’s apprehensive about pushing too hard to change version control platforms. -#### Frustrations -##### Unable to use GitLab at work +### Frustrations +#### Unable to use GitLab at work Karolina wants to use GitLab at work but isn’t sure how to approach the subject with management. In her current role, she doesn’t feel that she has the authority to request GitLab. -##### Performance +#### Performance GitLab.com is frequently slow and unavailable. Karolina has also heard that GitLab is a “memory hog” which has deterred her from running GitLab on her own machine for just hobby / personal projects. -##### UX/UI +#### UX/UI Karolina has an interest in UX and therefore has strong opinions about how GitLab should look and feel. She feels the interface is cluttered, “it has too many links/buttons” and the navigation “feels a bit weird sometimes. I get lost if I don’t pay attention.” As Karolina also enjoys contributing to open-source projects, it’s important to her that GitLab is well designed for public repositories, she doesn’t feel that GitLab currently achieves this. -#### Goals +### Goals * To develop her programming experience and to learn from other developers. * To contribute to both her own and other open source projects. -* To use a fast and intuitive version control platform.
\ No newline at end of file +* To use a fast and intuitive version control platform. diff --git a/doc/development/verifying_database_capabilities.md b/doc/development/verifying_database_capabilities.md index cc6d62957e3..ffdeff47d4a 100644 --- a/doc/development/verifying_database_capabilities.md +++ b/doc/development/verifying_database_capabilities.md @@ -24,3 +24,15 @@ else run_query end ``` + +# Read-only database + +The database can be used in read-only mode. In this case we have to +make sure all GET requests don't attempt any write operations to the +database. If one of those requests wants to write to the database, it needs +to be wrapped in a `Gitlab::Database.read_only?` or `Gitlab::Database.read_write?` +guard, to make sure it doesn't for read-only databases. + +We have a Rails Middleware that filters any potentially writing +operations (the CUD operations of CRUD) and prevent the user from trying +to update the database and getting a 500 error (see `Gitlab::Middleware::ReadOnly`). diff --git a/doc/development/writing_documentation.md b/doc/development/writing_documentation.md index eac9ec2a470..68ba3dd2da3 100644 --- a/doc/development/writing_documentation.md +++ b/doc/development/writing_documentation.md @@ -1,6 +1,6 @@ # Writing documentation - - **General Documentation**: written by the developers responsible by creating features. Should be submitted in the same merge request containing code. Feature proposals (by GitLab contributors) should also be accompanied by its respective documentation. They can be later improved by PMs and Technical Writers. + - **General Documentation**: written by the [developers responsible by creating features](#contributing-to-docs). Should be submitted in the same merge request containing code. Feature proposals (by GitLab contributors) should also be accompanied by its respective documentation. They can be later improved by PMs and Technical Writers. - **Technical Articles**: written by any [GitLab Team](https://about.gitlab.com/team/) member, GitLab contributors, or [Community Writers](https://about.gitlab.com/handbook/product/technical-writing/community-writers/). - **Indexes per topic**: initially prepared by the Technical Writing Team, and kept up-to-date by developers and PMs in the same merge request containing code. They gather all resources for that topic in a single page (user and admin documentation, articles, and third-party docs). @@ -69,6 +69,51 @@ Use the [writing method](https://about.gitlab.com/handbook/product/technical-wri All the docs follow the same [styleguide](doc_styleguide.md). +### Contributing to docs + +Whenever a feature is changed, updated, introduced, or deprecated, the merge +request introducing these changes must be accompanied by the documentation +(either updating existing ones or creating new ones). This is also valid when +changes are introduced to the UI. + +The one resposible for writing the first piece of documentation is the developer who +wrote the code. It's the job of the Product Manager to ensure all features are +shipped with its docs, whether is a small or big change. At the pace GitLab evolves, +this is the only way to keep the docs up-to-date. If you have any questions about it, +please ask a Technical Writer. Otherwise, when your content is ready, assign one of +them to review it for you. + +We use the [monthly release blog post](https://about.gitlab.com/handbook/marketing/blog/release-posts/#monthly-releases) as a changelog checklist to ensure everything +is documented. + +### Feature overview and use cases + +Every major feature (regardless if present in GitLab Community or Enterprise editions) +should present, at the beginning of the document, two main sections: **overview** and +**use cases**. Every GitLab EE-only feature should also contain these sections. + +**Overview**: at the name suggests, the goal here is to provide an overview of the feature. +Describe what is it, what it does, why it is important/cool/nice-to-have, +what problem it solves, and what you can do with this feature that you couldn't +do before. + +**Use cases**: provide at least two, ideally three, use cases for every major feature. +You should answer this question: what can you do with this feature/change? Use cases +are examples of how this feauture or change can be used in real life. + +Examples: +- CE and EE: [Issues](../user/project/issues/index.md#use-cases) +- CE and EE: [Merge Requests](../user/project/merge_requests/index.md#overview) +- EE-only: [Geo](https://docs.gitlab.com/ee/gitlab-geo/README.html#overview) +- EE-only: [Jenkins integration](https://docs.gitlab.com/ee/integration/jenkins.md#overview) + +Note that if you don't have anything to add between the doc title (`<h1>`) and +the header `## Overview`, you can omit the header, but keep the content of the +overview there. + +> **Overview** and **use cases** are required to **every** Enterprise Edition feature, +and for every **major** feature present in Community Edition. + ### Markdown Currently GitLab docs use Redcarpet as [markdown](../user/markdown.md) engine, but there's an [open discussion](https://gitlab.com/gitlab-com/gitlab-docs/issues/50) for implementing Kramdown in the near future. @@ -103,3 +148,87 @@ If that job fails, read the instructions in the job log for what to do next. Contributors do not need to submit their changes to EE, GitLab Inc. employees on the other hand need to make sure that their changes apply cleanly to both CE and EE. + +## Previewing the changes live + +If you want to preview the doc changes of your merge request live, you can use +the manual `review-docs-deploy` job in your merge request. + +TIP: **Tip:** +If your branch contains only documentation changes, you can use +[special branch names](#testing) to avoid long running pipelines. + + + +This job will: + +1. Create a new branch in the [gitlab-docs](https://gitlab.com/gitlab-com/gitlab-docs) + project named after the scheme: `preview-<branch-slug>` +1. Trigger a cross project pipeline and build the docs site with your changes + +After a few minutes, the Review App will be deployed and you will be able to +preview the changes. The docs URL can be found in two places: + +- In the merge request widget +- In the output of the `review-docs-deploy` job, which also includes the + triggered pipeline so that you can investigate whether something went wrong + +In case the Review App URL returns 404, follow these steps to debug: + +1. **Did you follow the URL from the merge request widget?** If yes, then check if + the link is the same as the one in the job output. It can happen that if the + branch name slug is longer than 35 characters, it is automatically + truncated. That means that the merge request widget will not show the proper + URL due to a limitation of how `environment: url` works, but you can find the + real URL from the output of the `review-docs-deploy` job. +1. **Did you follow the URL from the job output?** If yes, then it means that + either the site is not yet deployed or something went wrong with the remote + pipeline. Give it a few minutes and it should appear online, otherwise you + can check the status of the remote pipeline from the link in the job output. + If the pipeline failed or got stuck, drop a line in the `#docs` chat channel. + +TIP: **Tip:** +Someone that has no merge rights to the CE/EE projects (think of forks from +contributors) will not be able to run the manual job. In that case, you can +ask someone from the GitLab team who has the permissions to do that for you. + +NOTE: **Note:** +Make sure that you always delete the branch of the merge request you were +working on. If you don't, the remote docs branch won't be removed either, +and the server where the Review Apps are hosted will eventually be out of +disk space. + +### Behind the scenes + +If you want to know the hot details, here's what's really happening: + +1. You manually run the `review-docs-deploy` job in a CE/EE merge request. +1. The job runs the [`scirpts/trigger-build-docs`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/scripts/trigger-build-docs) + script with the `deploy` flag, which in turn: + 1. Takes your branch name and applies the following: + - The slug of the branch name is used to avoid special characters since + ultimately this will be used by NGINX. + - The `preview-` prefix is added to avoid conflicts if there's a remote branch + with the same name that you created in the merge request. + - The final branch name is truncated to 42 characters to avoid filesystem + limitations with long branch names (> 63 chars). + 1. The remote branch is then created if it doesn't exist (meaning you can + re-run the manual job as many times as you want and this step will be skipped). + 1. A new cross-project pipeline is triggered in the docs project. + 1. The preview URL is shown both at the job output and in the merge request + widget. You also get the link to the remote pipeline. +1. In the docs project, the pipeline is created and it + [skips the test jobs](https://gitlab.com/gitlab-com/gitlab-docs/blob/8d5d5c750c602a835614b02f9db42ead1c4b2f5e/.gitlab-ci.yml#L50-55) + to lower the build time. +1. Once the docs site is built, the HTML files are uploaded as artifacts. +1. A specific Runner tied only to the docs project, runs the Review App job + that downloads the artifacts and uses `rsync` to transfer the files over + to a location where NGINX serves them. + +The following GitLab features are used among others: + +- [Manual actions](../ci/yaml/README.md#manual-actions) +- [Multi project pipelines](https://docs.gitlab.com/ee/ci/multi_project_pipeline_graphs.html) +- [Review Apps](../ci/review_apps/index.md) +- [Artifacts](../ci/yaml/README.md#artifacts) +- [Specific Runner](../ci/runners/README.md#locking-a-specific-runner-from-being-enabled-for-other-projects) |
