diff options
Diffstat (limited to 'doc/development')
| -rw-r--r-- | doc/development/code_review.md | 128 | ||||
| -rw-r--r-- | doc/development/contributing/index.md | 86 | ||||
| -rw-r--r-- | doc/development/contributing/issue_workflow.md | 66 | ||||
| -rw-r--r-- | doc/development/contributing/merge_request_workflow.md | 9 | ||||
| -rw-r--r-- | doc/development/documentation/index.md | 22 | ||||
| -rw-r--r-- | doc/development/documentation/structure.md | 46 | ||||
| -rw-r--r-- | doc/development/i18n/proofreader.md | 4 | ||||
| -rw-r--r-- | doc/development/testing_guide/best_practices.md | 155 |
8 files changed, 369 insertions, 147 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 4bbcdc6329f..3fe79943fdc 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -1,55 +1,103 @@ # Code Review Guidelines -## Getting your merge request reviewed, approved, and merged +This guide contains advice and best practices for performing code review, and +having your code reviewed. + +All merge requests for GitLab CE and EE, whether written by a GitLab team member +or a volunteer contributor, must go through a code review process to ensure the +code is effective, understandable, and maintainable. -There are a few rules to get your merge request accepted: +## Getting your merge request reviewed, approved, and merged -1. Your merge request should only be **merged by a [maintainer][team]**. - 1. If your merge request includes only backend changes [^1], it must be - **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]**. +You are strongly encouraged to get your code **reviewed** by a +[reviewer](https://about.gitlab.com/handbook/engineering/#reviewer) as soon as +there is any code to review, to get a second opinion on the chosen solution and +implementation, and an extra pair of eyes looking for bugs, logic problems, or +uncovered edge cases. The reviewer can be from a different team, but it is +recommended to pick someone who knows the domain well. You can read more about the +importance of involving reviewer(s) in the section on the responsibility of the author below. + +If you need some guidance (e.g. it's your first merge request), feel free to ask +one of the [Merge request coaches][team]. + +Depending on the areas your merge request touches, it must be **approved** by one +or more [maintainers](https://about.gitlab.com/handbook/engineering/#maintainer): + + 1. If your merge request includes backend changes [^1], it must be + **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_backend)**. + 1. If your merge request includes frontend changes [^1], it must be + **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_frontend)**. + 1. If your merge request includes UX changes [^1], it must be + **approved by a [UX team member][team]**. 1. If your merge request includes adding a new JavaScript library [^1], it must be **approved by a [frontend lead][team]**. 1. If your merge request includes adding a new UI/UX paradigm [^1], it must be **approved by a [UX lead][team]**. - 1. If your merge request includes 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 [Distribution team member][team]*. See how to work with the [Distribution team for more details.](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) -1. To lower the amount of merge requests maintainers need to review, you can - ask or assign any [reviewers][projects] for a first review. - 1. If you need some guidance (e.g. it's your first merge request), feel free - to ask one of the [Merge request coaches][team]. - 1. It is recommended that you assign a maintainer that is from a different team than your own. - This ensures that all code across GitLab is consistent and can be easily understood by all contributors. - -1. Keep in mind that maintainers are also going to perform a final code review. - The ideal scenario is that the reviewer has already addressed any concerns - the maintainer would have found, and the maintainer only has to perform the - merge, but be prepared for further review comments. - -For more guidance, see [CONTRIBUTING.md](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md). + 1. If your merge request includes a new dependency or a filesystem change, it must be + **approved by a [Distribution team member][team]**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) for more details. -## Best practices +Getting your merge request **merged** also requires a maintainer. If it requires +more than one approval, the last maintainer to review and approve it will also merge it. -This guide contains advice and best practices for performing code review, and -having your code reviewed. +As described in the section on the responsibility of the maintainer below, you +are recommended to get your merge request approved and merged by maintainer(s) +from other teams than your own. -All merge requests for GitLab CE and EE, whether written by a GitLab team member -or a volunteer contributor, must go through a code review process to ensure the -code is effective, understandable, and maintainable. +### The responsibility of the merge request author -Any developer can, and is encouraged to, perform code review on merge requests -of colleagues and contributors. However, the final decision to accept a merge -request is up to one the project's maintainers, denoted on the -[engineering projects][projects]. +The responsibility to find the best solution and implement it lies with the +merge request author. + +Before assigning a merge request to a maintainer for approval and merge, they +should be confident that it actually solves the problem it was meant to solve, +that it does so in the most appropriate way, that it satisfies all requirements, +and that there are no remaining bugs, logical problems, or uncovered edge cases. +The merge request should also have a completed task list in its description and +a passing CI pipeline to avoid unnecessary back and forth. + +To reach the required level of confidence in their solution, an author is expected +to involve other people in the investigation and implementation processes as +appropriate. + +They are encouraged to reach out to domain experts to discuss different solutions +or get an implementation reviewed, to product managers and UX designers to clear +up confusion or verify that the end result matches what they had in mind, to +database specialists to get input on the data model or specific queries, or to +any other developer to get an in-depth review of the solution. + +If an author is unsure if a merge request needs a domain expert's opinion, that's +usually a pretty good sign that it does, since without it the required level of +confidence in their solution will not have been reached. + +### The responsibility of the maintainer + +Maintainers are responsible for the overall health, quality, and consistency of +the GitLab codebase, across domains and product areas. + +Consequently, their reviews will focus primarily on things like overall +architecture, code organization, separation of concerns, tests, DRYness, +consistency, and readability. + +Since a maintainer's job only depends on their knowledge of the overall GitLab +codebase, and not that of any specific domain, they can review, approve and merge +merge requests from any team and in any product area. + +In fact, authors are recommended to get their merge requests merged by maintainers +from other teams than their own, to ensure that all code across GitLab is consistent +and can be easily understood by all contributors, from both inside and outside the +company, without requiring team-specific expertise. + +Maintainers will do their best to also review the specifics of the chosen solution +before merging, but as they are not necessarily domain experts, they may be poorly +placed to do so without an unreasonable investment of time. In those cases, they +will defer to the judgment of the author and earlier reviewers and involved domain +experts, in favor of focusing on their primary responsibilities. + +If a developer who happens to also be a maintainer was involved in a merge request +as a domain expert and/or reviewer, it is recommended that they are not also picked +as the maintainer to ultimately approve and merge it. + +## Best practices ### Everyone diff --git a/doc/development/contributing/index.md b/doc/development/contributing/index.md index 29af8dcb9bb..9da4c66933c 100644 --- a/doc/development/contributing/index.md +++ b/doc/development/contributing/index.md @@ -3,10 +3,12 @@ Thank you for your interest in contributing to GitLab. This guide details how to contribute to GitLab in a way that is easy for everyone. +We want to create a welcoming environment for everyone who is interested in contributing. Please visit our [Code of Conduct page](https://about.gitlab.com/contributing/code-of-conduct) to learn more about our committment to an open and welcoming environment. + For a first-time step-by-step guide to the contribution process, please see ["Contributing to GitLab"](https://about.gitlab.com/contributing/). -Looking for something to work on? Look for issues in the [Backlog (Accepting merge requests) milestone](#i-want-to-contribute). +Looking for something to work on? Look for issues with the label [`Accepting merge requests`](#i-want-to-contribute). GitLab comes in two flavors, GitLab Community Edition (CE) our free and open source edition, and GitLab Enterprise Edition (EE) which is our commercial @@ -30,77 +32,8 @@ vulnerabilities. ## Code of conduct -### Our Pledge - -In the interest of fostering an open and welcoming environment, we as -contributors and maintainers pledge to making participation in our project and -our community a harassment-free experience for everyone, regardless of age, body -size, disability, ethnicity, sex characteristics, gender identity and expression, -level of experience, education, socio-economic status, nationality, personal -appearance, race, religion, or sexual identity and orientation. - -### Our Standards - -Examples of behavior that contributes to creating a positive environment -include: - -* Using welcoming and inclusive language -* Being respectful of differing viewpoints and experiences -* Gracefully accepting constructive criticism -* Focusing on what is best for the community -* Showing empathy towards other community members - -Examples of unacceptable behavior by participants include: - -* The use of sexualized language or imagery and unwelcome sexual attention or - advances -* Trolling, insulting/derogatory comments, and personal or political attacks -* Public or private harassment -* Publishing others' private information, such as a physical or electronic - address, without explicit permission -* Other conduct which could reasonably be considered inappropriate in a - professional setting - -### Our Responsibilities - -Project maintainers are responsible for clarifying the standards of acceptable -behavior and are expected to take appropriate and fair corrective action in -response to any instances of unacceptable behavior. - -Project maintainers have the right and responsibility to remove, edit, or -reject comments, commits, code, wiki edits, issues, and other contributions -that are not aligned to this Code of Conduct, or to ban temporarily or -permanently any contributor for other behaviors that they deem inappropriate, -threatening, offensive, or harmful. - -### Scope - -This Code of Conduct applies both within project spaces and in public spaces -when an individual is representing the project or its community. Examples of -representing a project or community include using an official project e-mail -address, posting via an official social media account, or acting as an appointed -representative at an online or offline event. Representation of a project may be -further defined and clarified by project maintainers. - -### Enforcement - -Instances of abusive, harassing, or otherwise unacceptable behavior may be -reported by contacting the project team at conduct@gitlab.com. All -complaints will be reviewed and investigated and will result in a response that -is deemed necessary and appropriate to the circumstances. The project team is -obligated to maintain confidentiality with regard to the reporter of an incident. -Further details of specific enforcement policies may be posted separately. - -Project maintainers who do not follow or enforce the Code of Conduct in good -faith may face temporary or permanent repercussions as determined by other -members of the project's leadership. - -### Attribution - -This Code of Conduct is adapted from the [Contributor Covenant][homepage], version 1.4, -available at https://www.contributor-covenant.org/version/1/4/code-of-conduct.html - -[homepage]: https://www.contributor-covenant.org +Our code of conduct can be found on the +["Contributing to GitLab"](https://about.gitlab.com/contributing/) page. ## Closing policy for issues and merge requests @@ -133,10 +66,10 @@ the remaining issues on the GitHub issue tracker. ## I want to contribute! -If you want to contribute to GitLab, [issues in the `Backlog (Accepting merge requests)` milestone][accepting-mrs-weight] -are a great place to start. Issues with a lower weight (1 or 2) are deemed -suitable for beginners. These issues will be of reasonable size and challenge, -for anyone to start contributing to GitLab. If you have any questions or need help visit [Getting Help](https://about.gitlab.com/getting-help/#discussion) to +If you want to contribute to GitLab, +[issues with the `Accepting merge requests` label](issue_workflow.md#label-for-community-contributors) +are a great place to start. +If you have any questions or need help visit [Getting Help](https://about.gitlab.com/getting-help/#discussion) to learn how to communicate with GitLab. If you're looking for a Gitter or Slack channel please consider we favor [asynchronous communication](https://about.gitlab.com/handbook/communication/#internal-communication) over real time communication. Thanks for your contribution! @@ -198,4 +131,3 @@ This [documentation](style_guides.md) outlines the current style guidelines. [team]: https://about.gitlab.com/team/ [getting-help]: https://about.gitlab.com/getting-help/ [codetriage]: http://www.codetriage.com/gitlabhq/gitlabhq -[accepting-mrs-weight]: https://gitlab.com/gitlab-org/gitlab-ce/issues?scope=all&utf8=✓&state=opened&assignee_id=0&milestone_title=Backlog%20(Accepting%20merge%20requests) diff --git a/doc/development/contributing/issue_workflow.md b/doc/development/contributing/issue_workflow.md index cd5eee6ea36..4661d11b29e 100644 --- a/doc/development/contributing/issue_workflow.md +++ b/doc/development/contributing/issue_workflow.md @@ -148,6 +148,9 @@ This label documents the planned timeline & urgency which is used to measure aga | ~P3 | Medium Priority | Within the next 3 releases (approx one quarter or 90 days) | | ~P4 | Low Priority | Anything outside the next 3 releases (more than one quarter or 120 days) | +If an issue seems to fall between two priority labels, assign it to the higher- +priority label. + ## Severity labels Severity labels help us clearly communicate the impact of a ~bug on users. @@ -159,6 +162,10 @@ Severity labels help us clearly communicate the impact of a ~bug on users. | ~S3 | Major Severity | Broken Feature, workaround acceptable | Can create merge requests only from the Merge Requests page, not through the Issue. | | ~S4 | Low Severity | Functionality inconvenience or cosmetic issue | Label colors are incorrect / not being displayed. | +If an issue seems to fall between two severity labels, even taking the +[severity impact guidance](#severity-impact-guidance) into account, assign +it to the higher-severity label. + ### Severity impact guidance Severity levels can be applied further depending on the facet of the impact; e.g. Affected customers, GitLab.com availability, performance and etc. The below is a guideline. @@ -174,10 +181,10 @@ Severity levels can be applied further depending on the facet of the impact; e.g Issues that are beneficial to our users, 'nice to haves', that we currently do not have the capacity for or want to give the priority to, are labeled as -~"Accepting Merge Requests", so the community can make a contribution. +~"Accepting merge requests", so the community can make a contribution. Community contributors can submit merge requests for any issue they want, but -the ~"Accepting Merge Requests" label has a special meaning. It points to +the ~"Accepting merge requests" label has a special meaning. It points to changes that: 1. We already agreed on, @@ -185,26 +192,26 @@ changes that: 1. Are likely to get accepted by a maintainer. We want to avoid a situation when a contributor picks an -~"Accepting Merge Requests" issue and then their merge request gets closed, +~"Accepting merge requests" issue and then their merge request gets closed, because we realize that it does not fit our vision, or we want to solve it in a different way. -We add the ~"Accepting Merge Requests" label to: +We add the ~"Accepting merge requests" label to: - Low priority ~bug issues (i.e. we do not add it to the bugs that we want to solve in the ~"Next Patch Release") - Small ~"feature proposal" - Small ~"technical debt" issues -After adding the ~"Accepting Merge Requests" label, we try to estimate the +After adding the ~"Accepting merge requests" label, we try to estimate the [weight](#issue-weight) of the issue. We use issue weight to let contributors know how difficult the issue is. Additionally: -- We advertise ["Accepting Merge Requests" issues with weight < 5][up-for-grabs] +- We advertise [`Accepting merge requests` issues with weight < 5][up-for-grabs] as suitable for people that have never contributed to GitLab before on the [Up For Grabs campaign](http://up-for-grabs.net) - We encourage people that have never contributed to any open source project to - look for ["Accepting Merge Requests" issues with a weight of 1][firt-timers] + look for [`Accepting merge requests` issues with a weight of 1][firt-timers] If you've decided that you would like to work on an issue, please @-mention the [appropriate product manager](https://about.gitlab.com/handbook/product/#who-to-talk-to-for-what) @@ -213,12 +220,12 @@ members to further discuss scope, design, and technical considerations. This wil ensure that your contribution is aligned with the GitLab product and minimize any rework and delay in getting it merged into master. -GitLab team members who apply the ~"Accepting Merge Requests" label to an issue +GitLab team members who apply the ~"Accepting merge requests" label to an issue should update the issue description with a responsible product manager, inviting any potential community contributor to @-mention per above. -[up-for-grabs]: https://gitlab.com/gitlab-org/gitlab-ce/issues?label_name=Accepting+Merge+Requests&scope=all&sort=weight_asc&state=opened -[firt-timers]: https://gitlab.com/gitlab-org/gitlab-ce/issues?label_name%5B%5D=Accepting+Merge+Requests&scope=all&sort=upvotes_desc&state=opened&weight=1 +[up-for-grabs]: https://gitlab.com/groups/gitlab-org/-/issues?state=opened&label_name[]=Accepting+merge+requests&assignee_id=0&sort=weight +[firt-timers]: https://gitlab.com/groups/gitlab-org/-/issues?state=opened&label_name[]=Accepting+merge+requests&assignee_id=0&sort=weight&weight=1 ## Issue triaging @@ -349,6 +356,45 @@ for a release by the appropriate person. Make sure to mention the merge request that the ~"technical debt" issue or ~"UX debt" issue is associated with in the description of the issue. +## Technical debt in follow-up issues + +It's common to discover technical debt during development of a new feature. In +the spirit of "minimum viable change", resolution is often deferred to a +follow-up issue. However, this cannot be used as an excuse to merge poor-quality +code that would otherwise not pass review, or to overlook trivial matters that +don't deserve the be scheduled independently, and would be best resolved in the +original merge request - or not tracked at all! + +The overheads of scheduling, and rate of change in the GitLab codebase, mean +that the cost of a trivial technical debt issue can quickly exceed the value of +tracking it. This generally means we should resolve these in the original merge +request - or simply not create a follow-up issue at all. + +For example, a typo in a comment that is being copied between files is worth +fixing in the same MR, but not worth creating a follow-up issue for. Renaming a +method that is used in many places to make its intent slightly clearer may be +worth fixing, but it should not happen in the same MR, and is generally not +worth the overhead of having an issue of its own. These issues would invariably +be labelled `~P4 ~S4` if we were to create them. + +More severe technical debt can have implications for development velocity. If +it isn't addressed in a timely manner, the codebase becomes needlessly difficult +to change, new features become difficult to add, and regressions abound. + +Discoveries of this kind of technical debt should be treated seriously, and +while resolution in a follow-up issue may be appropriate, maintainers should +generally obtain a scheduling commitment from the author of the original MR, or +the engineering or product manager for the relevant area. This may take the form +of appropriate Priority / Severity labels on the issue, or an explicit milestone +and assignee. + +The maintainer must always agree before an outstanding discussion is resolved in +this manner, and will be the one to create the issue. The title and description +should be of the same quality as those created +[in the usual manner](#technical-and-ux-debt) - in particular, the issue title +**must not** begin with `Follow-up`! The creating maintainer should also expect +to be involved in some capacity when work begins on the follow-up issue. + ## Stewardship For issues related to the open source stewardship of GitLab, diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index cc7d8a1e1db..1764e2d8b21 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -2,10 +2,9 @@ We welcome merge requests with fixes and improvements to GitLab code, tests, and/or documentation. The issues that are specifically suitable for -community contributions are listed with the -[`Backlog (Accepting merge requests)` milestone in the CE issue tracker][accepting-mrs-ce] -and [EE issue tracker][accepting-mrs-ee], but you are free to contribute to any other issue -you want. +community contributions are listed with +[the `Accepting merge requests` label](issue_workflow.md#label-for-community-contributors), +but you are free to contribute to any other issue you want. Please note that if an issue is marked for the current milestone either before or while you are working on it, a team member may take over the merge request @@ -25,8 +24,6 @@ some potentially easy issues. To start with GitLab development download the [GitLab Development Kit][gdk] and see the [Development section](../../README.md) for some guidelines. -[accepting-mrs-ce]: https://gitlab.com/gitlab-org/gitlab-ce/issues?milestone_title=Backlog%20(Accepting%20merge%20requests) -[accepting-mrs-ee]: https://gitlab.com/gitlab-org/gitlab-ee/issues?milestone_title=Backlog%20(Accepting%20merge%20requests) [gitlab-mr-tracker]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests [gdk]: https://gitlab.com/gitlab-org/gitlab-development-kit diff --git a/doc/development/documentation/index.md b/doc/development/documentation/index.md index 1dcdf788a3e..51f5ddfc1e0 100644 --- a/doc/development/documentation/index.md +++ b/doc/development/documentation/index.md @@ -451,16 +451,6 @@ preview the changes. The docs URL can be found in two places: - 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. -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 @@ -472,6 +462,18 @@ 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. +### Troubleshooting review apps + +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. +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. + ### Technical aspects If you want to know the hot details, here's what's really happening: diff --git a/doc/development/documentation/structure.md b/doc/development/documentation/structure.md index 01068e23082..607ad21d459 100644 --- a/doc/development/documentation/structure.md +++ b/doc/development/documentation/structure.md @@ -142,9 +142,49 @@ fancy words. The docs should be clear and easy to understand. <!-- ## Troubleshooting Add a troubleshooting guide when possible/applicable. --> -``` + +--- Notes: -- (1): Apply the [tier badges](styleguide.md#product-badges) accordingly -- (2): Apply the correct format for the [GitLab version introducing the feature](styleguide.md#gitlab-versions-and-tiers) +- (1): Apply the [tier badges](https://docs.gitlab.com/ee/development/documentation/styleguide.html#product-badges) accordingly +- (2): Apply the correct format for the [GitLab version introducing the feature](https://docs.gitlab.com/ee/development/documentation/styleguide.html#gitlab-versions-and-tiers) +``` + +## Help and feedback section + +The "help and feedback" section (introduced by [!319](https://gitlab.com/gitlab-com/gitlab-docs/merge_requests/319)) displayed at the end of each document +can be omitted from the doc by adding a key into the its frontmatter: + +```yaml +--- +feedback: false +--- +``` + +The default is to leave it there. If you want to omit it from a document, +you must check with a technical writer before doing so. + +### Disqus + +We also have integrated the docs site with Disqus (introduced by +[!151](https://gitlab.com/gitlab-com/gitlab-docs/merge_requests/151)), +allowing our users to post comments. + +To omit only the comments from the feedback section, use the following +key on the frontmatter: + +```yaml +--- +comments: false +--- +``` + +We are only hiding comments in main index pages, such as [the main documentation index](../../README.md), since its content is too broad to comment on. Before omitting Disqus, +you must check with a technical writer. + +Note that once `feedback: false` is added to the frontmatter, it will automatically omit +Disqus, therefore, don't add both keys to the same document. + +The click events in the feedback section are tracked with Google Tag Manager. The +conversions can be viewed on Google Analytics by navigating to **Behavior > Events > Top events > docs**. diff --git a/doc/development/i18n/proofreader.md b/doc/development/i18n/proofreader.md index 5e13c725e15..6a67aa7f610 100644 --- a/doc/development/i18n/proofreader.md +++ b/doc/development/i18n/proofreader.md @@ -74,6 +74,8 @@ are very appreciative of the work done by translators and proofreaders! have previously translated. 1. Your request to become a proofreader will be considered on the merits of - your previous translations. + your previous translations by [GitLab team members](https://about.gitlab.com/team/) + or [Core team members](https://about.gitlab.com/core-team/) who are fluent in + the language or current proofreaders. [proofreader-src]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/i18n/proofreader.md diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md index acbfa1850b4..7727bd74c3c 100644 --- a/doc/development/testing_guide/best_practices.md +++ b/doc/development/testing_guide/best_practices.md @@ -209,6 +209,130 @@ it 'is overdue' do end ``` +### Pristine test environments + +The code exercised by a single GitLab test may access and modify many items of +data. Without careful preparation before a test runs, and cleanup afterward, +data can be changed by a test in such a way that it affects the behaviour of +following tests. This should be avoided at all costs! Fortunately, the existing +test framework handles most cases already. + +When the test environment does get polluted, a common outcome is +[flaky tests](flaky_tests.md). Pollution will often manifest as an order +dependency: running spec A followed by spec B will reliably fail, but running +spec B followed by spec A will reliably succeed. In these cases, you can use +`rspec --bisect` (or a manual pairwise bisect of spec files) to determine which +spec is at fault. Fixing the problem requires some understanding of how the test +suite ensures the environment is pristine. Read on to discover more about each +data store! + +#### SQL database + +This is managed for us by the `database_cleaner` gem. Each spec is surrounded in +a transaction, which is rolled back once the test completes. Certain specs will +instead issue `DELETE FROM` queries against every table after completion; this +allows the created rows to be viewed from multiple database connections, which +is important for specs that run in a browser, or migration specs, among others. + +One consequence of using these strategies, instead of the well-known +`TRUNCATE TABLES` approach, is that primary keys and other sequences are **not** +reset across specs. So if you create a project in spec A, then create a project +in spec B, the first will have `id=1`, while the second will have `id=2`. + +This means that specs should **never** rely on the value of an ID, or any other +sequence-generated column. To avoid accidental conflicts, specs should also +avoid manually specifying any values in these kinds of columns. Instead, leave +them unspecified, and look up the value after the row is created. + +#### Redis + +GitLab stores two main categories of data in Redis: cached items, and sidekiq +jobs. + +In most specs, the Rails cache is actually an in-memory store. This is replaced +between specs, so calls to `Rails.cache.read` and `Rails.cache.write` are safe. +However, if a spec makes direct Redis calls, it should mark itself with the +`:clean_gitlab_redis_cache`, `:clean_gitlab_redis_shared_state` or +`:clean_gitlab_redis_queues` traits as appropriate. + +Sidekiq jobs are typically not run in specs, but this behaviour can be altered +in each spec through the use of `Sidekiq::Testing.inline!` blocks. Any spec that +causes Sidekiq jobs to be pushed to Redis should use the `:sidekiq` trait, to +ensure that they are removed once the spec completes. + +#### Filesystem + +Filesystem data can be roughly split into "repositories", and "everything else". +Repositories are stored in `tmp/tests/repositories`. This directory is emptied +before a test run starts, and after the test run ends. It is not emptied between +specs, so created repositories accumulate within this directory over the +lifetime of the process. Deleting them is expensive, but this could lead to +pollution unless carefully managed. + +To avoid this, [hashed storage](../../administration/repository_storage_types.md) +is enabled in the test suite. This means that repositories are given a unique +path that depends on their project's ID. Since the project IDs are not reset +between specs, this guarantees that each spec gets its own repository on disk, +and prevents changes from being visible between specs. + +If a spec manually specifies a project ID, or inspects the state of the +`tmp/tests/repositories/` directory directly, then it should clean up the +directory both before and after it runs. In general, these patterns should be +completely avoided. + +Other classes of file linked to database objects, such as uploads, are generally +managed in the same way. With hashed storage enabled in the specs, they are +written to disk in locations determined by ID, so conflicts should not occur. + +Some specs disable hashed storage by passing the `:legacy_storage` trait to the +`projects` factory. Specs that do this must **never** override the `path` of the +project, or any of its groups. The default path includes the project ID, so will +not conflict; but if two specs create a `:legacy_storage` project with the same +path, they will use the same repository on disk and lead to test environment +pollution. + +Other files must be managed manually by the spec. If you run code that creates a +`tmp/test-file.csv` file, for instance, the spec must ensure that the file is +removed as part of cleanup. + +#### Persistent in-memory application state + +All the specs in a given `rspec` run share the same Ruby process, which means +they can affect each other by modifying Ruby objects that are accessible between +specs. In practice, this means global variables, and constants (which includes +Ruby classes, modules, etc). + +Global variables should generally not be modified. If absolutely necessary, a +block like this can be used to ensure the change is rolled back afterwards: + +```ruby +around(:each) do |example| + old_value = $0 + + begin + $0 = "new-value" + example.run + ensure + $0 = old_value + end +end +``` + +If a spec needs to modify a constant, it should use the `stub_const` helper to +ensure the change is rolled back. + +If you need to modify the contents of the `ENV` constant, you can use the +`stub_env` helper method instead. + +While most Ruby **instances** are not shared between specs, **classes** +and **modules** generally are. Class and module instance variables, accessors, +class variables, and other stateful idioms, should be treated in the same way as +global variables - don't modify them unless you have to! In particular, prefer +using expectations, or dependency injection along with stubs, to avoid the need +for modifications. If you have no other choice, an `around` block similar to the +example for global variables, above, can be used, but this should be avoided if +at all possible. + ### Table-based / Parameterized tests This style of testing is used to exercise one piece of code with a comprehensive @@ -348,6 +472,37 @@ GitLab uses [factory_bot] as a test fixture replacement. All fixtures should be be placed under `spec/fixtures/`. +### Repositories + +Testing some functionality, e.g., merging a merge request, requires a git +repository with a certain state to be present in the test environment. GitLab +maintains the [gitlab-test](https://gitlab.com/gitlab-org/gitlab-test) +repository for certain common cases - you can ensure a copy of the repository is +used with the `:repository` trait for project factories: + +```ruby +let(:project) { create(:project, :repository) } +``` + +Where you can, consider using the `:custom_repo` trait instead of `:repository`. +This allows you to specify exactly what files will appear in the `master` branch +of the project's repository. For example: + +```ruby +let(:project) do + create( + :project, :custom_repo, + files: { + 'README.md' => 'Content here', + 'foo/bar/baz.txt' => 'More content here' + } + ) +end +``` + +This will create a repository containing two files, with default permissions and +the specified content. + ### Config RSpec config files are files that change the RSpec config (i.e. |