chore: Northstar screener should read from screenerStates.json #24848

ling1726 · 2022-09-19T10:08:03Z

Screener is now separated into two workflows for secrets encapsulation, the code to trigger the screener run is actually run on the master branch.

Current Behavior

The current northstar screener states are generated directly before running, this means that any changes to N* screener tests in a PR will never result in updated screenshots

New Behavior

Update the screener:build gulp command to write all states to a json file and updeates the screener:runner gulp tasks to read states from the json file as a result of the build.

Since the entire docs/dist artifact is uploaded, there should be no changes needed for the pipeline

Related Issue(s)

Fixes #24842

Screener is now separated into two workflows for secrets encapsulation, the code to trigger the screener run is actually run on the master branch. The current northstar screener states are generated directly before running, this means that any changes to N* screener tests will never result in updated screenshots Update the `screener:build` gulp command to write all states to a json file and updeates the `screener:runner` gulp tasks to read states from the json file. Since the entire `docs/dist` artifact is uploaded, there should be no changes needed for the pipeline

ling1726 · 2022-09-19T10:08:30Z

apps/vr-tests-react-components/screener.config.js

@@ -24,7 +24,7 @@ function getCurrentHash() {
 * @param {string} options.sourceBranchName
 * @param {string} options.deployUrl
 * @param {string} options.targetBranch
- * @returns
+ * @returns {import('@fluentui/scripts/screener/screener.types').ScreenerRunnerConfig}


improve typings in screener.config.js files

this deep import is bit unfortunate. can we expose scripts/screener api from barrel file ? this whole directory would be move do to separate package later on.

Note: not needed for this PR but as a follow-up

ling1726 · 2022-09-19T10:08:49Z

scripts/screener/screener.types.ts

    structure: boolean;
    layout: boolean;
    style: boolean;
    content: boolean;
    minLayoutPosition: number; // Optional threshold for Layout changes. Defaults to 4 pixels.
    minLayoutDimension: number; // Optional threshold for Layout changes. Defaults to 10 pixels.
    minShiftGraphic: number; // Optional threshold for pixel shifts in graphics.
-    compareSVGDOM: number; // Pass if SVG DOM is the same. Defaults to false.
+    compareSVGDOM: boolean; // Pass if SVG DOM is the same. Defaults to false.


This was always typed incorrectly before applying the types to the screener.config.js

ling1726 · 2022-09-19T10:09:19Z

scripts/screener/screener.types.ts

  };

  states: ScreenerState[];

  alwaysAcceptBaseBranch: boolean;
  baseBranch: string;
-  commit: string;
+  commit?: string;


This should be optional because master builds do not require a commit

codesandbox-ci · 2022-09-19T10:18:16Z

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

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

size-auditor · 2022-09-19T10:23:05Z

Asset size changes

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

Baseline commit: 17278782ca2c1d4051389cf8741224cde6672e88 (build)

fabricteam · 2022-09-19T10:24:17Z

Perf Analysis (`@fluentui/react-components`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	1290	1300	5000
Button	mount	924	925	5000
FluentProvider	mount	1461	1496	5000
FluentProviderWithTheme	mount	580	570	10
FluentProviderWithTheme	virtual-rerender	533	545	10
FluentProviderWithTheme	virtual-rerender-with-unmount	575	576	10
MakeStyles	mount	1964	1970	50000
SpinButton	mount	2353	2333	5000

fabricteam · 2022-09-19T10:29:40Z

📊 Bundle size report

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`global-context` createContext	`533 B` `341 B`
`global-context` createContextSelector	`554 B` `348 B`
`priority-overflow` createOverflowManager	`2.936 kB` `1.212 kB`
`react-accordion` Accordion (including children components)	`78.914 kB` `24.06 kB`
`react-alert` Alert	`83.511 kB` `20.921 kB`
`react-avatar` Avatar	`48.381 kB` `13.696 kB`
`react-avatar` AvatarGroup	`14.95 kB` `5.989 kB`
`react-avatar` AvatarGroupItem	`68.349 kB` `19.039 kB`
`react-badge` Badge	`22.6 kB` `7.205 kB`
`react-badge` CounterBadge	`23.503 kB` `7.497 kB`
`react-badge` PresenceBadge	`24.05 kB` `7.067 kB`
`react-button` Button	`36.119 kB` `9.647 kB`
`react-button` CompoundButton	`43.144 kB` `10.86 kB`
`react-button` MenuButton	`38.813 kB` `10.551 kB`
`react-button` SplitButton	`46.228 kB` `11.933 kB`
`react-button` ToggleButton	`51.888 kB` `11.127 kB`
`react-card` Card - All	`67.002 kB` `19.261 kB`
`react-card` Card	`62.684 kB` `18.177 kB`
`react-card` CardFooter	`8.561 kB` `3.601 kB`
`react-card` CardHeader	`9.604 kB` `3.94 kB`
`react-card` CardPreview	`8.662 kB` `3.656 kB`
`react-combobox` Combobox (including child components)	`74.636 kB` `24.186 kB`
`react-combobox` Dropdown (including child components)	`74.236 kB` `24.086 kB`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`188.672 kB` `52.359 kB`
`react-components` react-components: FluentProvider & webLightTheme	`33.394 kB` `11.007 kB`
`react-dialog` Dialog (including children components)	`81.048 kB` `24.208 kB`
`react-divider` Divider	`16.459 kB` `5.902 kB`
`react-image` Image	`10.78 kB` `4.264 kB`
`react-input` Input	`23.757 kB` `7.704 kB`
`react-label` Label	`9.338 kB` `3.86 kB`
`react-link` Link	`11.784 kB` `4.867 kB`
`react-menu` Menu (including children components)	`116.572 kB` `35.778 kB`
`react-menu` Menu (including selectable components)	`119.641 kB` `36.297 kB`
`react-overflow` hooks only	`10.685 kB` `4.104 kB`
`react-popover` Popover	`102.963 kB` `31.553 kB`
`react-portal` Portal	`10.576 kB` `3.875 kB`
`react-portal-compat` PortalCompatProvider	`5.851 kB` `1.964 kB`
`react-positioning` usePositioning	`19.7 kB` `7.404 kB`
`react-provider` FluentProvider	`15.755 kB` `5.883 kB`
`react-radio` Radio	`35.56 kB` `11.929 kB`
`react-radio` RadioGroup	`14.248 kB` `5.7 kB`
`react-select` Select	`20.846 kB` `7.346 kB`
`react-slider` Slider	`31.526 kB` `10.046 kB`
`react-spinbutton` SpinButton	`44.102 kB` `12.425 kB`
`react-spinner` Spinner	`19.977 kB` `6.438 kB`
`react-switch` Switch	`32.097 kB` `10.27 kB`
`react-text` Text - Default	`11.782 kB` `4.605 kB`
`react-text` Text - Wrappers	`15.092 kB` `5.044 kB`
`react-textarea` Textarea	`25.013 kB` `8.133 kB`
`react-theme` Single theme token import	`69 B` `89 B`
`react-theme` Teams: all themes	`29.65 kB` `6.444 kB`
`react-theme` Teams: Light theme	`17.486 kB` `5.057 kB`
`react-tooltip` Tooltip	`41.502 kB` `14.623 kB`
`react-utilities` SSRProvider	`180 B` `159 B`

_{🤖 This report was generated against 17278782ca2c1d4051389cf8741224cde6672e88}

fabricteam · 2022-09-19T10:33:15Z

Perf Analysis (`@fluentui/react-northstar`)

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
CardMinimalPerf.default	648	573	1.13:1
AttachmentMinimalPerf.default	171	155	1.1:1
ButtonSlotsPerf.default	601	544	1.1:1
AvatarMinimalPerf.default	216	198	1.09:1
RefMinimalPerf.default	228	212	1.08:1
AlertMinimalPerf.default	290	272	1.07:1
ReactionMinimalPerf.default	423	397	1.07:1
StatusMinimalPerf.default	766	717	1.07:1
LoaderMinimalPerf.default	711	673	1.06:1
ImageMinimalPerf.default	435	415	1.05:1
RadioGroupMinimalPerf.default	501	475	1.05:1
LabelMinimalPerf.default	414	397	1.04:1
TableMinimalPerf.default	426	409	1.04:1
BoxMinimalPerf.default	364	352	1.03:1
ChatMinimalPerf.default	793	773	1.03:1
FormMinimalPerf.default	442	428	1.03:1
HeaderMinimalPerf.default	386	373	1.03:1
ListWith60ListItems.default	658	639	1.03:1
TreeMinimalPerf.default	891	867	1.03:1
DatepickerMinimalPerf.default	6065	5934	1.02:1
DropdownManyItemsPerf.default	752	735	1.02:1
DropdownMinimalPerf.default	2799	2757	1.02:1
ItemLayoutMinimalPerf.default	1284	1264	1.02:1
ListNestedPerf.default	618	604	1.02:1
MenuMinimalPerf.default	919	905	1.02:1
RosterPerf.default	2336	2280	1.02:1
PopupMinimalPerf.default	658	642	1.02:1
CustomToolbarPrototype.default	2748	2702	1.02:1
ButtonMinimalPerf.default	181	179	1.01:1
ChatDuplicateMessagesPerf.default	303	299	1.01:1
EmbedMinimalPerf.default	3986	3945	1.01:1
FlexMinimalPerf.default	297	295	1.01:1
ListCommonPerf.default	702	696	1.01:1
MenuButtonMinimalPerf.default	1828	1808	1.01:1
ProviderMinimalPerf.default	400	397	1.01:1
SegmentMinimalPerf.default	384	380	1.01:1
TableManyItemsPerf.default	2136	2115	1.01:1
AttachmentSlotsPerf.default	1169	1166	1:1
ButtonOverridesMissPerf.default	1431	1436	1:1
HeaderSlotsPerf.default	824	827	1:1
LayoutMinimalPerf.default	380	380	1:1
ListMinimalPerf.default	544	542	1:1
IconMinimalPerf.default	697	694	1:1
TextMinimalPerf.default	371	371	1:1
TreeWith60ListItems.default	170	170	1:1
CheckboxMinimalPerf.default	2197	2217	0.99:1
GridMinimalPerf.default	364	368	0.99:1
PortalMinimalPerf.default	167	168	0.99:1
ProviderMergeThemesPerf.default	1255	1273	0.99:1
SkeletonMinimalPerf.default	372	374	0.99:1
SliderMinimalPerf.default	1683	1697	0.99:1
ToolbarMinimalPerf.default	996	1008	0.99:1
VideoMinimalPerf.default	766	775	0.99:1
AnimationMinimalPerf.default	547	559	0.98:1
DividerMinimalPerf.default	373	381	0.98:1
InputMinimalPerf.default	1180	1207	0.98:1
SplitButtonMinimalPerf.default	4681	4774	0.98:1
TooltipMinimalPerf.default	2437	2485	0.98:1
AccordionMinimalPerf.default	143	148	0.97:1
CarouselMinimalPerf.default	486	499	0.97:1
TextAreaMinimalPerf.default	522	536	0.97:1
ChatWithPopoverPerf.default	389	409	0.95:1
DialogMinimalPerf.default	805	845	0.95:1

ling1726 · 2022-09-19T10:39:13Z

scripts/gulp/tasks/vr-build.ts

@@ -0,0 +1,21 @@
+import { task, series } from 'gulp';


Split up screener.ts into vr-test.ts and vr-build.ts to better reflect the commands since they are actually used as vr:build and vr:test.

The two steps have nothing in common with each other either

ling1726 · 2022-09-19T10:39:52Z

scripts/screener/screener.states.ts

-
-    // https://www.npmjs.com/package/screener-runner#testing-interactions
-    steps: getScreenerSteps(pageUrl, `${exampleDir}/${exampleNameWithoutExtension}.steps`),
+export default function getScreenerStates() {


This file used to require all of northstar, refactored this to be a factory to only require northstar when needed

suggestion: can we stop using/introducing new default exports ? not explicit API contracts / subpar DX ....

the logic for particular library should not live here as it introduces tight coupling. I understand this PR purpose is to quick fix current behaviours, but would be great if we can come up with long term solution.

fabricteam · 2022-09-19T10:55:22Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
BaseButton	mount	1460	1447	5000
Breadcrumb	mount	3500	3575	1000
Checkbox	mount	3176	3198	5000
CheckboxBase	mount	2856	2804	5000
ChoiceGroup	mount	5408	5184	5000
ComboBox	mount	1495	1511	1000
CommandBar	mount	11661	11584	1000
ContextualMenu	mount	13392	13875	1000
DefaultButton	mount	1643	1620	5000
DetailsRow	mount	4423	4273	5000
DetailsRowFast	mount	4265	4259	5000
DetailsRowNoStyles	mount	4163	4258	5000
Dialog	mount	3686	3652	1000
DocumentCardTitle	mount	713	699	1000
Dropdown	mount	3846	3926	5000
FocusTrapZone	mount	2458	2443	5000
FocusZone	mount	2318	2410	5000
GroupedList	mount	63763	71930	2
GroupedList	virtual-rerender	29755	30235	2
GroupedList	virtual-rerender-with-unmount	110747	110896	2
GroupedListV2	mount	647	661	2
GroupedListV2	virtual-rerender	627	636	2
GroupedListV2	virtual-rerender-with-unmount	674	692	2
IconButton	mount	2340	2334	5000
Label	mount	864	876	5000
Layer	mount	5222	5136	5000
Link	mount	1013	1030	5000
MenuButton	mount	2043	2018	5000
MessageBar	mount	2817	2787	5000
Nav	mount	3922	3997	1000
OverflowSet	mount	1682	1678	5000
Panel	mount	2979	3049	1000
Persona	mount	1512	1550	1000
Pivot	mount	1976	2017	1000
PrimaryButton	mount	1823	1866	5000
Rating	mount	8477	8507	5000
SearchBox	mount	1868	1830	5000
Shimmer	mount	3498	3538	5000
Slider	mount	2535	2503	5000
SpinButton	mount	5595	5544	5000
Spinner	mount	978	960	5000
SplitButton	mount	3837	3710	5000
Stack	mount	1060	1044	5000
StackWithIntrinsicChildren	mount	2787	2787	5000
StackWithTextChildren	mount	5828	5902	5000
SwatchColorPicker	mount	12797	12589	5000
TagPicker	mount	3125	3206	5000
TeachingBubble	mount	104258	102468	5000
Text	mount	987	960	5000
TextField	mount	1901	1918	5000
ThemeProvider	mount	1852	1876	5000
ThemeProvider	virtual-rerender	1323	1270	5000
ThemeProvider	virtual-rerender-with-unmount	2607	2596	5000
Toggle	mount	1342	1309	5000
buttonNative	mount	660	669	5000

Hotell · 2022-09-19T10:52:49Z

apps/vr-tests-react-components/screener.config.js

@@ -24,7 +24,7 @@ function getCurrentHash() {
 * @param {string} options.sourceBranchName
 * @param {string} options.deployUrl
 * @param {string} options.targetBranch
- * @returns
+ * @returns {import('@fluentui/scripts/screener/screener.types').ScreenerRunnerConfig}


this deep import is bit unfortunate. can we expose scripts/screener api from barrel file ? this whole directory would be move do to separate package later on.

Note: not needed for this PR but as a follow-up

Hotell · 2022-09-19T10:55:35Z

scripts/gulp/tasks/vr-build.ts

+import fs from 'fs';
+
+import config from '../../config';
+import getScreenerStates from '../../screener/screener.states';


same public api chore like in former comment

Hotell · 2022-09-19T10:57:48Z

scripts/screener/screener.states.ts

-
-    // https://www.npmjs.com/package/screener-runner#testing-interactions
-    steps: getScreenerSteps(pageUrl, `${exampleDir}/${exampleNameWithoutExtension}.steps`),
+export default function getScreenerStates() {


suggestion: can we stop using/introducing new default exports ? not explicit API contracts / subpar DX ....

Hotell · 2022-09-19T11:00:16Z

scripts/screener/screener.states.ts

-
-    // https://www.npmjs.com/package/screener-runner#testing-interactions
-    steps: getScreenerSteps(pageUrl, `${exampleDir}/${exampleNameWithoutExtension}.steps`),
+export default function getScreenerStates() {


the logic for particular library should not live here as it introduces tight coupling. I understand this PR purpose is to quick fix current behaviours, but would be great if we can come up with long term solution.

* master: (28 commits) fix: use trigger prop for aria-haspopup (microsoft#24794) chore(react-dialog): scaffold DialogContent component (microsoft#24844) chore: Northstar screener should read from screenerStates.json (microsoft#24848) applying package updates (web components) Standardize focus treatment (microsoft#24771) Divider - allow default prop override (microsoft#24840) GroupedList: fix virtualization (unstable preview) (microsoft#24460) fix: Add explicit children prop to TeachingBubble to support React 18 (microsoft#24823) feat: Adds `visible` prop to `TableCellActions` (microsoft#24831) [Northstar][Dropdown] Fix styling mutation when merging themes (microsoft#24787) fix: export `tableCellActionsClassNames` from unstable (microsoft#24830) bugfix(react-dialog): Adds color style to DialogSurface (microsoft#24832) applying package updates Prevent group toggling from selecting the whole group (microsoft#24822) feat(react-textarea): Add shadow variant of filled appearance (microsoft#24512) applying package updates Adding lib-commonjs top-level entries to exports map (microsoft#24792) Created shim packages (microsoft#24780) feat(react-menu): replace keydown handlers by useARIAButtonShorthand on MenuItem (microsoft#24738) fix: update version mismatches triggered by v9 release (microsoft#24812) ...

layershifter · 2022-09-20T14:53:58Z

@ling1726

https://github.com/microsoft/fluentui/actions/runs/3090722190/jobs/4999892722

#24848)" This reverts commit d83195c.

msft-fluent-ui-bot assigned tomi-msft Sep 19, 2022

github-actions bot added this to the July Project Cycle Q3 2022 milestone Sep 19, 2022

ling1726 commented Sep 19, 2022

View reviewed changes

ling1726 marked this pull request as ready for review September 19, 2022 10:10

ling1726 requested review from a team as code owners September 19, 2022 10:10

fix danger

393af13

refactored screener states and gulp tasks

453a445

ling1726 commented Sep 19, 2022

View reviewed changes

ling1726 added 2 commits September 19, 2022 12:43

add docs build

8ee3bbc

fix to build:docs

ebde122

miroslavstastny approved these changes Sep 19, 2022

View reviewed changes

Hotell approved these changes Sep 19, 2022

View reviewed changes

Hotell unassigned tomi-msft Sep 19, 2022

Hotell removed this from the July Project Cycle Q3 2022 milestone Sep 19, 2022

ling1726 merged commit d83195c into microsoft:master Sep 19, 2022

layershifter added a commit that referenced this pull request Sep 20, 2022

Revert "chore: Northstar screener should read from screenerStates.json (

975ab60

#24848)" This reverts commit d83195c.

layershifter mentioned this pull request Sep 20, 2022

Revert "chore: Northstar screener should read from screenerStates.json" #24866

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Northstar screener should read from screenerStates.json #24848

chore: Northstar screener should read from screenerStates.json #24848

ling1726 commented Sep 19, 2022 •

edited

Loading

ling1726 Sep 19, 2022

Hotell Sep 19, 2022

ling1726 Sep 19, 2022

ling1726 Sep 19, 2022

codesandbox-ci bot commented Sep 19, 2022 •

edited

Loading

size-auditor bot commented Sep 19, 2022 •

edited

Loading

fabricteam commented Sep 19, 2022 •

edited

Loading

fabricteam commented Sep 19, 2022 •

edited

Loading

fabricteam commented Sep 19, 2022 •

edited

Loading

ling1726 Sep 19, 2022

ling1726 Sep 19, 2022

Hotell Sep 19, 2022

Hotell Sep 19, 2022

fabricteam commented Sep 19, 2022 •

edited

Loading

Hotell Sep 19, 2022

Hotell Sep 19, 2022

Hotell Sep 19, 2022

Hotell Sep 19, 2022

layershifter commented Sep 20, 2022

chore: Northstar screener should read from screenerStates.json #24848

chore: Northstar screener should read from screenerStates.json #24848

Conversation

ling1726 commented Sep 19, 2022 • edited Loading

Current Behavior

New Behavior

Related Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codesandbox-ci bot commented Sep 19, 2022 • edited Loading

size-auditor bot commented Sep 19, 2022 • edited Loading

Asset size changes

fabricteam commented Sep 19, 2022 • edited Loading

fabricteam commented Sep 19, 2022 • edited Loading

📊 Bundle size report

fabricteam commented Sep 19, 2022 • edited Loading

Perf Analysis (@fluentui/react-northstar)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabricteam commented Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Sep 20, 2022

ling1726 commented Sep 19, 2022 •

edited

Loading

codesandbox-ci bot commented Sep 19, 2022 •

edited

Loading

size-auditor bot commented Sep 19, 2022 •

edited

Loading

fabricteam commented Sep 19, 2022 •

edited

Loading

fabricteam commented Sep 19, 2022 •

edited

Loading

fabricteam commented Sep 19, 2022 •

edited

Loading

Perf Analysis (`@fluentui/react-northstar`)

fabricteam commented Sep 19, 2022 •

edited

Loading