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: Add tests for babel-preset-global-context #23453

Merged

Conversation

ling1726
Copy link
Member

@ling1726 ling1726 commented Jun 8, 2022

Following #23435, adds an extended cypress config in
babel-preset-global-context with tests.

Following microsoft#23435, adds an extended cypress config in
babel-preset-global-context with tests.
@ling1726 ling1726 marked this pull request as ready for review June 8, 2022 09:48
@ling1726 ling1726 requested a review from a team as a code owner June 8, 2022 09:48
@fabricteam
Copy link
Collaborator

fabricteam commented Jun 8, 2022

📊 Bundle size report

🤖 This report was generated against 0d766ec7503d3313962e7a4045128456a04f1a34

@@ -14,6 +14,9 @@
"build": "just-scripts build --commonjs",
"clean": "just-scripts clean",
"code-style": "just-scripts code-style",
"pree2e": "yarn build",
Copy link
Member Author

@ling1726 ling1726 Jun 8, 2022

Choose a reason for hiding this comment

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

preset needs to exist as cjs before the test starts. Since this package has no dependencies within the monorepo, it should be OK to build this before testing

Copy link
Contributor

Choose a reason for hiding this comment

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

can we inline this with the e2e command or what is the reasoning behind this? IMO this doesn't add any additional clarity rather may add confusion + the name pree2e is not very explanatory. I know npm gurus will know that pre is a special pragma to tell npm to execute prior to particular alias but still

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 can inline it... I tried to be an npm guru :)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 8, 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 32f212d:

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

@size-auditor
Copy link

size-auditor bot commented Jun 8, 2022

Asset size changes

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

Baseline commit: 0d766ec7503d3313962e7a4045128456a04f1a34 (build)

@ling1726 ling1726 requested a review from a team as a code owner June 8, 2022 10:03
@@ -106,7 +106,7 @@ jobs:

# only run e2e tests when the appropriate storybook is published by scoping to relevant packages
- script: |
yarn e2e $(sinceArg) --scope @fluentui/react-components
yarn e2e $(sinceArg) --scope @fluentui/react-components,@fluentui/babel-preset-global-context
Copy link
Member Author

Choose a reason for hiding this comment

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

Created a new issue to track better scoping: #23455

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this at all ? I'd assume that we wanna run e2e task on every affected lib/app in tree that has such a task defined. can that be done or we have limited by tooling? this creates tight coupling which is "hard" to maintain from my experience.

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 that's a good point, I'll make a quick PR separately to make that change directly in master

Copy link
Member Author

Choose a reason for hiding this comment

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

done in #23460

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1156 1219 5000
Button mount 768 749 5000
FluentProvider mount 2227 2188 5000
FluentProviderWithTheme mount 359 371 10
FluentProviderWithTheme virtual-rerender 327 363 10
FluentProviderWithTheme virtual-rerender-with-unmount 400 421 10
MakeStyles mount 1850 1883 50000

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
TreeWith60ListItems.default 189 167 1.13:1
AttachmentMinimalPerf.default 172 160 1.08:1
RefMinimalPerf.default 256 238 1.08:1
FlexMinimalPerf.default 312 292 1.07:1
PortalMinimalPerf.default 185 177 1.05:1
IconMinimalPerf.default 693 658 1.05:1
TextMinimalPerf.default 398 379 1.05:1
AccordionMinimalPerf.default 168 162 1.04:1
AnimationMinimalPerf.default 607 582 1.04:1
BoxMinimalPerf.default 397 383 1.04:1
ButtonOverridesMissPerf.default 1688 1625 1.04:1
ListCommonPerf.default 738 711 1.04:1
LoaderMinimalPerf.default 773 742 1.04:1
MenuMinimalPerf.default 968 929 1.04:1
TableMinimalPerf.default 460 441 1.04:1
TextAreaMinimalPerf.default 587 566 1.04:1
AttachmentSlotsPerf.default 1231 1199 1.03:1
ImageMinimalPerf.default 433 422 1.03:1
LabelMinimalPerf.default 431 417 1.03:1
ListMinimalPerf.default 595 577 1.03:1
ReactionMinimalPerf.default 428 416 1.03:1
StatusMinimalPerf.default 785 764 1.03:1
TableManyItemsPerf.default 2256 2192 1.03:1
AlertMinimalPerf.default 305 300 1.02:1
ButtonMinimalPerf.default 189 186 1.02:1
ButtonSlotsPerf.default 600 587 1.02:1
HeaderMinimalPerf.default 405 398 1.02:1
SkeletonMinimalPerf.default 394 385 1.02:1
VideoMinimalPerf.default 730 715 1.02:1
CardMinimalPerf.default 645 638 1.01:1
CheckboxMinimalPerf.default 2963 2942 1.01:1
EmbedMinimalPerf.default 4585 4529 1.01:1
GridMinimalPerf.default 369 366 1.01:1
MenuButtonMinimalPerf.default 1926 1914 1.01:1
SegmentMinimalPerf.default 385 380 1.01:1
ToolbarMinimalPerf.default 1096 1081 1.01:1
TooltipMinimalPerf.default 1298 1284 1.01:1
DatepickerMinimalPerf.default 6337 6356 1:1
DividerMinimalPerf.default 398 399 1:1
DropdownManyItemsPerf.default 774 772 1:1
HeaderSlotsPerf.default 870 873 1:1
ListWith60ListItems.default 716 713 1:1
SplitButtonMinimalPerf.default 4879 4860 1:1
ChatDuplicateMessagesPerf.default 306 310 0.99:1
DialogMinimalPerf.default 851 857 0.99:1
DropdownMinimalPerf.default 3324 3347 0.99:1
LayoutMinimalPerf.default 388 390 0.99:1
RosterPerf.default 1243 1257 0.99:1
ProviderMergeThemesPerf.default 1303 1310 0.99:1
ProviderMinimalPerf.default 418 423 0.99:1
CustomToolbarPrototype.default 2909 2929 0.99:1
TreeMinimalPerf.default 905 917 0.99:1
CarouselMinimalPerf.default 526 535 0.98:1
ChatMinimalPerf.default 839 853 0.98:1
ChatWithPopoverPerf.default 445 453 0.98:1
InputMinimalPerf.default 1410 1438 0.98:1
PopupMinimalPerf.default 682 693 0.98:1
SliderMinimalPerf.default 1832 1861 0.98:1
FormMinimalPerf.default 465 478 0.97:1
ItemLayoutMinimalPerf.default 1323 1369 0.97:1
RadioGroupMinimalPerf.default 481 495 0.97:1
AvatarMinimalPerf.default 222 232 0.96:1
ListNestedPerf.default 631 679 0.93:1

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 973 988 5000
Breadcrumb mount 2745 2758 1000
Checkbox mount 1573 1516 5000
CheckboxBase mount 1302 1290 5000
ChoiceGroup mount 4862 4875 5000
ComboBox mount 1021 991 1000
CommandBar mount 10758 10763 1000
ContextualMenu mount 11806 11833 1000
DefaultButton mount 1191 1170 5000
DetailsRow mount 3941 3889 5000
DetailsRowFast mount 3985 3964 5000
DetailsRowNoStyles mount 3775 3786 5000
Dialog mount 2355 2340 1000
DocumentCardTitle mount 182 182 1000
Dropdown mount 3483 3366 5000
FocusTrapZone mount 1903 1850 5000
FocusZone mount 1840 1959 5000
IconButton mount 1821 1840 5000
Label mount 354 345 5000
Layer mount 3004 3043 5000
Link mount 471 481 5000
MenuButton mount 1498 1497 5000
MessageBar mount 2221 2104 5000
Nav mount 3383 3363 1000
OverflowSet mount 1104 1115 5000
Panel mount 2224 2218 1000
Persona mount 1012 1033 1000
Pivot mount 1493 1491 1000
PrimaryButton mount 1314 1356 5000
Rating mount 7939 7904 5000
SearchBox mount 1375 1359 5000
Shimmer mount 2559 2504 5000
Slider mount 1976 2001 5000
SpinButton mount 5199 5139 5000
Spinner mount 434 437 5000
SplitButton mount 3198 3319 5000
Stack mount 549 536 5000
StackWithIntrinsicChildren mount 2429 2386 5000
StackWithTextChildren mount 5379 5342 5000
SwatchColorPicker mount 12024 11910 5000
TagPicker mount 2747 2727 5000
TeachingBubble mount 94384 94257 5000
Text mount 433 447 5000
TextField mount 1453 1496 5000
ThemeProvider mount 1216 1248 5000
ThemeProvider virtual-rerender 689 655 5000
ThemeProvider virtual-rerender-with-unmount 1945 1933 5000
Toggle mount 797 826 5000
buttonNative mount 136 141 5000

Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

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

few comments/questions - not blocking

@@ -106,7 +106,7 @@ jobs:

# only run e2e tests when the appropriate storybook is published by scoping to relevant packages
- script: |
yarn e2e $(sinceArg) --scope @fluentui/react-components
yarn e2e $(sinceArg) --scope @fluentui/react-components,@fluentui/babel-preset-global-context
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this at all ? I'd assume that we wanna run e2e task on every affected lib/app in tree that has such a task defined. can that be done or we have limited by tooling? this creates tight coupling which is "hard" to maintain from my experience.

@@ -14,6 +14,9 @@
"build": "just-scripts build --commonjs",
"clean": "just-scripts clean",
"code-style": "just-scripts code-style",
"pree2e": "yarn build",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we inline this with the e2e command or what is the reasoning behind this? IMO this doesn't add any additional clarity rather may add confusion + the name pree2e is not very explanatory. I know npm gurus will know that pre is a special pragma to tell npm to execute prior to particular alias but still

@@ -0,0 +1,9 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

q: did you added this by running migration-converged-pkg generator ?

@ling1726 ling1726 merged commit 581f878 into microsoft:master Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants