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 SSR tests (test command) #23457

Merged
merged 16 commits into from
Jun 23, 2022

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Jun 8, 2022

New Behavior

This PR finished implementation of #22788 and adds test command:

$ yarn test
Starting a browser...
Using HeadlessChrome/103.0.5058.0
Using "file://apps/ssr-tests-v9/dist/index.html"
Test finished successfully in 2.26s

The command uses assets that were built with build command, runs them in a browser and checks console.

If test fails it will emit an error:

image

@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 a3540fe:

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

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 8, 2022

📊 Bundle size report

🤖 This report was generated against c76e61e2a74f8e6b3741a4615f7241658db53be3

@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: c76e61e2a74f8e6b3741a4615f7241658db53be3 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 8, 2022

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
HeaderMinimalPerf.default 454 401 1.13:1
AttachmentMinimalPerf.default 200 178 1.12:1
AnimationMinimalPerf.default 676 615 1.1:1
RefMinimalPerf.default 273 250 1.09:1
ChatDuplicateMessagesPerf.default 345 326 1.06:1
HeaderSlotsPerf.default 912 862 1.06:1
TreeWith60ListItems.default 188 178 1.06:1
ChatMinimalPerf.default 889 849 1.05:1
SliderMinimalPerf.default 1986 1894 1.05:1
AccordionMinimalPerf.default 174 168 1.04:1
AlertMinimalPerf.default 322 310 1.04:1
DividerMinimalPerf.default 433 417 1.04:1
FormMinimalPerf.default 485 465 1.04:1
ChatWithPopoverPerf.default 449 437 1.03:1
CheckboxMinimalPerf.default 3142 3045 1.03:1
LoaderMinimalPerf.default 808 788 1.03:1
PortalMinimalPerf.default 195 189 1.03:1
AvatarMinimalPerf.default 231 226 1.02:1
ButtonSlotsPerf.default 646 632 1.02:1
CardMinimalPerf.default 681 670 1.02:1
FlexMinimalPerf.default 328 323 1.02:1
MenuMinimalPerf.default 1007 984 1.02:1
SplitButtonMinimalPerf.default 5242 5126 1.02:1
TextMinimalPerf.default 395 388 1.02:1
TextAreaMinimalPerf.default 592 578 1.02:1
TreeMinimalPerf.default 969 952 1.02:1
ItemLayoutMinimalPerf.default 1409 1389 1.01:1
PopupMinimalPerf.default 719 715 1.01:1
ProviderMergeThemesPerf.default 1398 1389 1.01:1
TableManyItemsPerf.default 2322 2288 1.01:1
TableMinimalPerf.default 457 452 1.01:1
DropdownManyItemsPerf.default 802 799 1:1
InputMinimalPerf.default 1460 1467 1:1
ListMinimalPerf.default 611 610 1:1
ProviderMinimalPerf.default 446 448 1:1
ToolbarMinimalPerf.default 1122 1119 1:1
TooltipMinimalPerf.default 1343 1347 1:1
AttachmentSlotsPerf.default 1265 1281 0.99:1
ButtonOverridesMissPerf.default 1725 1744 0.99:1
DatepickerMinimalPerf.default 6737 6782 0.99:1
ListWith60ListItems.default 716 726 0.99:1
MenuButtonMinimalPerf.default 1989 2015 0.99:1
RadioGroupMinimalPerf.default 524 531 0.99:1
SkeletonMinimalPerf.default 396 400 0.99:1
IconMinimalPerf.default 718 726 0.99:1
CustomToolbarPrototype.default 3013 3036 0.99:1
VideoMinimalPerf.default 749 754 0.99:1
CarouselMinimalPerf.default 553 564 0.98:1
DialogMinimalPerf.default 898 912 0.98:1
DropdownMinimalPerf.default 3499 3557 0.98:1
EmbedMinimalPerf.default 4681 4757 0.98:1
GridMinimalPerf.default 385 392 0.98:1
LayoutMinimalPerf.default 407 415 0.98:1
ListNestedPerf.default 667 682 0.98:1
ReactionMinimalPerf.default 444 454 0.98:1
BoxMinimalPerf.default 391 403 0.97:1
LabelMinimalPerf.default 449 462 0.97:1
ListCommonPerf.default 732 753 0.97:1
RosterPerf.default 1305 1350 0.97:1
StatusMinimalPerf.default 812 841 0.97:1
ImageMinimalPerf.default 435 453 0.96:1
ButtonMinimalPerf.default 192 205 0.94:1
SegmentMinimalPerf.default 386 418 0.92:1

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 8, 2022

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 1067 1078 5000
Breadcrumb mount 3039 3033 1000
Checkbox mount 1763 1791 5000
CheckboxBase mount 1536 1536 5000
ChoiceGroup mount 5461 5449 5000
ComboBox mount 1129 1151 1000
CommandBar mount 11522 11562 1000
ContextualMenu mount 12453 12362 1000
DefaultButton mount 1342 1340 5000
DetailsRow mount 4388 4317 5000
DetailsRowFast mount 4492 4438 5000
DetailsRowNoStyles mount 4151 4163 5000
Dialog mount 3163 3231 1000
DocumentCardTitle mount 218 215 1000
Dropdown mount 3890 3861 5000
FocusTrapZone mount 2091 2153 5000
FocusZone mount 1987 2096 5000
IconButton mount 1986 2049 5000
Label mount 387 385 5000
Layer mount 3302 3314 5000
Link mount 543 575 5000
MenuButton mount 1772 1735 5000
MessageBar mount 2318 2372 5000
Nav mount 3710 3832 1000
OverflowSet mount 1245 1271 5000
Panel mount 2414 2408 1000
Persona mount 1137 1155 1000
Pivot mount 1652 1619 1000
PrimaryButton mount 1544 1524 5000
Rating mount 9094 9215 5000
SearchBox mount 1545 1636 5000
Shimmer mount 2925 2874 5000
Slider mount 2212 2189 5000
SpinButton mount 5734 5760 5000
Spinner mount 477 498 5000
SplitButton mount 3685 3633 5000
Stack mount 614 612 5000
StackWithIntrinsicChildren mount 2738 2850 5000
StackWithTextChildren mount 6531 6384 5000
SwatchColorPicker mount 13166 13383 5000
TagPicker mount 3222 3152 5000
TeachingBubble mount 98561 97778 5000
Text mount 503 505 5000
TextField mount 1604 1651 5000
ThemeProvider mount 1318 1320 5000
ThemeProvider virtual-rerender 697 694 5000
ThemeProvider virtual-rerender-with-unmount 2175 2151 5000
Toggle mount 927 904 5000
buttonNative mount 134 143 5000

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 13, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1372 1367 5000
Button mount 997 1058 5000
FluentProvider mount 2332 2320 5000
FluentProviderWithTheme mount 686 669 10
FluentProviderWithTheme virtual-rerender 673 645 10
FluentProviderWithTheme virtual-rerender-with-unmount 739 749 10
MakeStyles mount 2092 2058 50000

@layershifter layershifter force-pushed the chore/add-ssr-tests-test branch from ff8a7c2 to b37e315 Compare June 13, 2022 10:33
@@ -2,6 +2,7 @@
"extends": "../../tsconfig.base.json",
"compilerOptions": {
"target": "ES2019",
"allowSyntheticDefaultImports": true,
Copy link
Member Author

Choose a reason for hiding this comment

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

Without it chalk is broken.

import chalk from 'chalk';
src/test.ts:1:8 - error TS1259: Module '"/Users/olfedias/WebstormProjects/office-ui-fabric-react/node_modules/chalk/index"' can only be default-imported using the 'allowSyntheticDefaultImports' flag

1 import chalk from 'chalk';
         ~~~~~

  ../../node_modules/chalk/index.d.ts:415:1
    415 export = chalk;
        ~~~~~~~~~~~~~~~
    This module is declared with using 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag.

import * as chalk from 'chalk';
/apps/ssr-tests-v9/src/test.ts:68
console.log(chalk.bgRed.whiteBright(' @fluentui/ssr-tests-v9 '));
                        ^
TypeError: Cannot read properties of undefined (reading 'whiteBright')
    at Object.<anonymous> (/apps/ssr-tests-v9/src/test.ts:68:25)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind but I am curious why you get that error ? I tried it out myself in one of react-menu test files

image

image

Copy link
Member Author

Choose a reason for hiding this comment

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

  • ts-node-register ignores local configs and uses own
    compilerOptions: {
    ...readConfig(path.join(__dirname, 'typescript/tsconfig.common.json')).compilerOptions,
    ...readConfig(path.join(__dirname, 'tsconfig.json')).compilerOptions,
    },
  • the second config has esModuleInterop that causes the problem
    "esModuleInterop": true,

Copy link
Contributor

Choose a reason for hiding this comment

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

ideal would be if we can follow same patterns (especially in module resolution area ) as it might bit us whenever 2 different packages needs to be used together ?

I'd recommend to not use that ts-node-register as it parts its ways in terms of setup for v9 related code.

Can we use rather ts-node directly for invocation and provide local tsconfig.lib.json + remove allowSyntheticDefaultImports ?

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Hotell did I correctly understand your suggestion (d106951)?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes !

@layershifter layershifter force-pushed the chore/add-ssr-tests-test branch from b80ab06 to 22c057a Compare June 20, 2022 14:31
@layershifter layershifter marked this pull request as ready for review June 20, 2022 15:47
@layershifter layershifter requested a review from a team as a code owner June 20, 2022 15:47
@layershifter layershifter requested a review from Hotell June 20, 2022 15:48
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.

great stuff ! added some comments/suggestions

apps/ssr-tests-v9/package.json Outdated Show resolved Hide resolved
apps/ssr-tests-v9/src/utils/buildAssets.ts Show resolved Hide resolved
apps/ssr-tests-v9/src/utils/buildAssets.test.ts Outdated Show resolved Hide resolved
apps/ssr-tests-v9/src/test.ts Outdated Show resolved Hide resolved
const htmlPath = path.resolve(__dirname, '..', 'dist', 'index.html');

if (!fs.existsSync(htmlPath)) {
throw new Error('"dist/index.html" does not exist, please run "yarn build" first');
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: for better DX, can we maybe initiate the build for the user with proper notification what's going on and re-run the test ?

apps/ssr-tests-v9/src/test.ts Outdated Show resolved Hide resolved
apps/ssr-tests-v9/src/test.ts Outdated Show resolved Hide resolved
import { hrToSeconds } from './utils/helpers';
import { launchBrowser } from './utils/launchBrowser';

class RenderError extends Error {}
Copy link
Contributor

Choose a reason for hiding this comment

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

"inheritance" in JS is a confusing paradigm that doesn't work as one might think it works. with that leveraging instanceof is potentialy dangeours operation as it wont give you similar guarantees like in true OO languagues.

for those reason I'd recommend if we could follow one of following patterns:

  1. dont use "extends" rather literal objects
error = { message: message.text(), name: 'RenderError' };


// later on

  if (err.name === 'RenderError') {
  1. properly instantiate name of custom error
class RenderError extends Error {
  name = 'RangeError'
}

// check will be the same as in 1st option = no instanceof

WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

curious, what is wrong with instanceof in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Hotell I added name 9c37647, but I am confused as @ling1726. What is wrong with instanceof? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

in this particular case there is nothing wrong.

To give you better context:

from experience I avoid "volatile" JS lang features in general, which besides various JS quirks is also "OOP features", thus duck typing is always an obvious choice.

apps/ssr-tests-v9/src/test.ts Outdated Show resolved Hide resolved
import { launchBrowser } from './utils/launchBrowser';

class RenderError extends Error {
public name = 'RangeError';
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for public as everything is public by default. TS class accessors is a "lie" in general

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 added because it's enforced by ESLint rule...

@layershifter
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@layershifter layershifter merged commit 4405e16 into microsoft:master Jun 23, 2022
@layershifter layershifter deleted the chore/add-ssr-tests-test branch June 23, 2022 08:31
rohitpagariya pushed a commit to rohitpagariya/fluentui that referenced this pull request Jun 28, 2022
* chore: add SSR tests (test command)

* use console.warn & console.error

* address review comments

* use waitForSelector

* inline the script

* use ts-node directly

* avoid mocks in buildAssets.test.ts

* Update apps/ssr-tests-v9/src/test.ts

Co-authored-by: Martin Hochel <[email protected]>

* Update apps/ssr-tests-v9/src/test.ts

Co-authored-by: Martin Hochel <[email protected]>

* Update apps/ssr-tests-v9/src/test.ts

Co-authored-by: Martin Hochel <[email protected]>

* close browser always

* add name to the error

* fix versions

* fix versions

Co-authored-by: Martin Hochel <[email protected]>
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