Merge pull request #52439 from Expensify/vit-unitTests

[NoQA] Update the checklists and proposal template to include requirement of unit tests

(cherry picked from commit bb4c89d)

(CP triggered by mountiny)

Author: MonilBhavsar

.github/PULL_REQUEST_TEMPLATE.md

@@ -94,7 +94,7 @@ This is a checklist for PR authors. Please make sure to complete all tasks and c
 - [ ] I followed the guidelines as stated in the [Review Guidelines](https://github.com/Expensify/App/blob/main/contributingGuides/PR_REVIEW_GUIDELINES.md)
 - [ ] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like `Avatar`, I verified the components using `Avatar` are working as expected)
 - [ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
-- [ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
+- [ ] I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
 - [ ] I verified that if a function's arguments changed that all usages have also been updated correctly
 - [ ] If any new file was added I verified that:
     - [ ] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
@@ -109,6 +109,7 @@ This is a checklist for PR authors. Please make sure to complete all tasks and c
     - [ ] I verified that all the inputs inside a form are aligned with each other.
     - [ ] I added `Design` label and/or tagged `@Expensify/design` so the design team can review the changes.
 - [ ] If a new page is added, I verified it's using the `ScrollView` component to make it scrollable when more elements are added to the page.
+- [ ] I added [unit tests](https://github.com/Expensify/App/blob/main/tests/README.md) for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
 - [ ] If the `main` branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the `Test` steps.
 
 ### Screenshots/Videos

contributingGuides/PROPOSAL_TEMPLATE.md

@@ -7,6 +7,9 @@
 ### What changes do you think we should make in order to solve the problem?
 <!-- DO NOT POST CODE DIFFS -->
 
+### What specific scenarios should we cover in unit tests to prevent reintroducing this issue in the future?
+<!-- Clearly describe the different test cases you recommend adding or updating. Explain how they will ensure the problem is fully covered and that any future changes do not cause a regression. Consider edge cases, input variations, and typical user interactions that could trigger this issue. To get guidance on how to write unit tests, refer to the README.md in the tests folder. -->
+
 ### What alternative solutions did you explore? (Optional)
 
 **Reminder:** Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

contributingGuides/REVIEWER_CHECKLIST.md

@@ -30,7 +30,7 @@
 - [ ] I verified that this PR follows the guidelines as stated in the [Review Guidelines](https://github.com/Expensify/App/blob/main/contributingGuides/PR_REVIEW_GUIDELINES.md)
 - [ ] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like `Avatar`, I verified the components using `Avatar` have been tested & I retested again)
 - [ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
-- [ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
+- [ ] I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
 - [ ] If a new component is created I verified that:
     - [ ] A similar component doesn't exist in the codebase
     - [ ] All props are defined accurately and each prop has a `/** comment above it */`
@@ -54,6 +54,7 @@
     - [ ] I verified that all the inputs inside a form are aligned with each other.
     - [ ] I added `Design` label and/or tagged `@Expensify/design` so the design team can review the changes.
 - [ ] If a new page is added, I verified it's using the `ScrollView` component to make it scrollable when more elements are added to the page.
+- [ ] For any bug fix or new feature in this PR, I verified that sufficient [unit tests](https://github.com/Expensify/App/blob/main/tests/README.md) are included to prevent regressions in this flow.
 - [ ] If the `main` branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the `Test` steps.
 - [ ] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.
 

tests/README.md

@@ -59,62 +59,51 @@ expect(onyxData).toBe(expectedOnyxData);
 
 ## Documenting Tests
 
-Tests aren't always clear about what exactly is being tested.  To make this a bit easier we recommend adopting the following format for code comments:
+Comments are just as critical in tests as the tests themselves. They provide context behind why the test was written and what the expected behavior is supposed to be which will benefit the future generations of engineers looking at them. Think about it. When was the last time you saw a unit test and wondered why it was written that way and then you didn't want to touch it because you didn't know if changing the behavior of the test was appropriate or not? It was probably pretty recent :D
 
-```
-// Given <initial_condition>
-...  code that sets initial condition
+In order to give future engineers the best context for a unit test, follow this guide:
+
+1. DO add three sections to every test:
+  - "Given" - This introduces the initial condition of the test
+  - "When" - Describes what action is being done and the thing that is being tested
+  - "Then" - Describes what is expected to happen
 
-// When <something_happens>
-... code that does something
+2. DO begin each comment section with the literal words "Given", "When", and "Then", just like the examples below.
+3. DO explain **WHY** the test is doing what it is doing in every comment.
+4. DO NOT explain **WHAT** the code is doing in comments. This information should be self-evident.
+
+The format looks like this:
 
-// Then <expectation>
-... code that performs the assertion
 ```
+// BAD
+// Given an account
+{* code that sets initial condition *}
 
-## Example Test
+// When it is closed
+{* code that does something *}
 
-```javascript
-HttpUtils.xhr = jest.fn();
-
-describe('actions/Report', () => {
-    it('adds an optimistic comment', () => {
-        // Given an Onyx subscription to a report's `reportActions`
-        const ACTION_ID = 1;
-        const REPORT_ID = 1;
-        let reportActions;
-        Onyx.connect({
-            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`,
-            callback: val => reportActions = val,
-        });
-
-        // Mock Report_AddComment command so it can succeed
-        HttpUtils.xhr.mockImplementation(() => Promise.resolve({
-            jsonCode: 200,
-        }));
-
-        // When we add a new action to that report
-        Report.addComment(REPORT_ID, 'Hello!');
-        return waitForBatchedUpdates()
-            .then(() => {
-                const action = reportActions[ACTION_ID];
-
-                // Then the action set in the Onyx callback should match
-                // the comment we left and it will be in a loading state because
-                // it's an "optimistic comment"
-                expect(action.message[0].text).toBe('Hello!');
-                expect(action.isPending).toBe(true);
-            });
-    });
-});
+// Then the user is logged out
+{* code that performs the assertion *}
+
+// GOOD
+// Given an account of a brand new user
+{* code that sets initial condition *}
+
+// When the account is closed by clicking on the close account button
+{* code that does something *}
+
+// Then the user should be logged out because their account is no longer active
+{* code that performs the assertion *}
 ```
 
 ## When to Write a Test
 
-Many of the UI features of our application should go through rigorous testing by you, your PR reviewer, and finally QA before deployment. It's also difficult to maintain UI tests when the UI changes often. Therefore, it's not valuable for us to place every single part of the application UI under test at this time. The manual testing steps should catch most major UI bugs. Therefore, if we are writing any test there should be a **good reason**.
+Many of the UI features of our application should go through rigorous testing by you, your PR reviewer, and finally QA before deployment. However, the code is mature enough now that protecting code against regressions is the top priority.
 
 **What's a "good reason" to write a test?**
 
+- Any PR that fixes a bug
+- When introducing a new feature, cover as much logic as possible by unit tests
 - Anything that is difficult or impossible to run a manual tests on
 	- e.g. a test to verify an outcome after an authentication token expires (which normally takes two hours)
 - Areas of the code that are changing often, breaking often, and would benefit from the resiliency an automated test would provide