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

Reusable tests #7011

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Reusable tests #7011

wants to merge 18 commits into from

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Sep 9, 2024

Closes

General idea here is that a test suite is built that tries to be as agnostic of the content as possible. Instead, based on a set of conditions which need to be met per-renderer, it will test aria patterns.

If we want to test controlled states, we just create a renderer which renders a controlled implementation of the component. This allows us to test things like the onChange/value props indirectly. This leaves as much of the API for each component as possible up to the implementation.

If a renderer is not provided, then a set of the tests are skipped. This is useful because you may not want or need to run the absolute full suite on every implementation. You may also not have implemented a particular aria feature yet, but it's good to have tests on everything else. You can turn on those tests when you implement the feature easily enough.

Still working on API for it.

Spectrum v3 and s2 tests would be restricted to their own suites using Daniel's work to simplify.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@rspbot
Copy link

rspbot commented Sep 11, 2024

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having standard tests built at the aria level that then the actual library implementation of the aria pattern can then reuse, definitely makes it more efficient. My only concern would be the more complicated nature of the setup haha, certainly more strict with the structuring than writing the test as you'd like for a given component. I think it will be beneficial in the long run though but will have to give it a shot first before further judgement

packages/@react-aria/test-utils/src/menu.ts Outdated Show resolved Hide resolved
packages/@react-aria/test-utils/src/menu.ts Show resolved Hide resolved
packages/@react-aria/test-utils/src/menu.ts Show resolved Hide resolved
Comment on lines +90 to +107
it('has default behavior (button renders, menu is closed)', function () {
let tree = renderers.standard();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I like the reusability of these tests, essentially establishes a standard suite that theoretically ANY implementation of the aria menu pattern could directly use. I assume you brain stormed these first and then pulled out/reused duplicates from the existing suites?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it a little more organically than that, I pulled out from the current React Spectrum MenuTrigger almost 1:1, then started grouping based on how different the render requirements were.

I set out to have the tests be agnostic of dynamic case, or static case, or uncontrolled, or controlled, but that was as much as I had originally planned ahead

});
});

// better to accept items from the test? or just have the test have a requirement that you render a certain-ish structure?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally leaning towards the test having a requirement that you render a certain structure of menu, would make it easier to have others reuse the tests for their own implementation. If we aren't interested in that (it would be a lot of work to take on), then maybe we could accept items/button label/any other standardization from the test perhaps

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd like to decouple more from the react testing library render or React in general, but right now it's still baked in pretty hard thanks to act most specifically.
Goal for another time, but hopefully this helps set us up for it eventually.

@LFDanLu
Copy link
Member

LFDanLu commented Sep 14, 2024

I've also updated the utils PR with the remainder of the comments, apologies about the conflicts but hopefully they aren't too bad

@snowystinger
Copy link
Member Author

I've also updated the utils PR with the remainder of the comments, apologies about the conflicts but hopefully they aren't too bad

no worries, it wasn't that bad

@rspbot
Copy link

rspbot commented Sep 16, 2024

@rspbot
Copy link

rspbot commented Sep 20, 2024

Base automatically changed from aria_pattern_utils to main September 24, 2024 23:26
@rspbot
Copy link

rspbot commented Sep 27, 2024

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

Successfully merging this pull request may close these issues.

3 participants