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

Choicegroup (v8): convert tests to use testing-library #22208

Conversation

TristanWatanabe
Copy link
Member

Current Behavior

  • v8 Choicegroup tests don't use testing-library

New Behavior

  • Convert Choicegroup to fully use testing-library

Related Issue(s)

part of #21663

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 25, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4f6ff43:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Mar 25, 2022

📊 Bundle size report

🤖 This report was generated against 76476da509050cdbaf554da6ee45ed62fcc83332

@size-auditor
Copy link

size-auditor bot commented Mar 25, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 0844caf318ae0c5ac484019a23cdf167c5ba25ae (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Mar 25, 2022

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 786 783 5000
Breadcrumb mount 2362 2321 1000
Checkbox mount 1302 1284 5000
CheckboxBase mount 1087 1087 5000
ChoiceGroup mount 4104 4038 5000
ComboBox mount 831 887 1000
CommandBar mount 9387 9054 1000
ContextualMenu mount 10008 10025 1000
DefaultButton mount 990 966 5000
DetailsRow mount 3251 3334 5000
DetailsRowFast mount 3316 3231 5000
DetailsRowNoStyles mount 3143 3088 5000
Dialog mount 1891 1891 1000
DocumentCardTitle mount 156 140 1000
Dropdown mount 2853 2852 5000
FocusTrapZone mount 1570 1583 5000
FocusZone mount 1562 1528 5000
IconButton mount 1528 1525 5000
Label mount 300 302 5000
Layer mount 2516 2531 5000
Link mount 407 403 5000
MenuButton mount 1273 1265 5000
MessageBar mount 1876 1771 5000
Nav mount 2832 2820 1000
OverflowSet mount 948 940 5000
Panel mount 1823 1844 1000
Persona mount 878 887 1000
Pivot mount 1247 1247 1000
PrimaryButton mount 1137 1117 5000
Rating mount 6691 6705 5000
SearchBox mount 1126 1111 5000
Shimmer mount 2157 2151 5000
Slider mount 1683 1677 5000
SpinButton mount 4404 4367 5000
Spinner mount 367 374 5000
SplitButton mount 2730 2712 5000
Stack mount 446 447 5000
StackWithIntrinsicChildren mount 1991 2001 5000
StackWithTextChildren mount 4512 4522 5000
SwatchColorPicker mount 10013 10007 5000
TagPicker mount 2336 2334 5000
TeachingBubble mount 81888 82010 5000
Text mount 371 375 5000
TextField mount 1203 1224 5000
ThemeProvider mount 987 1012 5000
ThemeProvider virtual-rerender 552 563 5000
ThemeProvider virtual-rerender-with-unmount 1543 1574 5000
Toggle mount 685 689 5000
buttonNative mount 110 115 5000

@TristanWatanabe TristanWatanabe marked this pull request as ready for review March 25, 2022 15:05

expect(choiceGroupRef.current!.checkedOption).toBeUndefined();
ReactTestUtils.Simulate.change(choiceOptions[0]);
userEvent.click(choiceOptions[0]);
expect(choiceGroupRef.current!.checkedOption).toEqual(TEST_OPTIONS[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Not a new issue but we really ought to verify that the checked state is updated in the DOM, not just in the component state

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've noticed that whenever the selected element is changed, the checked attribute in the DOM never actually changes; it stays on the element that was initially selected on page load. Codepen here

Copy link
Member

Choose a reason for hiding this comment

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

I tried this too and it looks like what's checked in the actual DOM does get updated, though that may not happen until after a re-render. (The option object doesn't get updated, which is expected.)
https://codepen.io/ecraig12345/pen/mdpMyWO?editors=0011

Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

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

LGTM aside from the thing about actually testing the checked state in the DOM, but since that's not a new issue it's okay to just go ahead and merge as-is if you prefer.

@TristanWatanabe TristanWatanabe merged commit 81bcf26 into microsoft:master Mar 30, 2022
@TristanWatanabe TristanWatanabe deleted the choicegroup-convert-to-testing-library branch March 30, 2022 14:52
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.

5 participants