From 6c69ab83991f4f4b610bf14670402ca358e61e59 Mon Sep 17 00:00:00 2001 From: Vit Horacek Date: Wed, 13 Nov 2024 01:08:47 +0100 Subject: [PATCH 1/8] Update the reviewer and author checklist to include unit tests and update the proposal template --- .github/PULL_REQUEST_TEMPLATE.md | 3 ++- contributingGuides/PROPOSAL_TEMPLATE.md | 3 +++ contributingGuides/REVIEWER_CHECKLIST.md | 3 ++- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 459a780ca8b4..917ca7fff142 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.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 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 diff --git a/contributingGuides/PROPOSAL_TEMPLATE.md b/contributingGuides/PROPOSAL_TEMPLATE.md index 8c9fa7968fe2..437355448269 100644 --- a/contributingGuides/PROPOSAL_TEMPLATE.md +++ b/contributingGuides/PROPOSAL_TEMPLATE.md @@ -7,6 +7,9 @@ ### What changes do you think we should make in order to solve the problem? +### What specific scenarios should we cover in unit tests to prevent reintroducing this issue in the future? + + ### 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. diff --git a/contributingGuides/REVIEWER_CHECKLIST.md b/contributingGuides/REVIEWER_CHECKLIST.md index 5fc14328f3b4..baee49e27156 100644 --- a/contributingGuides/REVIEWER_CHECKLIST.md +++ b/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 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. From f1a930d0687272601b2cb0e7c74cde6923c096a4 Mon Sep 17 00:00:00 2001 From: Vit Horacek Date: Wed, 13 Nov 2024 01:13:26 +0100 Subject: [PATCH 2/8] Update the template --- contributingGuides/PROPOSAL_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributingGuides/PROPOSAL_TEMPLATE.md b/contributingGuides/PROPOSAL_TEMPLATE.md index 437355448269..aee4aa5d22e5 100644 --- a/contributingGuides/PROPOSAL_TEMPLATE.md +++ b/contributingGuides/PROPOSAL_TEMPLATE.md @@ -8,7 +8,7 @@ ### What specific scenarios should we cover in unit tests to prevent reintroducing this issue in the future? - + ### What alternative solutions did you explore? (Optional) From e2fcccf8e6b60adb6ab2c58931bc5ec3c80b8c13 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 13 Nov 2024 14:12:57 -0700 Subject: [PATCH 3/8] Add more info on given/when/then format --- tests/README.md | 70 ++++++++++++++++++++----------------------------- 1 file changed, 28 insertions(+), 42 deletions(-) diff --git a/tests/README.md b/tests/README.md index 890697d80719..d119179114f3 100644 --- a/tests/README.md +++ b/tests/README.md @@ -59,54 +59,40 @@ 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 criticle 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 -... 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 -... code that does something +2. DO explain **WHY** the test is doing what it is doing in every comment. +3. DO NOT explain **WHAT** the code is doing in comments. This information should be self-evident. + +The format looks like this: -// Then -... 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 From 258a87a3e74b33f5443d4877bc607c172c6699d3 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 13 Nov 2024 14:18:05 -0700 Subject: [PATCH 4/8] Improve some more of the docs --- tests/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/README.md b/tests/README.md index d119179114f3..8a97660f09b5 100644 --- a/tests/README.md +++ b/tests/README.md @@ -97,10 +97,11 @@ The format looks like this: ## 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 - 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 From a6fb2039de9dbb9125b651e594daca64399d84ce Mon Sep 17 00:00:00 2001 From: Vit Horacek Date: Thu, 14 Nov 2024 19:19:37 +0100 Subject: [PATCH 5/8] Address PR feedback --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- contributingGuides/PROPOSAL_TEMPLATE.md | 2 +- contributingGuides/REVIEWER_CHECKLIST.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 917ca7fff142..228a605d2c86 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -109,7 +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 for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow. +- [ ] 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 diff --git a/contributingGuides/PROPOSAL_TEMPLATE.md b/contributingGuides/PROPOSAL_TEMPLATE.md index aee4aa5d22e5..ea908b5b0666 100644 --- a/contributingGuides/PROPOSAL_TEMPLATE.md +++ b/contributingGuides/PROPOSAL_TEMPLATE.md @@ -8,7 +8,7 @@ ### What specific scenarios should we cover in unit tests to prevent reintroducing this issue in the future? - + ### What alternative solutions did you explore? (Optional) diff --git a/contributingGuides/REVIEWER_CHECKLIST.md b/contributingGuides/REVIEWER_CHECKLIST.md index baee49e27156..545c79a95af1 100644 --- a/contributingGuides/REVIEWER_CHECKLIST.md +++ b/contributingGuides/REVIEWER_CHECKLIST.md @@ -54,7 +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 are included to prevent regressions in this flow. +- [ ] 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. From e5baec5f9f91821f464bf6c1ff5523482504194d Mon Sep 17 00:00:00 2001 From: Vit Horacek Date: Thu, 14 Nov 2024 19:27:17 +0100 Subject: [PATCH 6/8] Also mention new features --- tests/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/README.md b/tests/README.md index 8a97660f09b5..cd6cdefd0b40 100644 --- a/tests/README.md +++ b/tests/README.md @@ -102,6 +102,7 @@ Many of the UI features of our application should go through rigorous testing by **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 From 8bd9c6cabf7b471e4b67e34276bf8de41e90379c Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 19 Nov 2024 09:24:19 -0700 Subject: [PATCH 7/8] Update tests/README.md --- tests/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/README.md b/tests/README.md index cd6cdefd0b40..87aa9068013b 100644 --- a/tests/README.md +++ b/tests/README.md @@ -59,7 +59,7 @@ expect(onyxData).toBe(expectedOnyxData); ## Documenting Tests -Comments are just as criticle 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 +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 In order to give future engineers the best context for a unit test, follow this guide: From a73f1421afeba523a992457861c676bc70935afe Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 19 Nov 2024 09:25:55 -0700 Subject: [PATCH 8/8] Be even more clear --- tests/README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/README.md b/tests/README.md index 87aa9068013b..08df0b89794b 100644 --- a/tests/README.md +++ b/tests/README.md @@ -68,8 +68,9 @@ In order to give future engineers the best context for a unit test, follow this - "When" - Describes what action is being done and the thing that is being tested - "Then" - Describes what is expected to happen -2. DO explain **WHY** the test is doing what it is doing in every comment. -3. DO NOT explain **WHAT** the code is doing in comments. This information should be self-evident. +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: