Update northstar screener runner to scope runs #21650

ling1726 · 2022-02-08T11:35:22Z

Current Behavior

Every PR will trigger a screener run

New Behavior

Only PRs that result in a change in @fluentui/docs will trigger a screener run.

Scoping is done with lage

Related Issue(s)

Fixes #

codesandbox-ci · 2022-02-08T11:39:48Z

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

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

size-auditor · 2022-02-08T11:48:20Z

Asset size changes

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

Baseline commit: 3c71bc76cfffde8800964edb980c02f8e410310f (build)

fabricteam · 2022-02-08T11:56:03Z

📊 Bundle size report

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-accordion` Accordion (including children components)	`54.905 kB` `17.567 kB`
`react-avatar` Avatar	`44.339 kB` `12.767 kB`
`react-badge` Badge	`20.831 kB` `6.533 kB`
`react-badge` CounterBadge	`21.699 kB` `6.827 kB`
`react-badge` PresenceBadge	`21.785 kB` `6.507 kB`
`react-button` Button	`27.932 kB` `8.025 kB`
`react-button` CompoundButton	`33.196 kB` `8.987 kB`
`react-button` MenuButton	`29.584 kB` `8.598 kB`
`react-button` SplitButton	`35.729 kB` `9.676 kB`
`react-button` ToggleButton	`37.209 kB` `8.627 kB`
`react-card` Card - All	`48.512 kB` `14.352 kB`
`react-card` Card	`43.987 kB` `13.188 kB`
`react-card` CardFooter	`7.615 kB` `3.23 kB`
`react-card` CardHeader	`8.893 kB` `3.675 kB`
`react-card` CardPreview	`7.806 kB` `3.357 kB`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`165.717 kB` `46.859 kB`
`react-components` react-components: FluentProvider & webLightTheme	`32.479 kB` `10.625 kB`
`react-divider` Divider	`15.256 kB` `5.511 kB`
`react-image` Image	`10.067 kB` `3.934 kB`
`react-input` Input	`21.5 kB` `7.116 kB`
`react-label` Label	`8.303 kB` `3.472 kB`
`react-link` Link	`11.064 kB` `4.487 kB`
`react-menu` Menu (including children components)	`102.89 kB` `31.719 kB`
`react-menu` Menu (including selectable components)	`105.245 kB` `32.089 kB`
`react-popover` Popover	`95.816 kB` `29.246 kB`
`react-portal` Portal	`6.249 kB` `2.163 kB`
`react-positioning` usePopper	`22.808 kB` `7.935 kB`
`react-provider` FluentProvider	`13.962 kB` `5.231 kB`
`react-slider` Slider	`22.928 kB` `7.747 kB`
`react-spinner` Spinner	`6.773 kB` `2.879 kB`
`react-switch` Switch	`25.387 kB` `8.204 kB`
`react-text` Text - Default	`10.755 kB` `4.212 kB`
`react-text` Text - Wrappers	`14.067 kB` `4.558 kB`
`react-theme` Single theme token import	`69 B` `89 B`
`react-theme` Teams: all themes	`29.426 kB` `6.551 kB`
`react-theme` Teams: Light theme	`18.42 kB` `5.27 kB`
`react-tooltip` Tooltip	`42.455 kB` `14.61 kB`
`react-utilities` SSRProvider	`189 B` `161 B`

_{🤖 This report was generated against 3c71bc76cfffde8800964edb980c02f8e410310f}

layershifter · 2022-02-08T11:57:02Z

scripts/screener/screener.runner.ts

+  // https://github.com/microsoft/azure-pipelines-tasks/issues/9801
+  const commit = process.env.SYSTEM_PULLREQUEST_SOURCECOMMITID;
+
+  await notifyIntegration({ commit, url: '', status: 'completed', project: screenerConfig.projectRepo });


I don't how it's implemented, but can we use conclusion: skipped? IMO it will be clearer what happened, for example:

https://docs.github.com/en/rest/reference/checks#create-a-check-run

I agree with this but would bring this up later in a new PR as a new feature as what's implemented here is the current behavior. Also because the proxy doesn't just propagate the status value into the Github API (I don't think) and that would required changes on the proxy code first, but correct me if I'm wrong @ling1726.

@layershifter it all depends on if our branch protection counts skipped as a valid status to merge the PR :)

Can we screenshare to check ? because I don't have access to branch protection

@ling1726 as I know Github does not allow to configure statuses for checks in repository settings.

I tested this in PR #21652... it seems to pass the status check. I can add it, but again the only way we'll know is if it goes into master :D

so the lesson:

cancelled will not pass the check

skipped will pass the check

andrefcdias · 2022-02-08T11:55:43Z

scripts/gulp/tasks/screener.ts

+  if (changedPackages.has('@fluentui/docs')) {
+    handlePromiseExit(screenerRunner(screenerConfigPath));
+  } else {
+    handlePromiseExit(cancelScreenerRun(screenerConfigPath));
+  }


Good addition 👍🏻

andrefcdias · 2022-02-08T12:02:52Z

scripts/screener/screener.runner.ts

+  // https://github.com/microsoft/azure-pipelines-tasks/issues/9801
+  const commit = process.env.SYSTEM_PULLREQUEST_SOURCECOMMITID;
+
+  await notifyIntegration({ commit, url: '', status: 'completed', project: screenerConfig.projectRepo });


I agree with this but would bring this up later in a new PR as a new feature as what's implemented here is the current behavior. Also because the proxy doesn't just propagate the status value into the Github API (I don't think) and that would required changes on the proxy code first, but correct me if I'm wrong @ling1726.

fabricteam · 2022-02-08T12:05:38Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	924	823	5000
BaseButton	mount	1025	1005	5000
Breadcrumb	mount	2860	2750	1000
ButtonNext	mount	509	493	5000
Checkbox	mount	1638	1778	5000
CheckboxBase	mount	1497	1435	5000
ChoiceGroup	mount	5195	5227	5000
ComboBox	mount	1091	1097	1000
CommandBar	mount	11275	11381	1000
ContextualMenu	mount	8891	9042	1000
DefaultButton	mount	1211	1238	5000
DetailsRow	mount	4217	3966	5000
DetailsRowFast	mount	3977	4193	5000
DetailsRowNoStyles	mount	3961	3793	5000
Dialog	mount	3163	3129	1000
DocumentCardTitle	mount	214	227	1000
Dropdown	mount	3561	3646	5000
FluentProviderNext	mount	2077	2098	5000
FluentProviderWithTheme	mount	178	165	10
FluentProviderWithTheme	virtual-rerender	129	138	10
FluentProviderWithTheme	virtual-rerender-with-unmount	219	200	10
FocusTrapZone	mount	1900	2014	5000
FocusZone	mount	2055	1941	5000
IconButton	mount	1773	1950	5000
Label	mount	407	427	5000
Layer	mount	3201	3083	5000
Link	mount	545	571	5000
MakeStyles	mount	1920	1968	50000
MenuButton	mount	1520	1790	5000
MessageBar	mount	2203	2251	5000
Nav	mount	3511	3780	1000
OverflowSet	mount	1288	1257	5000
Panel	mount	2590	2816	1000
Persona	mount	1015	988	1000
Pivot	mount	1693	1717	1000
PrimaryButton	mount	1389	1437	5000
Rating	mount	8532	8641	5000
SearchBox	mount	1595	1412	5000
Shimmer	mount	2944	2780	5000
Slider	mount	2070	2174	5000
SpinButton	mount	5675	5578	5000
Spinner	mount	466	457	5000
SplitButton	mount	3659	3394	5000
Stack	mount	563	553	5000
StackWithIntrinsicChildren	mount	2456	2490	5000
StackWithTextChildren	mount	5734	5535	5000
SwatchColorPicker	mount	12312	12350	5000
TagPicker	mount	2940	2865	5000
TeachingBubble	mount	14113	14253	5000
Text	mount	438	452	5000
TextField	mount	1598	1471	5000
ThemeProvider	mount	1384	1354	5000
ThemeProvider	virtual-rerender	647	642	5000
ThemeProvider	virtual-rerender-with-unmount	1980	2144	5000
Toggle	mount	1001	906	5000
buttonNative	mount	181	186	5000

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

⚠️ No perf measurements available

Hotell · 2022-02-08T11:42:14Z

scripts/screener/screener.types.ts

@@ -88,3 +88,10 @@ export type ScreenerTestsConfig = {
  steps?: ScreenerStep[];
  themes?: ScreenerThemeName[];
 };
+
+export interface ScreenerProxyPayload {


q: can we (would it make sense to) export this from the proxy package we have ?

our current screener proxy is just a node server app. It's possible to publish package for this from the ADO repo, but it would need to be setup to publish and then turn the screener-proxy repo into a monorepo.... I can create an issue for a follow up, but I would say not in the scope of current work.

created #21655 to follow up and added it to the build board

scripts/monorepo/getChangedPackages.js

scripts/screener/screener.runner.ts

scripts/gulp/tasks/screener.ts

scripts/monorepo/getChangedPackages.js

scripts/screener/screener.types.ts

ecraig12345 · 2022-02-09T00:21:14Z

scripts/monorepo/getChangedPackages.js

+ * @param {string} since - Commit to compare against
+ * @returns {Set<string>} - Set of packages that are changed
+ */
+function getChangedPackages(since = 'origin/master') {


This already exists in workspace-tools, so you should probably just import it from there instead of re-implementing.

The workspace-tools one is different, I'm using Lage here to get the changed packages and their dependents... maybe I should rename this ?

I'll rename this to getAffectedPacakges

ecraig12345 · 2022-02-09T00:22:12Z

scripts/gulp/tasks/screener.ts

+      .then(() => {
+        cb();
+        process.exit(0);
+      })


I don't think it's necessary to explicitly exit 0? If so, can you just do this?

Suggested change

.then(() => {

cb();

process.exit(0);

})

.then(cb)

I'm not entirely sure why this was done for gulp tbh.... it was the previous behaviour and I don't want to refactor this now

Gulp doesn't handle exit codes properly as you can see here gulpjs/gulp#2081

Ah I hadn't noticed that this was the old behavior. Thanks for the background.

ling1726 added 2 commits January 24, 2022 12:10

chore: Update screener runner to run only if there are changes

cc42862

Merge branch 'master' into chore/update-screener-runner

19bb354

msft-fluent-ui-bot added the Area: Build System label Feb 8, 2022

msft-fluent-ui-bot assigned TristanWatanabe Feb 8, 2022

ling1726 marked this pull request as ready for review February 8, 2022 11:36

ling1726 requested a review from a team as a code owner February 8, 2022 11:36

layershifter reviewed Feb 8, 2022

View reviewed changes

andrefcdias approved these changes Feb 8, 2022

View reviewed changes

Hotell reviewed Feb 8, 2022

View reviewed changes

Hotell added this to the February Project Cycle Q1 2022 milestone Feb 8, 2022

ling1726 added 2 commits February 8, 2022 16:06

use conclusion

710e490

pr suggestion

63f3946

layershifter approved these changes Feb 8, 2022

View reviewed changes

ling1726 added 4 commits February 8, 2022 17:23

use shell true

742c2a7

add pckage name check

d36ad2a

better check

bff6988

update error msg

61b0a67

ecraig12345 reviewed Feb 9, 2022

View reviewed changes

rename to getAffectedPackages

d5ccbc1

ling1726 merged commit 1947b26 into microsoft:master Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update northstar screener runner to scope runs #21650

Update northstar screener runner to scope runs #21650

ling1726 commented Feb 8, 2022

codesandbox-ci bot commented Feb 8, 2022 •

edited

Loading

size-auditor bot commented Feb 8, 2022 •

edited

Loading

fabricteam commented Feb 8, 2022 •

edited

Loading

layershifter Feb 8, 2022

andrefcdias Feb 8, 2022

ling1726 Feb 8, 2022

layershifter Feb 8, 2022

ling1726 Feb 8, 2022

ling1726 Feb 8, 2022

andrefcdias Feb 8, 2022

andrefcdias Feb 8, 2022

fabricteam commented Feb 8, 2022 •

edited

Loading

Hotell Feb 8, 2022

ling1726 Feb 8, 2022

ling1726 Feb 8, 2022

ecraig12345 Feb 9, 2022

ling1726 Feb 9, 2022

ling1726 Feb 9, 2022

ecraig12345 Feb 9, 2022

ling1726 Feb 9, 2022

ling1726 Feb 9, 2022

ecraig12345 Feb 10, 2022

Update northstar screener runner to scope runs #21650

Update northstar screener runner to scope runs #21650

Conversation

ling1726 commented Feb 8, 2022

Current Behavior

New Behavior

Related Issue(s)

codesandbox-ci bot commented Feb 8, 2022 • edited Loading

size-auditor bot commented Feb 8, 2022 • edited Loading

Asset size changes

fabricteam commented Feb 8, 2022 • edited Loading

📊 Bundle size report

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

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 Feb 8, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codesandbox-ci bot commented Feb 8, 2022 •

edited

Loading

size-auditor bot commented Feb 8, 2022 •

edited

Loading

fabricteam commented Feb 8, 2022 •

edited

Loading

fabricteam commented Feb 8, 2022 •

edited

Loading

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