Merge branch 'fe-docs-bad-good-examples' into 'master'

Docs proposal for bad / good examples in Frontend Guidelines

See merge request gitlab-org/gitlab!36576

doc/development/documentation/structure.md

@@ -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.
 - **Use cases**: describes real use case scenarios for that feature/configuration.
 - **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).
 
 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.
 
 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**.
+
+## 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`.

doc/development/testing_guide/frontend_testing.md

@@ -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.
 
-**Bad:**
-
 ```javascript
 const spy = jest.spyOn(idGenerator, 'create')
 spy.mockImplementation = () = '1234'
 
+// Bad
 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>')
 ```
 
@@ -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 Vue `ref` (if using `@vue/test-utils`)
 
-Examples:
-
 ```javascript
+// Bad
 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('.btn-primary');
     wrapper.find('.qa-foo-component');
     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.
@@ -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,
 please use the method name as the describe block name.
 
+**Bad**:
+
 ```javascript
-// Good
-describe('methodName', () => {
+describe('#methodName', () => {
   it('passes', () => {
     expect(true).toEqual(true);
   });
 });
 
-// Bad
-describe('#methodName', () => {
+describe('.methodName', () => {
   it('passes', () => {
     expect(true).toEqual(true);
   });
 });
+```
 
-// Bad
-describe('.methodName', () => {
+**Good**:
+
+```javascript
+describe('methodName', () => {
   it('passes', () => {
     expect(true).toEqual(true);
   });
@@ -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:
 
+**Bad**:
+
 ```javascript
-// Good
+// missing done callback
+it('tests a promise', () => {
+  promise.then(data => {
+    expect(data).toBe(asExpected);
+  });
+});
+
+// missing catch
 it('tests a promise', done => {
   promise
     .then(data => {
       expect(data).toBe(asExpected);
     })
-    .then(done)
-    .catch(done.fail);
+    .then(done);
 });
 
-// Good
-it('tests a promise rejection', done => {
+// use done.fail in asynchronous tests
+it('tests a promise', done => {
   promise
-    .then(done.fail)
-    .catch(error => {
-      expect(error).toBe(expectedError);
+    .then(data => {
+      expect(data).toBe(asExpected);
     })
     .then(done)
-    .catch(done.fail);
-});
-
-// Bad (missing done callback)
-it('tests a promise', () => {
-  promise.then(data => {
-    expect(data).toBe(asExpected);
-  });
+    .catch(fail);
 });
 
-// Bad (missing catch)
-it('tests a promise', done => {
+// missing catch
+it('tests a promise rejection', done => {
   promise
-    .then(data => {
-      expect(data).toBe(asExpected);
+    .catch(error => {
+      expect(error).toBe(expectedError);
     })
     .then(done);
 });
+```
 
-// Bad (use done.fail in asynchronous tests)
+**Good**:
+
+```javascript
+// handling success
 it('tests a promise', done => {
   promise
     .then(data => {
       expect(data).toBe(asExpected);
     })
     .then(done)
-    .catch(fail);
+    .catch(done.fail);
 });
 
-// Bad (missing catch)
+// failure case
 it('tests a promise rejection', done => {
   promise
+    .then(done.fail)
     .catch(error => {
       expect(error).toBe(expectedError);
     })
-    .then(done);
+    .then(done)
+    .catch(done.fail);
 });
 ```
 
@@ -564,11 +565,11 @@ Examples:
 ```javascript
 const foo = 1;
 
-// good
-expect(foo).toBe(1);
-
-// bad
+// Bad
 expect(foo).toEqual(1);
+
+// Good
+expect(foo).toBe(1);
 ```
 
 #### Prefer more befitting matchers
@@ -621,12 +622,11 @@ Jest has the tricky `toBeDefined` matcher that can produce false positive test.
 the given value for `undefined` only.
 
 ```javascript
-// good
-expect(wrapper.find('foo').exists()).toBe(true);
-
-// bad
-// if finder returns null, the test will pass
+// Bad: if finder returns null, the test will pass
 expect(wrapper.find('foo')).toBeDefined();
+
+// Good
+expect(wrapper.find('foo').exists()).toBe(true);
 ```
 
 #### Avoid using `setImmediate`