Components: consistently use render from @ariakit/test
#64180
Conversation
|
Size Change: 0 B Total Size: 1.77 MB ℹ️ View Unchanged
|
|
Did a first pass with the codemod. It seems to have worked pretty well (0fcf959), but there are some things to take care of manually. Some code would silently fail (renders outside of "it" functions) and there are a few errors due to the differences in API between RTL and the Ariakit test library. |
DaniGuardiola
changed the title
|
After the codemod pass, I did a manual pass (2919849) to fix all remaining type errors and test failures. Here's a comprehensive list of the types of changes I've had to manually execute and why:
This fixes everything except for:
Finally, there's a potential opportunity to remove the |
DaniGuardiola
changed the title
render from @ariakit/test
…ix/enforce-ariakit-test-library
|
Update: I decided to simply revert to the I also decided to address user events in a separate PR, to keep this one focused - it's already pretty big as it is. |
DaniGuardiola
added
[Type] Code Quality
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Thanks for working on it @DaniGuardiola!
I'm a bit torn on having double standards for testing our components. I believe the entire repo should use a consistent approach for testing. With that in mind, to me it doesn't make much sense to force the components package to one approach, and leave the rest to use the other. If we all agree on a particular approach, then I'd advise we migrate the entire repository to it, update all documentation accordingly and announce it to the community properly.
Also, RTL is at this time the officially recommended testing library for React, and we need a really strong reason to move away from it. Especially considering that RTL might not necessarily be part of ariakit/test's future. I really wish there was a better way, even if we have to be more robust in our tests.
|
@tyxla that's a fair concern, though I would highlight a few observations:
Regarding its use in the components package, I'd say the fact that we use Ariakit extensively is a good reason to enforce it. Whether it should be used across the monorepo is a separate discussion IMO, though my hunch is that yes, we probably should (at least In any case, since both libraries can coexist I don't think this should be a big concern for now, and enforcing only in the components package is good enough to meet our pragmatic goals. I recommend reading my discussion with @ciampo about enforcing in components package / use outside of it: #64066 (comment) |
There was a problem hiding this comment.
Thanks for the thorough elaboration on it @DaniGuardiola! Makes a lot of sense, and I'm generally not opposed to using @ariakit/test, especially if we have really good reasons to prefer it. I do, however, have some concerns, as you will see below.
I've read the discussion with @ciampo you linked to, and I think a good reason to reach a compromise is to expose a layer with testing functionality and use that. This is what we do with Ariakit, too - we don't expose it directly; rather, we expose components that use it. Why not do the same with tests? If I understand correctly, this is what Marco suggested, too. Such an approach can help with concealing implementation details and would make an eventual migration to another testing approach a breeze. Unfortunately, it would still add some maintenance costs (we'll have to maintain that layer).
I do have a few concerns that are worth mentioning, too, and can help evaluate if the effort is worth the time, and if it's the right time for switching to @ariakit/test:
- RTL is recommended as part of the React 19 migration. Yes, it's in the context of deprecating shallow rendering practices, but still, RTL is the preferred react testing library, and if we move away from that, we'll need to spend a larger effort on convincing developers why that's a good idea, educating them on the new best practices, and updating all documentation and examples (some of which we may not have control over) across all sources.
@ariakit/testfalls behind on any developer documentation - there are no docs on the site or in the repo; it actually doesn't even have a README. If we're going to use it (and enforce it), it needs to be really well documented, otherwise it can really hurt the developer experience. I don't think there's a good way around that.- Consistency is important for the
@wordpress/componentspackage, but also beyond it and across the entire monorepo. I'd very much favor using the same approach for testing all components in Gutenberg, not just@wordpress/components. I don't see a good reason to limit any testing approach to solely@wordpress/components. That can make the effort bigger, which is worth considering. - Being future-proof is important as we're betting on WordPress living for many more years. When making choices for tools we use, we need to be careful about the bus factor of these tools, and I'm worried
@ariakit/testhas been worked on almost exclusively by a single person.
It can be helpful to get some more feedback on this one. I'm curious to hear what @ciampo, @mirka, and @jsnajdr could add to the discussion.
|
I don't think this is a controversial change or that there are some long-term consequences. The When I see Ariakit PRs like ariakit/ariakit#2894 I see that @diegohaz has been implementing my suggestions, which is another evidence that all this is a good idea 🙂 The Another thing that I slightly dislike is that // test-helpers.js
import { render } from '@testing-library/react';
import { wrapRender } from '@ariakit/test';
export const render = wrapRender( render );Then my tests would import Yes, probably all unit tests should eventually use |
|
@jsnajdr thanks for your thoughts, I agree for the most part. I would add that RTL is an implementation detail though, and
|
|
I agree it doesn't have to happen all at once - we just need a solid plan that covers all existing tests and execute on it step by step. Realistically, the biggest concern right now is I wonder if @diegohaz considers adding documentation as one of the short-term project priorities. |
|
Reading ariakit/ariakit#3939 I realize that What I think is not controversial is the rendering wrapper around
One thing I never understood about these Ariakit techniques is why the same can't be accomplished with effects and layout effects. If you want something to render in DOM and then be able to measure it, that's what |
To clarify, Alternatively, an issue could be raised in the RTL repo to address microtasks and other timers in |
|
Thanks for chiming in @diegohaz, much appreciated!
That's a bummer, and a real blocker if we're to force
Since e2e tests are slow and time-consuming, we try to use them mostly for testing entire flows. Component tests with RTL, flawed as they are, still provide a much faster way to test against components, and in many cases, that's enough. I think that in favor of developer experience, we need to maintain this tricky balance of having both e2es and component tests.
That would make a lot of sense! I'd be surprised if we were the only ones to need that. |
|
I'm not sure why it'd be a blocker. The render and event trigger utilities are quite straightforward, already do what we need, and are easy to contribute to at any time, or even fork locally if it's ever necessary (as I already did in the Ariakit upgrade). With a bit of JSDOC we're pretty much set on the documentation side of things, and it's 100% compatible with anything else from RTL so we have zero lock-in and can mix and match. What's the specific issue, short or long-term, that would make this a blocker? |
|
The problem is that right now, only we know those details that you're explaining. Someone else who's aiming to contribute component tests will not be aware of those nuances and details, and that can be a blocker for them. This is undesirable, and we should not let it happen IMO. So, if the If you're still unsure, try to put yourself in the shoes of a new contributor. Where would they start? How would they know what and how to use without reading the testing library code? If that part is covered and we have sufficient documentation, we would have solved this blocker (for the most part). |
I think @DaniGuardiola meant Ariakit Test and RTL can be used together (render, queries, user events, etc.) since they both deal with the (JS)DOM, not any unique aspect of either. The APIs are not equivalent, so linking to RTL docs may not be sufficient. I agree with @tyxla that adopting something like this project-wide would require proper documentation. It's not only time-consuming, but Ariakit Test isn't quite ready for that yet. We've added a README to clarify this: https://www.npmjs.com/package/@ariakit/test I initially suggested some Ariakit Test user events to @ciampo as a workaround for issues caused by RTL and as an easy way to check if the failure was due to microtasks or timers. However, I believe the best path for WordPress is to get the RTL packages to consider these factors.
|
|
Honestly, I don't see any of this as a big deal. We simply need to wait for some microtasks to be flushed after render and events. The ariakit test library does this, so we use it. I agree we need to document this (it already is one of the follow-up tasks in the Ariakit upgrade PR, along with this one), which can be done in about a 4 line paragraph. If we really think ariakit test is not the way to go, then sure, let's fork it locally. I personally don't think that's the right call as it's added maintenance with no apparent (to me) advantages, but I'm fine with the decision for the sake of moving this forward and making tests work long term, which is the goal of this PR. To be clear I also support the idea of having RTL add these features, though it seems unlikely to happen any time soon if it happens at all, so it shouldn't block this effort imo. |
That's my understanding as well. I was surprised when @DaniGuardiola claimed 100% compatibility. Especially considering that
I agree with @diegohaz here. Can we realistically pursue contributing this upstream to RTL @DaniGuardiola?
This is where we disagree. If we just use it where it's necessary, that's fine. But if we enforce the entire package to use that approach, then it becomes a bigger deal to me.
I'm strongly against those local forks because this adds extra maintenance costs (we know how temporary solutions become permanent when we add the factor "time") and loses the opportunity to fix the issue upstream, which is the responsible, community-appreciative way to approach this problem IMHO. |
|
Let me be clear: RTL and Ariakit Test are 100% compatible, in the same sense that jQuery and document.querySelector are compatible. Both are libraries that do stuff in a shared DOM. I can render with RTL, get elements with Ariakit Test, and trigger events with RTL. I can render with Ariakit Test, get elements with RTL, and trigger events with Ariakit Test. I can render with React.createRoot, get elements with document.querySelector, and trigger events with element.click. It's all 100% compatible. Of course, RTL's render has a different API to React.createRoot and Ariakit Tests' render. Different API's != incompatible. |
Makes sense. But then you can see why documentation is so important, and why enforcing the different APIs without proper documentation is problematic. As I recommended in my earlier reply, the best long-term solution would be to resolve this upstream in RTL. In the meantime, we could live with RTL and use |
What?
Enforce the use of
@ariakit/testin tests in the components package, as discussed in #64066Why?
The Ariakit test library addresses some shortcomings with RTL in relation to timers. I recommend looking at the linked issue for deeper context. This library also has nicer APIs.
How?
Restrict the use of
renderfrom RTL (event triggers will be addressed in a separate PR), and replace those with the equivalent@ariakit/testutilities. A codemod is used to facilitate the migration.I've added the codemod code to the PR temporarily for reference (
migration/dir), but it will be removed before merge.Tips for reviewing
I separated the codemod changes from manual changes, so I recommend reviewing the diff up till the codemod changes, then looking at the manual changes commit (and successive ones) independently. Please also read the comments since I've written important information throughout.
Follow ups
Read #64180 (comment) to understand follow-up tasks after this PR merges.