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

chore: implement test in memory execution based on TS aliases #17161

Merged
merged 7 commits into from
Mar 11, 2021

Conversation

Hotell
Copy link
Contributor

@Hotell Hotell commented Feb 24, 2021

Pull request checklist

Description of changes

  • implements new infra for executing test without need to build first (based on one source of truth - TS path aliases)
  • makes react conformance package conformant with rest of our packages (removal of esModuleInterop)
  • get rid of deep imports within react-utilities package so tsPath aliases resolution works

Before:

// build
yarn lage build --to @fluentui/react-menu
// run test in watch mode
yarn workspace @fluentui/react-menu start-test

// change files outside react-menu
// kill watch

// re-build
yarn lage build --to @fluentui/react-menu
// re-run watch mode
yarn workspace @fluentui/react-menu start-test

// repeat previous until done

After:

yarn workspace @fluentui/react-menu test --watch
// change files within/outside react-menu - all good, continue with your workflow ⚡️

Focus areas to test

(optional)

@Hotell Hotell changed the title Hotell/16889/new jest config chore: implement test in memory execution based on TS aliases Feb 24, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 24, 2021

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 d2570b4:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Feb 24, 2021

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1119 1135 5000
BaseButton mount 854 844 5000
Breadcrumb mount 41163 41143 5000
ButtonNext mount 1273 1204 5000
Checkbox mount 1425 1444 5000
CheckboxBase mount 1195 1189 5000
ChoiceGroup mount 4488 4490 5000
ComboBox mount 911 921 1000
CommandBar mount 9648 9686 1000
ContextualMenu mount 5855 5825 1000
DefaultButton mount 1085 1067 5000
DetailsRow mount 3494 3398 5000
DetailsRowFast mount 3426 3443 5000
DetailsRowNoStyles mount 3238 3207 5000
Dialog mount 1410 1383 1000
DocumentCardTitle mount 1740 1780 1000
Dropdown mount 3142 3128 5000
FocusTrapZone mount 1743 1694 5000
FocusZone mount 1774 1748 5000
IconButton mount 1628 1676 5000
Label mount 320 314 5000
Layer mount 1684 1701 5000
Link mount 440 443 5000
MakeStyles mount 1955 1942 50000
MenuButton mount 1414 1402 5000
MessageBar mount 1925 1922 5000
Nav mount 3127 3095 1000
OverflowSet mount 1006 960 5000
Panel mount 1370 1345 1000
Persona mount 804 778 1000
Pivot mount 1343 1332 1000
PrimaryButton mount 1219 1229 5000
Rating mount 7057 7146 5000
SearchBox mount 1237 1253 5000
Shimmer mount 2389 2446 5000
Slider mount 1892 1885 5000
SpinButton mount 4793 4741 5000
Spinner mount 406 401 5000
SplitButton mount 3033 2961 5000
Stack mount 469 471 5000
StackWithIntrinsicChildren mount 1425 1461 5000
StackWithTextChildren mount 4331 4265 5000
SwatchColorPicker mount 9812 9658 5000
Tabs mount 1314 1330 1000
TagPicker mount 2714 2658 5000
TeachingBubble mount 11113 11124 5000
Text mount 387 390 5000
TextField mount 1276 1315 5000
ThemeProvider mount 1120 1113 5000
ThemeProvider virtual-rerender 589 571 5000
ThemeProviderNext mount 15312 15446 5000
Toggle mount 774 758 5000
buttonNative mount 115 104 5000

Perf Analysis (Fluent)

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
DatepickerMinimalPerf.default 43091 43107 1:1 analysis
Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🦄 Avatar.Fluent 0.16 0.45 0.36:1 2000 327
🦄 Button.Fluent 0.11 0.19 0.58:1 5000 526
🔧 Checkbox.Fluent 0.63 0.32 1.97:1 1000 631
🎯 Dialog.Fluent 0.15 0.2 0.75:1 5000 765
🔧 Dropdown.Fluent 3.02 0.4 7.55:1 1000 3021
🔧 Icon.Fluent 0.13 0.05 2.6:1 5000 651
🦄 Image.Fluent 0.08 0.12 0.67:1 5000 379
🔧 Slider.Fluent 1.5 0.43 3.49:1 1000 1501
🔧 Text.Fluent 0.07 0.03 2.33:1 5000 350
🦄 Tooltip.Fluent 0.13 0.87 0.15:1 5000 672

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AccordionMinimalPerf.default 162 148 1.09:1
LabelMinimalPerf.default 434 411 1.06:1
ChatMinimalPerf.default 629 598 1.05:1
ItemLayoutMinimalPerf.default 1236 1181 1.05:1
TextAreaMinimalPerf.default 478 457 1.05:1
AvatarMinimalPerf.default 204 196 1.04:1
ProviderMinimalPerf.default 924 886 1.04:1
SegmentMinimalPerf.default 366 351 1.04:1
ToolbarMinimalPerf.default 973 940 1.04:1
VideoMinimalPerf.default 638 614 1.04:1
Checkbox.Fluent 631 606 1.04:1
ButtonOverridesMissPerf.default 1634 1592 1.03:1
DividerMinimalPerf.default 367 358 1.03:1
HeaderSlotsPerf.default 772 748 1.03:1
StatusMinimalPerf.default 731 711 1.03:1
TooltipMinimalPerf.default 927 902 1.03:1
Dialog.Fluent 765 744 1.03:1
CardMinimalPerf.default 554 545 1.02:1
CarouselMinimalPerf.default 473 464 1.02:1
HeaderMinimalPerf.default 385 376 1.02:1
IconMinimalPerf.default 671 660 1.02:1
ButtonMinimalPerf.default 178 177 1.01:1
ChatWithPopoverPerf.default 383 381 1.01:1
DialogMinimalPerf.default 778 772 1.01:1
DropdownMinimalPerf.default 3076 3049 1.01:1
GridMinimalPerf.default 371 368 1.01:1
ImageMinimalPerf.default 386 382 1.01:1
LayoutMinimalPerf.default 415 411 1.01:1
LoaderMinimalPerf.default 705 699 1.01:1
TableMinimalPerf.default 415 411 1.01:1
TextMinimalPerf.default 367 362 1.01:1
Dropdown.Fluent 3021 2982 1.01:1
ButtonUseCssPerf.default 803 806 1:1
ButtonUseCssNestingPerf.default 1031 1033 1:1
EmbedMinimalPerf.default 4078 4098 1:1
MenuMinimalPerf.default 860 859 1:1
MenuButtonMinimalPerf.default 1522 1520 1:1
ProviderMergeThemesPerf.default 1560 1559 1:1
RadioGroupMinimalPerf.default 436 435 1:1
SplitButtonMinimalPerf.default 3620 3629 1:1
CustomToolbarPrototype.default 3579 3584 1:1
AttachmentSlotsPerf.default 1161 1170 0.99:1
BoxMinimalPerf.default 360 364 0.99:1
ButtonSlotsPerf.default 559 567 0.99:1
CheckboxMinimalPerf.default 2732 2754 0.99:1
DropdownManyItemsPerf.default 695 702 0.99:1
FlexMinimalPerf.default 302 305 0.99:1
InputMinimalPerf.default 1240 1256 0.99:1
ReactionMinimalPerf.default 397 399 0.99:1
SkeletonMinimalPerf.default 366 371 0.99:1
TableManyItemsPerf.default 1970 1985 0.99:1
TreeMinimalPerf.default 764 773 0.99:1
Icon.Fluent 651 656 0.99:1
Text.Fluent 350 353 0.99:1
Tooltip.Fluent 672 676 0.99:1
AnimationMinimalPerf.default 390 400 0.98:1
PortalMinimalPerf.default 168 171 0.98:1
SliderMinimalPerf.default 1499 1529 0.98:1
Button.Fluent 526 537 0.98:1
Image.Fluent 379 387 0.98:1
Slider.Fluent 1501 1535 0.98:1
ListMinimalPerf.default 488 503 0.97:1
ListWith60ListItems.default 608 624 0.97:1
PopupMinimalPerf.default 684 704 0.97:1
RefMinimalPerf.default 232 238 0.97:1
AlertMinimalPerf.default 281 292 0.96:1
ChatDuplicateMessagesPerf.default 280 293 0.96:1
FormMinimalPerf.default 410 426 0.96:1
ListCommonPerf.default 625 650 0.96:1
ListNestedPerf.default 549 569 0.96:1
Avatar.Fluent 327 341 0.96:1
AttachmentMinimalPerf.default 151 161 0.94:1
RosterPerf.default 1094 1214 0.9:1
TreeWith60ListItems.default 161 178 0.9:1

@size-auditor
Copy link

size-auditor bot commented Feb 24, 2021

Asset size changes

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

Baseline commit: 7f5086718eec496063c6830302c162117fcfc4ec (build)

@Hotell Hotell force-pushed the hotell/16889/new-jest-config branch 3 times, most recently from b709efc to e19ae57 Compare February 25, 2021 13:49
@Hotell
Copy link
Contributor Author

Hotell commented Feb 25, 2021

another interesting CI failure. on local machine I'm getting api extractor error on web-components package , any idea how to resolve this ?

Local machine issue when running yarn buildci
image

Ci issue
image

@ecraig12345
Copy link
Member

@chrisdholt Can you help with the web-components build error above?

@chrisdholt
Copy link
Member

@chrisdholt Can you help with the web-components build error above?

@ecraig12345 @Hotell It looks like API extractor was (possibly) completed successfully. There are two commands running, can you confirm that running yarn doc:ci results in the above error? If it doesn't, try running yarn test-chrome:verbose and see if that's where the error is coming from.

@Hotell
Copy link
Contributor Author

Hotell commented Feb 25, 2021

@chrisdholt Can you help with the web-components build error above?

@ecraig12345 @Hotell It looks like API extractor was (possibly) completed successfully. There are two commands running, can you confirm that running yarn doc:ci results in the above error? If it doesn't, try running yarn test-chrome:verbose and see if that's where the error is coming from.

That error is happening on my local machine, on CI, the error is completely different 🎲 😀

@Hotell Hotell added the Status: Blocked Resolution blocked by another issue label Feb 25, 2021
@Hotell Hotell marked this pull request as ready for review February 25, 2021 17:55
@chrisdholt
Copy link
Member

@chrisdholt Can you help with the web-components build error above?

@ecraig12345 @Hotell It looks like API extractor was (possibly) completed successfully. There are two commands running, can you confirm that running yarn doc:ci results in the above error? If it doesn't, try running yarn test-chrome:verbose and see if that's where the error is coming from.

That error is happening on my local machine, on CI, the error is completely different 🎲 😀

Sure, I'm just trying to confirm as I don't repro locally on my part so I'm trying to identify which part of the script it's coming from. If you run both of the above in isolation, which spurs the error?

@Hotell
Copy link
Contributor Author

Hotell commented Feb 25, 2021

@chrisdholt

yarn doc:ci

image

yarn test-chrome:verbose
image

all good.

Note I run yarn buildci when I got that error. So I don't think your codebase has anything to do with this. thanks for you quick reaction though.

@ecraig12345
Copy link
Member

Is it possible that this could be caused by different yarn installation layouts between local and CI, causing different dep versions to be used? We have the same API Extractor versions currently but different TS, and I'm not sure if it could vary which TS version API Extractor uses.

@Hotell Hotell force-pushed the hotell/16889/new-jest-config branch 2 times, most recently from 6ae178e to 4494020 Compare March 1, 2021 16:45
@Hotell
Copy link
Contributor Author

Hotell commented Mar 1, 2021

I found and fixed the issue (with temporary workarounds).

2 main issues:

utilities package

  • incompatible with TS strict version

make-styles package

  • custom resolution of path to add type dependency
  • using custom global ambient types scoped only for that particular package

Going forward all of this needs to be resolved, otherwise these kind of issues will constantly happen

  • TS strict mode on for every package in convergence deps tree
  • Proper resolution of missing types, without need to explicitly state those globals per package (for examample, in case of react-menu, this is quite confusing as we need to use global package that is not used at all , rather buried deep in one of those deps)

@Hotell Hotell removed the Status: Blocked Resolution blocked by another issue label Mar 1, 2021
@ling1726
Copy link
Member

ling1726 commented Mar 1, 2021

After #17197 utilities should no longer be used by any converged package

Comment on lines 24 to 25
"@fluentui/utilities": ["packages/utilities/src/index.ts"],
"@fluentui/merge-styles": ["packages/merge-styles/src/index.ts"],
"@fluentui/dom-utilities": ["packages/dom-utilities/src/index.ts"],
"@fluentui/theme": ["packages/theme/src/index.ts"],
"@fluentui/react-window-provider": ["packages/react-window-provider/src/index.ts"],
"@fluentui/react-focus-management": ["packages/react-focus-management/src/index.ts"],
"@fluentui/react-menu": ["packages/react-menu/src/index.ts"]
Copy link
Member

@layershifter layershifter Mar 1, 2021

Choose a reason for hiding this comment

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

Suggested change
"@fluentui/utilities": ["packages/utilities/src/index.ts"],
"@fluentui/merge-styles": ["packages/merge-styles/src/index.ts"],
"@fluentui/dom-utilities": ["packages/dom-utilities/src/index.ts"],
"@fluentui/theme": ["packages/theme/src/index.ts"],
"@fluentui/react-window-provider": ["packages/react-window-provider/src/index.ts"],
"@fluentui/react-focus-management": ["packages/react-focus-management/src/index.ts"],
"@fluentui/react-menu": ["packages/react-menu/src/index.ts"]
"@fluentui/react-focus-management": ["packages/react-focus-management/src/index.ts"],
"@fluentui/react-menu": ["packages/react-menu/src/index.ts"]

FYI: As #17197 have been merged (and @ling1726 pointed out), we don't need these packages for convergence, so you can remove these aliases if it will help you somehow 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's great :) wish I got this info sooner to save lot of time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw can you help me understand what can be removed in particular, when there are still those direct deps in react menu?

on the left: react-menu
image

Copy link
Member

@layershifter layershifter Mar 2, 2021

Choose a reason for hiding this comment

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

I mentioned packages that can be removed (are not part of converged packages) in a diff:

  • @fluentui/utilities
  • @fluentui/merge-styles
  • @fluentui/dom-utilities
  • @fluentui/theme
  • @fluentui/react-window-provider

Sorry, but I don't understand that screenshot, what package do you have on right? Is it @fluentui/react-make-styles? I don't see any dependencies from the list above either on master or on this branch in it.

"@fluentui/keyboard-key": "^0.2.14",
"@fluentui/react-focus-management": "^9.0.0-alpha.3",
"@fluentui/react-context-selector": "^0.53.3",
"@fluentui/react-make-styles": "^9.0.0-alpha.5",
"@fluentui/react-utilities": "^9.0.0-alpha.4",

"@fluentui/make-styles": "^9.0.0-alpha.2",
"@fluentui/react-provider": "^9.0.0-alpha.5",
"@fluentui/react-theme": "^9.0.0-alpha.4",
"@fluentui/react-theme-provider": "^9.0.0-alpha.5",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I don't understand that screenshot, what package do you have on right? Is it @fluentui/react-make-styles? I don't see any dependencies from the list above either on master or on this branch in it.

yes, sorry I thought that arrow is self explanatory.

those deps are still there (except set-version)

react-make-styles deps
image

@Hotell Hotell force-pushed the hotell/16889/new-jest-config branch from 88b0844 to d2570b4 Compare March 9, 2021 11:50
@Hotell Hotell removed the request for review from xugao March 9, 2021 12:42
@Hotell Hotell merged commit 860bb82 into microsoft:master Mar 11, 2021
ling1726 added a commit to ling1726/fluentui that referenced this pull request Mar 12, 2021
Recently microsoft#17161 changed `react-menu` to use native `jest` command for
testing which does not support the `--production` flag
@ling1726 ling1726 mentioned this pull request Mar 12, 2021
2 tasks
ecraig12345 pushed a commit that referenced this pull request Mar 12, 2021
Recently #17161 changed `react-menu` to use native `jest` command for
testing which does not support the `--production` flag
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

joshualamusga1 pushed a commit to joshualamusga1/fluentui that referenced this pull request Mar 25, 2021
…oft#17161)

* chore(react-menu): implement in memory test execution
* fix(react-conformance): remove esModuleInterop so the package is compliant with other packages
* fix(utilities): make pkg compile in strict mode
* chore(make-styles): propagate deps to root and remove path overrides so the package can be properly consumed by ts path aliases converged packages
* chore(react-menu): add global types from dependant packages until we properly define globals type resolution for all
* chore: remove packages from path aliases that have been removed from convergence
* Change files
joshualamusga1 pushed a commit to joshualamusga1/fluentui that referenced this pull request Mar 25, 2021
Recently microsoft#17161 changed `react-menu` to use native `jest` command for
testing which does not support the `--production` flag
miroslavstastny pushed a commit to miroslavstastny/fluentui that referenced this pull request May 5, 2021
…oft#17161)

* chore(react-menu): implement in memory test execution
* fix(react-conformance): remove esModuleInterop so the package is compliant with other packages
* fix(utilities): make pkg compile in strict mode
* chore(make-styles): propagate deps to root and remove path overrides so the package can be properly consumed by ts path aliases converged packages
* chore(react-menu): add global types from dependant packages until we properly define globals type resolution for all
* chore: remove packages from path aliases that have been removed from convergence
* Change files
miroslavstastny pushed a commit to miroslavstastny/fluentui that referenced this pull request May 5, 2021
Recently microsoft#17161 changed `react-menu` to use native `jest` command for
testing which does not support the `--production` flag
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.

8 participants