-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
feat(react-conformance): add test for callback arguments #19369
Conversation
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 f5ed1ea:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 7dfbf1225a317a7a930a177b5f4674add1322ad7 (build) |
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 963 | 980 | 5000 | |
BaseButton | mount | 993 | 965 | 5000 | |
Breadcrumb | mount | 2650 | 2650 | 1000 | |
ButtonNext | mount | 469 | 481 | 5000 | |
Checkbox | mount | 1673 | 1781 | 5000 | |
CheckboxBase | mount | 1393 | 1409 | 5000 | |
ChoiceGroup | mount | 5060 | 5147 | 5000 | |
ComboBox | mount | 1021 | 1005 | 1000 | |
CommandBar | mount | 10476 | 10809 | 1000 | |
ContextualMenu | mount | 6596 | 6248 | 1000 | |
DefaultButton | mount | 1196 | 1203 | 5000 | |
DetailsRow | mount | 3900 | 3944 | 5000 | |
DetailsRowFast | mount | 3964 | 3986 | 5000 | |
DetailsRowNoStyles | mount | 3824 | 3802 | 5000 | |
Dialog | mount | 2232 | 2226 | 1000 | |
DocumentCardTitle | mount | 141 | 150 | 1000 | |
Dropdown | mount | 3486 | 3410 | 5000 | |
FluentProviderNext | mount | 7094 | 7513 | 5000 | |
FluentProviderWithTheme | mount | 388 | 339 | 10 | |
FluentProviderWithTheme | virtual-rerender | 101 | 100 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 478 | 480 | 10 | |
FocusTrapZone | mount | 1899 | 1878 | 5000 | |
FocusZone | mount | 1860 | 1955 | 5000 | |
IconButton | mount | 1878 | 1858 | 5000 | |
Label | mount | 356 | 361 | 5000 | |
Layer | mount | 1867 | 1906 | 5000 | |
Link | mount | 491 | 482 | 5000 | |
MakeStyles | mount | 1899 | 1915 | 50000 | |
MenuButton | mount | 1609 | 1616 | 5000 | |
MessageBar | mount | 2209 | 2214 | 5000 | |
Nav | mount | 3762 | 3652 | 1000 | |
OverflowSet | mount | 1118 | 1136 | 5000 | |
Panel | mount | 2170 | 2127 | 1000 | |
Persona | mount | 898 | 867 | 1000 | |
Pivot | mount | 1467 | 1467 | 1000 | |
PrimaryButton | mount | 1326 | 1379 | 5000 | |
Rating | mount | 8453 | 8644 | 5000 | |
SearchBox | mount | 1466 | 1457 | 5000 | |
Shimmer | mount | 2766 | 2809 | 5000 | |
Slider | mount | 2129 | 2116 | 5000 | |
SpinButton | mount | 5343 | 5355 | 5000 | |
Spinner | mount | 429 | 425 | 5000 | |
SplitButton | mount | 3331 | 3250 | 5000 | |
Stack | mount | 552 | 549 | 5000 | |
StackWithIntrinsicChildren | mount | 1703 | 1749 | 5000 | |
StackWithTextChildren | mount | 5002 | 5134 | 5000 | |
SwatchColorPicker | mount | 10884 | 10920 | 5000 | |
Tabs | mount | 1485 | 1437 | 1000 | |
TagPicker | mount | 2747 | 2760 | 5000 | |
TeachingBubble | mount | 12169 | 12314 | 5000 | |
Text | mount | 451 | 458 | 5000 | |
TextField | mount | 1501 | 1559 | 5000 | |
ThemeProvider | mount | 1240 | 1249 | 5000 | |
ThemeProvider | virtual-rerender | 606 | 626 | 5000 | |
ThemeProvider | virtual-rerender-with-unmount | 2065 | 2039 | 5000 | |
Toggle | mount | 840 | 861 | 5000 | |
buttonNative | mount | 125 | 118 | 5000 |
Perf Analysis (@fluentui/react-northstar
)
Perf tests with no regressions
Scenario | Current PR Ticks | Baseline Ticks | Ratio |
---|---|---|---|
RefMinimalPerf.default | 246 | 226 | 1.09:1 |
ImageMinimalPerf.default | 456 | 424 | 1.08:1 |
TreeWith60ListItems.default | 195 | 180 | 1.08:1 |
AnimationMinimalPerf.default | 483 | 451 | 1.07:1 |
ListMinimalPerf.default | 582 | 543 | 1.07:1 |
MenuMinimalPerf.default | 971 | 905 | 1.07:1 |
PortalMinimalPerf.default | 183 | 171 | 1.07:1 |
ButtonMinimalPerf.default | 194 | 183 | 1.06:1 |
ChatWithPopoverPerf.default | 408 | 385 | 1.06:1 |
HeaderSlotsPerf.default | 909 | 855 | 1.06:1 |
AlertMinimalPerf.default | 300 | 287 | 1.05:1 |
AttachmentMinimalPerf.default | 180 | 171 | 1.05:1 |
ChatMinimalPerf.default | 750 | 711 | 1.05:1 |
DropdownManyItemsPerf.default | 776 | 738 | 1.05:1 |
FlexMinimalPerf.default | 330 | 315 | 1.05:1 |
PopupMinimalPerf.default | 650 | 619 | 1.05:1 |
ButtonSlotsPerf.default | 597 | 573 | 1.04:1 |
HeaderMinimalPerf.default | 406 | 391 | 1.04:1 |
ReactionMinimalPerf.default | 435 | 417 | 1.04:1 |
SkeletonMinimalPerf.default | 397 | 382 | 1.04:1 |
VideoMinimalPerf.default | 729 | 702 | 1.04:1 |
SliderMinimalPerf.default | 1699 | 1654 | 1.03:1 |
CardMinimalPerf.default | 629 | 618 | 1.02:1 |
ChatDuplicateMessagesPerf.default | 326 | 321 | 1.02:1 |
FormMinimalPerf.default | 478 | 470 | 1.02:1 |
ListNestedPerf.default | 621 | 611 | 1.02:1 |
RadioGroupMinimalPerf.default | 487 | 478 | 1.02:1 |
SegmentMinimalPerf.default | 381 | 373 | 1.02:1 |
StatusMinimalPerf.default | 781 | 766 | 1.02:1 |
TableMinimalPerf.default | 476 | 468 | 1.02:1 |
TextAreaMinimalPerf.default | 559 | 549 | 1.02:1 |
AttachmentSlotsPerf.default | 1163 | 1157 | 1.01:1 |
ItemLayoutMinimalPerf.default | 1333 | 1325 | 1.01:1 |
LabelMinimalPerf.default | 436 | 431 | 1.01:1 |
ListCommonPerf.default | 700 | 690 | 1.01:1 |
MenuButtonMinimalPerf.default | 1781 | 1763 | 1.01:1 |
ProviderMergeThemesPerf.default | 1722 | 1709 | 1.01:1 |
ProviderMinimalPerf.default | 1092 | 1078 | 1.01:1 |
TextMinimalPerf.default | 383 | 380 | 1.01:1 |
BoxMinimalPerf.default | 388 | 389 | 1:1 |
ButtonOverridesMissPerf.default | 1874 | 1866 | 1:1 |
CheckboxMinimalPerf.default | 2901 | 2904 | 1:1 |
DialogMinimalPerf.default | 840 | 837 | 1:1 |
DividerMinimalPerf.default | 401 | 401 | 1:1 |
GridMinimalPerf.default | 406 | 405 | 1:1 |
ListWith60ListItems.default | 723 | 723 | 1:1 |
TableManyItemsPerf.default | 2130 | 2140 | 1:1 |
CustomToolbarPrototype.default | 4032 | 4039 | 1:1 |
ToolbarMinimalPerf.default | 1017 | 1015 | 1:1 |
TooltipMinimalPerf.default | 1091 | 1093 | 1:1 |
TreeMinimalPerf.default | 854 | 857 | 1:1 |
DropdownMinimalPerf.default | 3340 | 3376 | 0.99:1 |
InputMinimalPerf.default | 1367 | 1379 | 0.99:1 |
LoaderMinimalPerf.default | 740 | 747 | 0.99:1 |
RosterPerf.default | 1294 | 1303 | 0.99:1 |
CarouselMinimalPerf.default | 503 | 513 | 0.98:1 |
EmbedMinimalPerf.default | 4584 | 4655 | 0.98:1 |
SplitButtonMinimalPerf.default | 4537 | 4635 | 0.98:1 |
DatepickerMinimalPerf.default | 5767 | 5973 | 0.97:1 |
LayoutMinimalPerf.default | 391 | 406 | 0.96:1 |
AccordionMinimalPerf.default | 170 | 180 | 0.94:1 |
AvatarMinimalPerf.default | 212 | 226 | 0.94:1 |
IconMinimalPerf.default | 663 | 710 | 0.93:1 |
const handlerArguments = getCallbackArguments(tsProgram, rootFileName, typeName, propName); | ||
|
||
if (Object.keys(handlerArguments).length !== 2) { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to assert that both arguments are objects, since you parse the types already ? or at least they aren't primitites like string, number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be done in a follow up PR as I need to improve resolving logic first (see commented test)
expect(type).toMatchObject({ a: 'String', b: 'Number', c: 'Boolean', d: undefined }); | ||
}); | ||
|
||
it('handles arrays', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it kind of seems like a lot of copy pasted test structure, since you always provide the same structure of input and assertion, it would be nice to use parametrized test like it.each
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but due size of fixtures I would like to leave them as it is. It also easier to use it.only
to debug failing tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome! I just want to echo some of @ling1726's comments and one other minor comment but otherwise looks good to me and would be more than happy to approve once those are addressed.
packages/react-conformance/src/utils/getCallbackArguments.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, and sorry for the delayed review! I have some comments but nothing blocking.
`Definition for "${propName}" does not have ".declarations".`, | ||
'Please report a bug if this happens', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this message will be meaningful to someone who hasn't been immersed in typescript AST analysis, or that it will be clear who to notify or what the bug should contain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to improve it to be more meaningful. Let me know what do you think
let program: ts.Program; | ||
|
||
/** | ||
* Creates a cached TS Program. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't an issue new in this PR, but I realized a couple months ago that due to the way Jest runs tests (separate per file), the program is actually not cached across components. I'm surprised and a bit confused that I didn't notice this during the initial implementation. 😬
It's fine to leave this as-is for now, but ultimately I think to increase test performance, we may need to split all typing-based tests into a separate test suite (maybe in its own package) that invokes typing-related tests on all components at once to get rid of the duplicate analysis. I started work on that back when I noticed the issue but unfortunately haven't gotten to finish. (I can share what I have so far--actually one of the holdups was figuring out the "right" design, so feedback on that would be great.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made an issue #19697 also with a link to my branch with the unfinished (and outdated) changes
…eat/conformance-callbacks � Conflicts: � packages/react-conformance/src/isConformant.ts � packages/react-menu/src/components/MenuList/MenuList.test.tsx
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Pull request checklist
$ yarn change
Description of changes
This PR adds first test for callback arguments to ensure that #18974 is implemented properly.
on*
callbacks (already found issues inMenu*
components 😉)Menu*
,FocusZone
, N* & v9 testsCustom AST processing
In conformance tests we use information about components generated by
react-typescript-docgen
, unfortunately it stringifies information about types:👆 output is simplified
type.name
is just a stringAccordion
's case, in
Tooltip`'s case it's better, but we will need to parse strings)react-docgen-typescript
callschecker.typeToString()
that stringifies type:https://github.com/styleguidist/react-docgen-typescript/blob/40edb172243ed3a431e1346bda1bd74c929af6d4/src/parser.ts#L606
Taking this into account I created additional type parsing via TS AST, see
getCallbackArguments.ts
. This function takes details about an interface and props and returns arguments in readable format. For example:Data provided by this function are used by conformance tests: now we can ensure that number of arguments, their order & types are correct.
For Northstar, we have similar post-processing, microsoft/fluent-ui-react#1634.
Minor changes
createTsProgram()
was moved fromgetComponentDoc()
so it can be reused. We don't want to create new instances for performance reasonsFollow ups
type AccordionToggleEventHandler = (event: AccordionToggleEvent, data: AccordionToggleData) => void