Commit 578d0370 authored by Angelo Gulina's avatar Angelo Gulina Committed by Mike Lewis

Apply suggestion from TW reviewer:

	- formatting following styleguide
	- move reference to proposal at 'introduced in' section
parent 11d6fa43
...@@ -27,7 +27,7 @@ be used. There is no need to add a title called "Introduction" or "Overview," be ...@@ -27,7 +27,7 @@ be used. There is no need to add a title called "Introduction" or "Overview," be
search for these terms. Just put this information after the title. search for these terms. Just put this information after the title.
- **Use cases**: describes real use case scenarios for that feature/configuration. - **Use cases**: describes real use case scenarios for that feature/configuration.
- **Requirements**: describes what software, configuration, account, or knowledge is required. - **Requirements**: describes what software, configuration, account, or knowledge is required.
- **Instructions**: One or more sets of detailed instructions to follow. - **Instructions**: one or more sets of detailed instructions to follow.
- **Troubleshooting** guide (recommended but not required). - **Troubleshooting** guide (recommended but not required).
For additional details on each, see the [template for new docs](#template-for-new-docs), For additional details on each, see the [template for new docs](#template-for-new-docs),
...@@ -167,3 +167,23 @@ Disqus, therefore, don't add both keys to the same document. ...@@ -167,3 +167,23 @@ 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 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**. conversions can be viewed on Google Analytics by navigating to **Behavior > Events > Top events > docs**.
## Guidelines for good practices
> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/36576/) in GitLab 13.2 as GitLab Development documentation.
'Good practice' examples demonstrate encouraged ways of writing code while comparing with examples of practices to avoid.
These examples are labeled as "Bad" or "Good".
In GitLab development guidelines, when presenting the cases, it is recommended
to follow a **first-bad-then-good** strategy. First demonstrate the "Bad" practice (how things _could_ be done, which is often still working code),
and then how things _should_ be done better, using a "Good" example. This is typically an improved example of the same code.
Consider the following guidelines when offering examples:
- First, offer the "Bad" example, then the "Good" one.
- When only one bad case and one good case is given, use the same code block.
- When more than one bad case or one good case is offered, use separated code blocks for each.
With many examples being presented, a clear separation helps the reader to go directly to the good part.
Consider offering an explanation (for example, a comment, a link to a resource, etc.) on why something is bad practice.
- Better and best cases can be considered part of the good case(s) code block.
In the same code block, precede each with comments: `# Better` and `# Best`.
...@@ -170,22 +170,14 @@ Some more examples can be found in the [Frontend unit tests section](testing_lev ...@@ -170,22 +170,14 @@ Some more examples can be found in the [Frontend unit tests section](testing_lev
Another common gotcha is that the specs end up verifying the mock is working. If you are using mocks, the mock should support the test, but not be the target of the test. Another common gotcha is that the specs end up verifying the mock is working. If you are using mocks, the mock should support the test, but not be the target of the test.
**Bad:**
```javascript ```javascript
const spy = jest.spyOn(idGenerator, 'create') const spy = jest.spyOn(idGenerator, 'create')
spy.mockImplementation = () = '1234' spy.mockImplementation = () = '1234'
// Bad
expect(idGenerator.create()).toBe('1234') expect(idGenerator.create()).toBe('1234')
```
**Good:**
```javascript
const spy = jest.spyOn(idGenerator, 'create')
spy.mockImplementation = () = '1234'
// Actually focusing on the logic of your component and just leverage the controllable mocks output // Good: actually focusing on the logic of your component and just leverage the controllable mocks output
expect(wrapper.find('div').html()).toBe('<div id="1234">...</div>') expect(wrapper.find('div').html()).toBe('<div id="1234">...</div>')
``` ```
...@@ -212,22 +204,22 @@ Preferentially, in component testing with `@vue/test-utils`, you should query fo ...@@ -212,22 +204,22 @@ Preferentially, in component testing with `@vue/test-utils`, you should query fo
- A `data-testid` attribute ([recommended by maintainers of `@vue/test-utils`](https://github.com/vuejs/vue-test-utils/issues/1498#issuecomment-610133465)) - A `data-testid` attribute ([recommended by maintainers of `@vue/test-utils`](https://github.com/vuejs/vue-test-utils/issues/1498#issuecomment-610133465))
- a Vue `ref` (if using `@vue/test-utils`) - a Vue `ref` (if using `@vue/test-utils`)
Examples:
```javascript ```javascript
// Bad
it('exists', () => { it('exists', () => {
// Good
wrapper.find(FooComponent);
wrapper.find('input[name=foo]');
wrapper.find('[data-testid="foo"]');
wrapper.find({ ref: 'foo'});
// Bad
wrapper.find('.js-foo'); wrapper.find('.js-foo');
wrapper.find('.btn-primary'); wrapper.find('.btn-primary');
wrapper.find('.qa-foo-component'); wrapper.find('.qa-foo-component');
wrapper.find('[data-qa-selector="foo"]'); wrapper.find('[data-qa-selector="foo"]');
}); });
// Good
it('exists', () => {
wrapper.find(FooComponent);
wrapper.find('input[name=foo]');
wrapper.find('[data-testid="foo"]');
wrapper.find({ ref: 'foo'});
});
``` ```
It is not recommended that you add `.js-*` classes just for testing purposes. Only do this if there are no other feasible options available. It is not recommended that you add `.js-*` classes just for testing purposes. Only do this if there are no other feasible options available.
...@@ -239,23 +231,26 @@ Do not use a `.qa-*` class or `data-qa-selector` attribute for any tests other t ...@@ -239,23 +231,26 @@ Do not use a `.qa-*` class or `data-qa-selector` attribute for any tests other t
When writing describe test blocks to test specific functions/methods, When writing describe test blocks to test specific functions/methods,
please use the method name as the describe block name. please use the method name as the describe block name.
**Bad**:
```javascript ```javascript
// Good describe('#methodName', () => {
describe('methodName', () => {
it('passes', () => { it('passes', () => {
expect(true).toEqual(true); expect(true).toEqual(true);
}); });
}); });
// Bad describe('.methodName', () => {
describe('#methodName', () => {
it('passes', () => { it('passes', () => {
expect(true).toEqual(true); expect(true).toEqual(true);
}); });
}); });
```
// Bad **Good**:
describe('.methodName', () => {
```javascript
describe('methodName', () => {
it('passes', () => { it('passes', () => {
expect(true).toEqual(true); expect(true).toEqual(true);
}); });
...@@ -286,61 +281,67 @@ it('tests a promise rejection', async () => { ...@@ -286,61 +281,67 @@ it('tests a promise rejection', async () => {
You can also work with Promise chains. In this case, you can make use of the `done` callback and `done.fail` in case an error occurred. Following are some examples: You can also work with Promise chains. In this case, you can make use of the `done` callback and `done.fail` in case an error occurred. Following are some examples:
**Bad**:
```javascript ```javascript
// Good // missing done callback
it('tests a promise', () => {
promise.then(data => {
expect(data).toBe(asExpected);
});
});
// missing catch
it('tests a promise', done => { it('tests a promise', done => {
promise promise
.then(data => { .then(data => {
expect(data).toBe(asExpected); expect(data).toBe(asExpected);
}) })
.then(done) .then(done);
.catch(done.fail);
}); });
// Good // use done.fail in asynchronous tests
it('tests a promise rejection', done => { it('tests a promise', done => {
promise promise
.then(done.fail) .then(data => {
.catch(error => { expect(data).toBe(asExpected);
expect(error).toBe(expectedError);
}) })
.then(done) .then(done)
.catch(done.fail); .catch(fail);
});
// Bad (missing done callback)
it('tests a promise', () => {
promise.then(data => {
expect(data).toBe(asExpected);
});
}); });
// Bad (missing catch) // missing catch
it('tests a promise', done => { it('tests a promise rejection', done => {
promise promise
.then(data => { .catch(error => {
expect(data).toBe(asExpected); expect(error).toBe(expectedError);
}) })
.then(done); .then(done);
}); });
```
// Bad (use done.fail in asynchronous tests) **Good**:
```javascript
// handling success
it('tests a promise', done => { it('tests a promise', done => {
promise promise
.then(data => { .then(data => {
expect(data).toBe(asExpected); expect(data).toBe(asExpected);
}) })
.then(done) .then(done)
.catch(fail); .catch(done.fail);
}); });
// Bad (missing catch) // failure case
it('tests a promise rejection', done => { it('tests a promise rejection', done => {
promise promise
.then(done.fail)
.catch(error => { .catch(error => {
expect(error).toBe(expectedError); expect(error).toBe(expectedError);
}) })
.then(done); .then(done)
.catch(done.fail);
}); });
``` ```
...@@ -564,11 +565,11 @@ Examples: ...@@ -564,11 +565,11 @@ Examples:
```javascript ```javascript
const foo = 1; const foo = 1;
// good // Bad
expect(foo).toBe(1);
// bad
expect(foo).toEqual(1); expect(foo).toEqual(1);
// Good
expect(foo).toBe(1);
``` ```
#### Prefer more befitting matchers #### Prefer more befitting matchers
...@@ -621,12 +622,11 @@ Jest has the tricky `toBeDefined` matcher that can produce false positive test. ...@@ -621,12 +622,11 @@ Jest has the tricky `toBeDefined` matcher that can produce false positive test.
the given value for `undefined` only. the given value for `undefined` only.
```javascript ```javascript
// good // Bad: if finder returns null, the test will pass
expect(wrapper.find('foo').exists()).toBe(true);
// bad
// if finder returns null, the test will pass
expect(wrapper.find('foo')).toBeDefined(); expect(wrapper.find('foo')).toBeDefined();
// Good
expect(wrapper.find('foo').exists()).toBe(true);
``` ```
#### Avoid using `setImmediate` #### Avoid using `setImmediate`
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment