summaryrefslogtreecommitdiff
path: root/doc/development/refactoring_guide/index.md
diff options
context:
space:
mode:
Diffstat (limited to 'doc/development/refactoring_guide/index.md')
-rw-r--r--doc/development/refactoring_guide/index.md77
1 files changed, 77 insertions, 0 deletions
diff --git a/doc/development/refactoring_guide/index.md b/doc/development/refactoring_guide/index.md
new file mode 100644
index 00000000000..4bd9d0e9c11
--- /dev/null
+++ b/doc/development/refactoring_guide/index.md
@@ -0,0 +1,77 @@
+# Refactoring guide
+
+This document is a collection of techniques and best practices to consider while performing a refactor.
+
+## Pinning tests
+
+Pinning tests help you ensure that you don't unintentionally change the output or behavior of the entity you're refactoring. This even includes preserving any existing *buggy* behavior, since consumers may rely on those bugs implicitly.
+
+### Example steps
+
+1. Identify all the possible inputs to the refactor subject (e.g. anything that's injected into the template or used in a conditional).
+1. For each possible input, identify the significant possible values.
+1. Create a test to save a full detailed snapshot for each helpful combination values per input. This should guarantee that we have "pinned down" the current behavior. The snapshot could be literally a screenshot, a dump of HTML, or even an ordered list of debugging statements.
+1. Run all the pinning tests against the code, before you start refactoring (Oracle)
+1. Perform the refactor (or checkout the commit with the work done)
+1. Run again all the pinning test against the post refactor code (Pin)
+1. Compare the Oracle with the Pin. If the Pin is different, you know the refactoring doesn't preserve existing behavior.
+1. Repeat the previous three steps as necessary until the refactoring is complete.
+
+### Example commit history
+
+Leaving in the commits for adding and removing pins helps others checkout and verify the result of the test.
+
+```bash
+AAAAAA Add pinning tests to funky_foo
+BBBBBB Refactor funky_foo into nice_foo
+CCCCCC Remove pinning tests for funky_foo
+```
+
+Then you can leave a reviewer instructions on how to run the pinning test in your MR. Example:
+
+> First revert the commit which removes the pin.
+>
+> ```bash
+> git revert --no-commit $(git log -1 --grep="Remove pinning test for funky_foo" --pretty=format:"%H")
+> ```
+>
+> Then run the test
+>
+> ```bash
+> yarn run jest path/to/funky_foo_pin_spec.js
+> ```
+
+### Try to keep pins green
+
+It's hard for a refactor to be 100% pure. This means that a pin which captures absolutely everything is bound to fail with
+some trivial and expected differences. Try to keep the pins green by cleaning the pin with the expected changes. This helps
+others quickly verify that a refactor was safe.
+
+[Example](https://gitlab.com/gitlab-org/gitlab/-/commit/7b73da4078a60cf18f5c10c712c66c302174f506?merge_request_iid=29528#a061e6835fd577ccf6802c8a476f4e9d47466d16_0_23):
+
+```javascript
+// funky_foo_pin_spec.js
+
+const cleanForSnapshot = el => {
+ Array.from(rootEl.querySelectorAll('[data-deprecated-attribute]')).forEach(el => {
+ el.removeAttribute('data-deprecated-attribute');
+ });
+};
+
+// ...
+
+expect(cleanForSnapshot(wrapper.element)).toMatchSnapshot();
+```
+
+### Resources
+
+[Unofficial wiki explanation](http://wiki.c2.com/?PinningTests)
+
+### Examples
+
+- [Pinning test in a haml to vue refactor](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/27691#pinning-tests)
+- [Pinning test in isolating a bug](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/32198#note_212736225)
+- [Pinning test in refactoring dropdown](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28173)
+- [Pinning test in refactoring vulnerability_details.vue](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25830/commits)
+- [Pinning test in refactoring notes_award_list.vue](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/29528#pinning-test)
+- [Video of pair programming session on pinning tests](https://youtu.be/LrakPcspBK4)