New rule: require unit tests for user actions in E/App

[roryabraham]

Slack discussion: https://expensify.slack.com/archives/C03TQ48KC/p1705363865577839

Proposal

Start requiring unit tests for all actions in src/libs/actions that involve optimistic Onyx data.

Problem

One downside of the 1:1:1 API architecture is that API endpoints can have a lot of effects. For a good example or this, consider IOU.splitBill. When a user splits a bill with his three friends, a lot happens all at once:

• We optimistically create an IOU report
• We optimistically create up to four new chat reports (the group DM, and the 1:1 DMs for each of the three friends)
• All created reports have a CREATED action optimistically created.
• We add ADDCOMMENT actions to all four reports.
• We create a transaction
• We create IOU actions on each of the chat reports and the IOU report
• We create REPORTPREVIEW actions too (though, this might actually be missing in SplitBill currently)

Beyond that, Onyx updates may come from the API response or via Pusher / reliable updates. There's a ton going on, and it's really hard as an engineer to even understand how all this is supposed to work. So when there are problems or regressions in the flow (as is common, here are two recent examples #33770, #34547), it requires some substantial legwork to understand what's meant to happen, diagnose the problem, and fix it.

Solution

Create a new rule for the E/App codebase: all actions in src/libs/actions that apply optimisticData in Onyx must be covered by unit tests, similar to the ones we have in IOUTest.js.

These tests will be valuable in preventing regressions and ensuring that all edge cases are properly handled. But perhaps more importantly, they'll double as documentation for how the user action flows are meant to work.

[roryabraham]

Main action items here:

• Document this change in READMEs / style guides
• Communicate this change to the open source community
• Create a webhook comment that will check if you open a PR that modifies any file in actions/libs, and if so have Melvin leave a comment on the PR calling out this requirement to bring attention to it to the PR author and reviewer.
• Update the author and reviewer checklists to account for this requirement. This can be easily done using a dynamic checklist item, which we have a fledgling POC for in place.

[roryabraham]

I think I was too quick to make this issue. People agreed in general that these tests are valuable, but not as a blanket rule. I think I'm more bullish on pursuing a solution to this problem that in the form of compile-time type declarations.

I think if I'm going to pursue this problem further, I'll do it in the context of https://github.com/Expensify/Expensify/issues/363913