feat: improve conformance test to check argument types#20096
feat: improve conformance test to check argument types#20096layershifter merged 9 commits intomasterfrom
Conversation
📊 Bundle size reportUnchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: f728d06fc101970d9d42b5f46de6806a59a0a0c5 (build) |
|
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 481ddaa:
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1000 | 999 | 5000 | |
| BaseButton | mount | 1037 | 1054 | 5000 | |
| Breadcrumb | mount | 2785 | 2781 | 1000 | |
| ButtonNext | mount | 555 | 549 | 5000 | |
| Checkbox | mount | 1718 | 1691 | 5000 | |
| CheckboxBase | mount | 1471 | 1519 | 5000 | |
| ChoiceGroup | mount | 5354 | 5243 | 5000 | |
| ComboBox | mount | 1069 | 1058 | 1000 | |
| CommandBar | mount | 10887 | 10920 | 1000 | |
| ContextualMenu | mount | 7073 | 6890 | 1000 | |
| DefaultButton | mount | 1264 | 1273 | 5000 | |
| DetailsRow | mount | 4125 | 4115 | 5000 | |
| DetailsRowFast | mount | 4132 | 4135 | 5000 | |
| DetailsRowNoStyles | mount | 3918 | 3913 | 5000 | |
| Dialog | mount | 2687 | 2698 | 1000 | |
| DocumentCardTitle | mount | 193 | 191 | 1000 | |
| Dropdown | mount | 3666 | 3685 | 5000 | |
| FluentProviderNext | mount | 3523 | 3442 | 5000 | |
| FluentProviderWithTheme | mount | 222 | 221 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 110 | 102 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 257 | 255 | 10 | |
| FocusTrapZone | mount | 1964 | 1950 | 5000 | |
| FocusZone | mount | 1977 | 1993 | 5000 | |
| IconButton | mount | 2002 | 1979 | 5000 | |
| Label | mount | 376 | 382 | 5000 | |
| Layer | mount | 3337 | 3318 | 5000 | |
| Link | mount | 541 | 537 | 5000 | |
| MakeStyles | mount | 2011 | 1976 | 50000 | |
| MenuButton | mount | 1671 | 1711 | 5000 | |
| MessageBar | mount | 2279 | 2188 | 5000 | |
| Nav | mount | 3662 | 3671 | 1000 | |
| OverflowSet | mount | 1247 | 1226 | 5000 | |
| Panel | mount | 2483 | 2634 | 1000 | |
| Persona | mount | 950 | 946 | 1000 | |
| Pivot | mount | 1586 | 1642 | 1000 | |
| PrimaryButton | mount | 1415 | 1445 | 5000 | |
| Rating | mount | 8732 | 8858 | 5000 | |
| SearchBox | mount | 1514 | 1515 | 5000 | |
| Shimmer | mount | 2807 | 2844 | 5000 | |
| Slider | mount | 2139 | 2194 | 5000 | |
| SpinButton | mount | 5529 | 5465 | 5000 | |
| Spinner | mount | 462 | 459 | 5000 | |
| SplitButton | mount | 3445 | 3802 | 5000 | |
| Stack | mount | 588 | 576 | 5000 | |
| StackWithIntrinsicChildren | mount | 1923 | 1961 | 5000 | |
| StackWithTextChildren | mount | 5444 | 5308 | 5000 | |
| SwatchColorPicker | mount | 11443 | 11455 | 5000 | |
| Tabs | mount | 1576 | 1552 | 1000 | |
| TagPicker | mount | 2862 | 2880 | 5000 | |
| TeachingBubble | mount | 13940 | 13931 | 5000 | |
| Text | mount | 493 | 480 | 5000 | |
| TextField | mount | 1573 | 1534 | 5000 | |
| ThemeProvider | mount | 1314 | 1302 | 5000 | |
| ThemeProvider | virtual-rerender | 653 | 657 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 2075 | 2077 | 5000 | |
| Toggle | mount | 887 | 896 | 5000 | |
| buttonNative | mount | 155 | 151 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AttachmentMinimalPerf.default | 208 | 178 | 1.17:1 |
| AccordionMinimalPerf.default | 191 | 171 | 1.12:1 |
| RefMinimalPerf.default | 266 | 245 | 1.09:1 |
| FormMinimalPerf.default | 492 | 456 | 1.08:1 |
| ReactionMinimalPerf.default | 448 | 416 | 1.08:1 |
| ButtonMinimalPerf.default | 211 | 198 | 1.07:1 |
| ImageMinimalPerf.default | 442 | 418 | 1.06:1 |
| LabelMinimalPerf.default | 455 | 428 | 1.06:1 |
| FlexMinimalPerf.default | 325 | 311 | 1.05:1 |
| TreeWith60ListItems.default | 210 | 200 | 1.05:1 |
| AnimationMinimalPerf.default | 467 | 447 | 1.04:1 |
| AvatarMinimalPerf.default | 223 | 214 | 1.04:1 |
| ChatDuplicateMessagesPerf.default | 348 | 334 | 1.04:1 |
| DialogMinimalPerf.default | 846 | 815 | 1.04:1 |
| HeaderMinimalPerf.default | 434 | 418 | 1.04:1 |
| PortalMinimalPerf.default | 192 | 185 | 1.04:1 |
| SkeletonMinimalPerf.default | 429 | 411 | 1.04:1 |
| TextMinimalPerf.default | 400 | 384 | 1.04:1 |
| TreeMinimalPerf.default | 905 | 870 | 1.04:1 |
| ChatWithPopoverPerf.default | 430 | 417 | 1.03:1 |
| TableManyItemsPerf.default | 2192 | 2137 | 1.03:1 |
| VideoMinimalPerf.default | 739 | 720 | 1.03:1 |
| ChatMinimalPerf.default | 760 | 745 | 1.02:1 |
| DropdownMinimalPerf.default | 3430 | 3352 | 1.02:1 |
| HeaderSlotsPerf.default | 869 | 851 | 1.02:1 |
| InputMinimalPerf.default | 1444 | 1422 | 1.02:1 |
| LayoutMinimalPerf.default | 421 | 413 | 1.02:1 |
| SliderMinimalPerf.default | 1868 | 1832 | 1.02:1 |
| TextAreaMinimalPerf.default | 592 | 580 | 1.02:1 |
| TooltipMinimalPerf.default | 1173 | 1147 | 1.02:1 |
| CardMinimalPerf.default | 641 | 635 | 1.01:1 |
| CarouselMinimalPerf.default | 541 | 534 | 1.01:1 |
| EmbedMinimalPerf.default | 4677 | 4629 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1356 | 1347 | 1.01:1 |
| ListCommonPerf.default | 719 | 712 | 1.01:1 |
| ListMinimalPerf.default | 570 | 566 | 1.01:1 |
| ListNestedPerf.default | 646 | 637 | 1.01:1 |
| LoaderMinimalPerf.default | 764 | 756 | 1.01:1 |
| MenuMinimalPerf.default | 956 | 944 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1833 | 1814 | 1.01:1 |
| ProviderMinimalPerf.default | 1241 | 1231 | 1.01:1 |
| RadioGroupMinimalPerf.default | 521 | 514 | 1.01:1 |
| SegmentMinimalPerf.default | 406 | 403 | 1.01:1 |
| IconMinimalPerf.default | 690 | 681 | 1.01:1 |
| ToolbarMinimalPerf.default | 1062 | 1051 | 1.01:1 |
| BoxMinimalPerf.default | 395 | 395 | 1:1 |
| ButtonOverridesMissPerf.default | 1967 | 1958 | 1:1 |
| ButtonSlotsPerf.default | 643 | 640 | 1:1 |
| ListWith60ListItems.default | 717 | 717 | 1:1 |
| PopupMinimalPerf.default | 632 | 634 | 1:1 |
| SplitButtonMinimalPerf.default | 4732 | 4718 | 1:1 |
| CustomToolbarPrototype.default | 4451 | 4454 | 1:1 |
| DatepickerMinimalPerf.default | 6032 | 6068 | 0.99:1 |
| DividerMinimalPerf.default | 417 | 421 | 0.99:1 |
| GridMinimalPerf.default | 391 | 395 | 0.99:1 |
| ProviderMergeThemesPerf.default | 1800 | 1815 | 0.99:1 |
| StatusMinimalPerf.default | 775 | 782 | 0.99:1 |
| AlertMinimalPerf.default | 304 | 309 | 0.98:1 |
| CheckboxMinimalPerf.default | 2936 | 3001 | 0.98:1 |
| AttachmentSlotsPerf.default | 1194 | 1237 | 0.97:1 |
| RosterPerf.default | 1336 | 1388 | 0.96:1 |
| TableMinimalPerf.default | 438 | 461 | 0.95:1 |
| DropdownManyItemsPerf.default | 773 | 824 | 0.94:1 |
layershifter
force-pushed
the
test/conformance-arg-types
branch
from
b0d22b2 to
7eccf6d
Compare
| * Data attached to open/close events | ||
| */ | ||
| export type OnOpenChangeData = Pick<PopoverState, 'open'>; | ||
| export type OnOpenChangeData = { open: boolean }; |
There was a problem hiding this comment.
As I mentioned I had to simplify this to avoid handling of Pick<> which is quite complex
There was a problem hiding this comment.
I someone uses Pick how easy would it be for them to know that test failures are a result of limitations of conformance tests ?
There was a problem hiding this comment.
I added additional handling in 481ddaa, now it will throw meaningful errors in such cases.
layershifter
requested review from
a team,
behowell and
khmakoto
as code owners
ecraig12345
left a comment
ecraig12345
left a comment
There was a problem hiding this comment.
This is great, thanks! Some minor questions and wording suggestions.
| } | ||
|
|
||
| function typeToString(typeChecker: ts.TypeChecker, type: ts.Type): ArgumentValue | ArgumentValue[] { | ||
| if (isIntrinsicType(type)) { |
There was a problem hiding this comment.
Maybe a silly question, what's an intrinsic type?
There was a problem hiding this comment.
export type TypeC = number;
Not sure that it's correct explanation: but they are internal TS types.
There was a problem hiding this comment.
I also made minor rename in 06ea16b to highlight when we deal with ts.Type vs ts.TypeNode
| // "React" types are handled separately to add "React." prefix otherwise it's impossible to distinguish | ||
| // "React.MouseEvent" and "MouseEvent" types. | ||
|
|
||
| if (/\/node_modules\/@types\/react\//.test(fileName)) { |
There was a problem hiding this comment.
Do you know for sure that the filename always uses forward slashes?
There was a problem hiding this comment.
| return parseArgumentType(typeChecker, type.symbol.declarations[0]); | ||
| } | ||
|
|
||
| if (typeHasSubtypes(type)) { |
There was a problem hiding this comment.
Maybe another silly question, what's an example of a type with subtypes?
There was a problem hiding this comment.
The simplest case is below:
export type TypeB = MouseEvent | number;
export type TypeC = number;
export interface AccordionProps {
onToggle: (b: TypeB) => void;
}
(check an Identifier for TypeB)
So it's "a type with union". May be naming should be improved there, any suggestions?
packages/react-conformance/src/utils/validateCallbackArguments.ts
Outdated
Show resolved
Hide resolved
|
|
||
| if (typeof valueInUnion === 'string') { | ||
| if (valueInUnion === 'Event' || valueInUnion === 'React.SyntheticEvent') { | ||
| throw new Error( |
There was a problem hiding this comment.
I wonder if it would be better to just return an error message rather than throwing in these cases--including the stack from an Error might make things more confusing
There was a problem hiding this comment.
Probably it's my personal preference from backend background 😀 Technically we can return a message, but then we will need to handle checks during all path:
const result1 = validateEventArgument(callbackArguments[argumentNames[0]]);
if(result1) { return result1 }
const result2 = validateDataArgument(callbackArguments[argumentNames[1]]);
if (result2) { return result1 }^ this looks less cleaner for me. Any suggestions?
There was a problem hiding this comment.
personally I'm a fan of throwing errors too 😀, but the above looks good to me
There was a problem hiding this comment.
My concern was less with the control flow (I agree throwing an error is cleaner) and more with the way it will be presented to users through logging, since I don't think the stack from this point would typically be relevant.
There was a problem hiding this comment.
These errors are handled in the test itself and users will see a proper formatted error message:
May be I don't understand something, but I don't see a problem with it 😀
packages/react-conformance/src/utils/validateCallbackArguments.ts
Outdated
Show resolved
Hide resolved
packages/react-conformance/src/utils/validateCallbackArguments.ts
Outdated
Show resolved
Hide resolved
* improve conformance test * fix lint errors * improve error messages * improve error messages * minor rename * use arrays * fix test * throw errors on complex types * fix lint error
5 participants
Pull request checklist
$ yarn changeSample error of the test
Description of changes
This PR improves
consistent-callback-argsconformance test to also check types of parameters 🎉getCallbackArguments()was improve to handle types resolution, i.e. resolveOnOpenChangetypesvalidateCallbackArguments()provides detailed error reportingOther change 💥
I have to update types in
MenuandPopoverto simplify them as resolvingPickis not the simplest thing 😐