search

PostHog / posthog

🦔 PostHog provides open-source web & product analytics, session recording, feature flagging and A/B testing that you can self-host. Get started - free.
https://posthog.com
Other
21.88k stars 1.31k forks source link

Frontend testing strategy - epic #6589

Closed mariusandra closed 5 months ago

mariusandra commented 3 years ago

Note: the following below is a proposal. Suggest alternatives, yet let's agree to move fast and not get into long arguments.

We now have the following ways we test our full stack code (excluding deployments & friends here):

This can get overwhelming and we need a set of best practices for the frontend to guide new code. This starts with figuring out what test we actually want to write, which starts with figuring out what needs to be tested.

We can simplify our stack as a series of isolated layers:

image

What is working (backend & logic)

With the current setup we have really solid backend tests, and are starting to get decent test coverage in the Kea layer. So testing these layers seems to be solved:

... along with the connection between them. We do check that the right mocked APIs are called.

E2E tests

Then we have:

While we all have a love and hate relationship with them, I think they're essential. They are the only test we have that simultaneously checks that all the layers in the diagram above work at the same time. I strongly believe we need some E2E tests always running, to make sure the critical user paths we expect users to be able to do in our app (can look at dashboards, can add feature flags, etc) work. We'll never have 100% coverage here and shouldn't strive for it, but these tests have proven to be useful multiple times and I don't see a strong case other than "flakey" for removing them. Flakiness should be fixed, or let's just remove all flakey tests and just test that the page loads or whatnot.

Frontend tests

Thus keeping E2E tests leads to this set to sort through, which all tests the frontend layer in slightly different ways:

Decision 1 - functions: use jest unit tests (and remove given2 from the two tests)

To test regular functions, I'd advocate for removing given to simplify the codebase, and keep regular jest unit tests. Just having given installed for two files adds avoidable complexity, and since we can't use given with TypeScript, I'm not a big fan of the approach anyway. So simple jest tests it is.

Decision 2 - dumb components: react testing library and/or storybook

For testing "dumb components", aka components that are not connected to a logic, the react testing library makes a lot of sense. It's small, the tests run on node, and can do whatever cypress component tests can do with a browser. The development experience might not be that nice since you don't have the cypress interface, though I think not booting up that stuff might actually be a benefit.

This should mean we should start writing tests for any new dumb component under lib/, yet I don't want to make this a hard requirement, as some are just visual with very limited logic. Icons. Buttons. Etc. These should instead be tested with storybook, and checked for visual regressions.

So for a lib component, either add it to storybook, write a react testing utils test, or do both.

Decision 3 smart components: react testing library and/or storybook

Then, finally we have "smart components", aka components tightly coupled to kea logics. All of these can also be tested with the react test utils, but not all should.

  1. Smart components under frontend/src/lib/ should be unit tested using the react testing library. After all, these components offer some sort of visual and functional contract to the rest of the app, and we want to ensure this contract will be met in the future.

  2. Smart components under frontend/src/scenes/ should not be unit tested using the react testing library. This is where a lot of product innovation takes place, and we will forever be falling behind on "no tests, as I'm just going to release it with a flag" behaviour. Interfaces keep changing all the time, and with a unit testing requirement here, about 48% of a manager's time will be spent asking the team to write tests. Instead we should make sure to add various permutations of a scene and its components to storybook, and use visual regression testing to make sure things look right. We will rely on logic tests to catch most of the functional errors between these components.

We can generally assure with TypeScript that the connection between the kea and react layers is intact. However the only thing not assured with the tests in point 2 is that clicking on different elements fires the right actions. We would be flying slightly blind here, and relying on E2E tests and bug reports to catch problems. I honestly think that's fine, especially with TypeScript's helping hand in making sure we at least call actions that exist.

This also means I propose to either remove or rewrite the existing cypress component tests with the react testing library and/or storybook.

Multiple ways to mock

We also have multiple ways to mock requests:

These could be consolidated, but one step at a time. For another PR?

Comments?

Questions? Comments? What am I missing?

Action items:

pauldambra commented 3 years ago

To test regular functions, I'd advocate for removing given to simplify the codebase, and keep regular jest unit tests.

The principle of least surprise gives a strong +1 for this. Particularly as we grow jest tests will be familiar and given will need onboarding. As someone new to them, they feel like an unnecessary layer of abstraction over test setup

I don't see a strong case other than "flakey" for removing them. Flakiness should be fixed, or let's just remove all flakey tests and just test that the page loads or whatnot.

I've never seen a visual testing framework that avoids flakiness. We're getting to be overdue for the thing that replaces Cypress and promises to solve all the problems :)

More than once I've seen good success from adding visual regression tests and then deleting the UI tests that are redundant as a result. So strong agreement from me to push Cypress tests out into one of visual regression testing and react testing library. CI will be faster and should be just as safe.

Smart components under frontend/src/scenes/ should not be unit tested using the react testing library.

Do we need a hard rule? If we always try to answer the questions: how do I know it works now? and how will I know it is working tomorrow? We can approach this on a case by case basis... E.g. components on a payment page might need many tests and an inline edit text field might need zero.

However, I'm struggling to think of a concrete example where RTL does anything other than proving a component is passing the expected event to a logic 🤔

Generally speaking - strong agreement from me on this direction of travel

I'd add that we could do with better documentation/setup on how to run the tests. I can think of four or five commands I've had to learn. Would be good to tell the developer story "here's how you run the tests"

pauldambra commented 3 years ago

Re flaky cypress... Another approach I've seen used is to split that test pack into two. One set stops the build if they fail, and one set nags you but lets the build continue.

Does make it easier to ignore them but also gives you a smaller set to consider deleting or fixing

posthog-bot commented 1 year ago

This issue hasn't seen activity in two years! If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in two weeks.

Twixes commented 5 months ago

Thi may come in useful as context in the future, but we've done parts of this and are probably okay now.