Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Expose additional profiling tools for library testing #30031

Open
phryneas opened this issue Jun 21, 2024 · 5 comments
Open

Comments

@phryneas
Copy link
Contributor

phryneas commented Jun 21, 2024

Hi there!
I did bring this up recently in a tangentially-related PR discussion, and @sebmarkbage pointed out that this would do better as it's own issue, so here goes :)

We found that for testing a library, the "eventual consistency" style of RTL-testing doesn't really work, and we need different tools.

While the test is waiting for a consistent result, we could inadvertedly rerender 20 times with inconsistent return values in-between, and our tests wouldn't catch it.
While that is usually fine in UIs, having that happen in a library... well, every single accidental rerender we perform trickles down to thousands of applications, and an inconsistent render would cause tons of userland bugs.

So, as a result, most of our newer tests use the component and every time a React render happened, we take a snapshot - depending of the test, we sometimes snapshot a DOM, sometimes we snapshot hook return values, sometimes other things.

Especially in our Suspense hooks tests, it has been proven invaluable that we also track which components actively participated in a specific render. Tracking those is a bit fiddly at the moment, but we make do. (Keep in mind, we're testing a hook library, so every test defines a bunch of components and then we test how they interact - we can just add something like useTrackRender() to each component.)
Then, our tests consume all these snapshots in an async-iterator-like pattern, so we can make assumptions on every single render.

Examples on how those tests look:

Just to be clear again: we know that we're very close to "testing implementation details" with something like that, and I'd never recommend anyone to test an actual application like that. But for a library, this style of testing is an amazing tool - and we're fine with the drawback that we have some tests that have to test for different "correct" behaviour depending on the React version we're testing against.

Since you are currently adding more apis to access internals like stack traces during testing, I was wondering if it would be a good time to ask for additional Profiler tooling.
Right now, it is next to impossible for any tooling except for the React Devtools to get the information of a Profiler recording (there are too many internals involved, and on top of that, a lot of interprocess communication that would not be necessary in a testing scenario) - but for us, getting that data would be invaluable for that style of testing.

Could we get something like a react-dom/test renderWithProfilerRecording (or even better, renderToProfilerIterator) API?

Getting information like

  • how many rerenders happened
  • which components caused which rerender
  • which components participated in which rerender

would be incredibly powerful to guarantee an optimal library experience.

@markerikson
Copy link
Contributor

I'd love to see this myself. There's actually a couple very relevant PRs here:

But the comment on that by Joe was somewhat discouraging:

We discussed this as a team a while back, and we are very concerned about the cost of supporting this long-term. Although the intent would be just for non-critical (to application correctness) things like performance monitoring, it’s inevitable that applications or libraries would end up relying on these hooks for their core behavior. That would slow us down since it would make many more internal refactors into breaking changes, delaying those refactors and the improvements they would unlock.
We think the right first step is the transition tracing api that we’ve discussed before in labs posts. In our backlog.

@phryneas
Copy link
Contributor Author

phryneas commented Jun 21, 2024

Yeah, this is definitely not something that should hold React back. If things change, they change. This would just surface it.

The idea is more to ensure that if a new React version is released and one of our apis unexpectedly starts seeing additional renders, we can catch and fix that sooner rather than later.
It's definitely only targeted for a "testing" use case - which is also why I suggest something like a react-dom/test entrypoint.

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 22, 2024

Have you explored wrapping your tests in React.Profiler and asserting on the render callbacks? I explored that originaly in MUI. The idea was that we wrap every test in React.Profiler and track changes. It seemed promising but I didn't finish it with tracking numbers in a useful way (p90, diffs between commits, measurements over time etc).

You may want to check out if you get more accurate real world results by not using act in these tests.

Transition tracing would help for more detailed insights. Though maybe we can just expose the updater tracking React Devtools has already access to ("Why did this render?" in the Profiler panel).

@phryneas
Copy link
Contributor Author

Have you explored wrapping your tests in React.Profiler and asserting on the render callbacks?

We kinda do - we wrap the tests in React.Profiler, and on every onRender callback, we create snapshots of the DOM and additional test-specific additional values (hook return values etc.).
We just don't immediately make assertions, but we save these values to the side, to make assertions on them at another time.

You may want to check out if you get more accurate real world results by not using act in these tests.

Now I'm intrigued - isn't the advice to use act to get more real world results?

Though maybe we can just expose the updater tracking React Devtools has already access to

Yes, please, let's start small - perfect is the enemy of "at all", and there is a ton of data currently available to the DevTools that we cannot access from tests right now. Getting access to these would already be incredible, we don't need anything beyond that right now!

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 25, 2024

We kinda do - we wrap the tests in React.Profiler, and on every onRender callback, we create snapshots of the DOM and additional test-specific additional values (hook return values etc.).
We just don't immediately make assertions, but we save these values to the side, to make assertions on them at another time.

So you're not actually interested in profiling but making more granular assertions. I think you just want to test without act to assert on the intermediated steps not profiling tools.

Now I'm intrigued - isn't the advice to use act to get more real world results?

act() flushes everything scheduled before it exits. That's not necessarily what you'll see in real world applications e.g. React receives mousemove events it may batch the state updates into a single commit. If you wrap both in act, you'll always get two commits.

Yes, please, let's start small - perfect is the enemy of "at all", and there is a ton of data currently available to the DevTools that we cannot access from tests right now. Getting access to these would already be incredible, we don't need anything beyond that right now!

Except these have holes and bugs at the moment. Transition tracing is still in progress so we shouldn't just expose updater tracking just that we have something. There is a cost to shipping unfinished things at React's scale. If it doesn't work out, there's a lot of work required to remove it holding back the ecosystem. This works in internal repos like at Meta since you have direct access but doesn't scale to the ecosystem with unknown priorities and motivations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants