[NoQA] Update the checklists and proposal template to include requirement of unit tests #52439
Conversation
…date the proposal template
There was a problem hiding this comment.
I'd love to throw a few changes to the unit test README into this PR too. Maybe you want to take a crack at them?
I'm particularly interested in expanding the "Documenting Tests" section here: https://github.com/Expensify/App/blob/main/tests/README.md
We need to explain:
- How important comments are (ie. they give context for every engineer that will come after you)
- The comments need to explain "why" and not just "what" (with some examples)
- We should maybe clarify that hard-coded JSON (like is used to seed Onyx with) is best to be put in an external JSON file with the same name as the test.
|
I will try to incorporate that thanks |
|
Thanks @tgolen, useful changes! Addressed the feedback |
|
LGTM |
There was a problem hiding this comment.
I love that we are forcing this and we are documenting the requirements of test cases well. But it feels to me like we're missing the 'how'. We have lots of unit tests but no UI or Component tests as far as I can see.
Let's say tomorrow someone updates the SearchRouter component and they ask how to do this. If we can't point to an example then it seems to me that the developer and contributor will be figuring out a lot of things in isolation from the rest of the team.
Maybe this is easy and I am overestimating the complexity here, but I worry we're going to end up with a wide variety of UI test implementations that we'll need to take time to standardise in a few weeks time.
|
Not sure if i follow that we do not have any ui tests, the three examples in the folder are ui tests, where we mock the components and screen to verify the behaviour is correct. But i agree with you that for ui tests more examples will be handy and we will add them to make the "how" a bit less of a concern |
| @@ -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. --> | |||
There was a problem hiding this comment.
We say 'To get guidance on how to write unit tests' here, but I believe we want all changes to be under test which includes UI/component testing. Should we update this to link to both unit and component testing folders?
There was a problem hiding this comment.
Perhaps we are including component tests under 'unit', which is fine but to a contributor they might think: 'oh well I modified this components state when another component is modified, therefore I don't need to add a unit test'
| ``` | ||
|
|
||
| ## 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?** |
There was a problem hiding this comment.
Lets remove this line so that you must have a good reason to NOT write a test
There was a problem hiding this comment.
And lets include these as a starting point: https://expensify.slack.com/archives/CC7NECV4L/p1731686459863719?thread_ts=1731609633.072659&cid=CC7NECV4L
Explanation of Change
To uphold the quality of our codebase and decrease the number of regressions, we need a robust test suite of unit tests. For each bug found or new feature developed, we will require adding a unit test that should catch the bug before reintroducing it.
Fixed Issues
$ N/A
PROPOSAL:
Tests
N/A
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop