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: Northstar screener should read from screenerStates.json #24848

Merged
merged 5 commits into from
Sep 19, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/vr-tests-react-components/screener.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Member Author

Choose a reason for hiding this comment

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

improve typings in screener.config.js files

Copy link
Contributor

Choose a reason for hiding this comment

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

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

*/
function getConfig({ screenerApiKey, sourceBranchName, deployUrl, targetBranch }) {
const baseBranch = targetBranch ? targetBranch.replace(/^refs\/heads\//, '') : 'master';
Expand All @@ -38,6 +38,7 @@ function getConfig({ screenerApiKey, sourceBranchName, deployUrl, targetBranch }
baseBranch,
failureExitCode: 0,
alwaysAcceptBaseBranch: true,
states: [],
...(sourceBranchName !== 'master' ? { commit: getCurrentHash() } : null),
baseUrl: `${deployUrl}/react-components-screener/iframe.html`,
};
Expand Down
3 changes: 2 additions & 1 deletion apps/vr-tests/screener.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ function getCurrentHash() {
* @param {string} options.sourceBranchName
* @param {string} options.deployUrl
* @param {string} options.targetBranch
* @returns
* @returns {import('@fluentui/scripts/screener/screener.types').ScreenerRunnerConfig}
*/
function getConfig({ screenerApiKey, sourceBranchName, deployUrl, targetBranch }) {
const baseBranch = targetBranch ? targetBranch.replace(/^refs\/heads\//, '') : 'master';
Expand All @@ -44,6 +44,7 @@ function getConfig({ screenerApiKey, sourceBranchName, deployUrl, targetBranch }
alwaysAcceptBaseBranch: true,
...(sourceBranchName !== 'master' ? { commit: getCurrentHash() } : null),
baseUrl: `${deployUrl}/react-screener/iframe.html`,
states: [],
};
console.log('Screener config: ' + JSON.stringify({ ...config, apiKey: '...' }, null, 2));
return config;
Expand Down
3 changes: 2 additions & 1 deletion gulpfile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ tsPaths.register({
require('./scripts/gulp/tasks/bundle');
require('./scripts/gulp/tasks/component-info');
require('./scripts/gulp/tasks/docs');
require('./scripts/gulp/tasks/screener');
require('./scripts/gulp/tasks/vr-build');
require('./scripts/gulp/tasks/vr-test');
require('./scripts/gulp/tasks/stats');
require('./scripts/gulp/tasks/test-unit');
require('./scripts/gulp/tasks/perf');
Expand Down
21 changes: 21 additions & 0 deletions scripts/gulp/tasks/vr-build.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { task, series } from 'gulp';
Copy link
Member Author

Choose a reason for hiding this comment

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

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

import fs from 'fs';

import config from '../../config';
import getScreenerStates from '../../screener/screener.states';
Copy link
Contributor

Choose a reason for hiding this comment

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

same public api chore like in former comment


const { paths } = config;

task('screener:states', cb => {
const states = getScreenerStates();
const statesJson = JSON.stringify(states, null, 2);
fs.writeFile(paths.docsDist('screenerStates.json'), statesJson, { encoding: 'utf8' }, err => {
if (err) {
cb(err);
}

cb();
});
});

task('screener:build', series('screener:states'));
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { task, series } from 'gulp';
import { task } from 'gulp';
import { argv } from 'yargs';

import config from '../../config';
import { getAllPackageInfo } from '../../monorepo';
import { screenerRunner } from '../../screener/screener.runner';
import getConfig from '../../screener/screener.config';

const { paths } = config;
const docsPackageName = '@fluentui/docs';

// ----------------------------------------
// Visual
Expand All @@ -15,15 +17,11 @@ task('screener:runner', cb => {
// screener-runner doesn't allow to pass custom options
if (argv.filter) process.env.SCREENER_FILTER = argv.filter as string;

const docsPackageName = '@fluentui/docs';

const packageInfos = getAllPackageInfo();
if (Object.values(packageInfos).every(packageInfo => packageInfo.packageJson.name !== docsPackageName)) {
throw new Error(`package ${docsPackageName} does not exist in the repo`);
}

const screenerConfigPath = paths.base('scripts/screener/screener.config.js');

// kill the server when done
const handlePromiseExit = promise =>
promise
Expand All @@ -36,17 +34,14 @@ task('screener:runner', cb => {
process.exit(1);
});

const getConfig = require(screenerConfigPath);
const screenerConfig = getConfig({
screenerApiKey: process.env.SCREENER_API_KEY,
sourceBranchName: process.env.BUILD_SOURCEBRANCHNAME,
deployUrl: process.env.DEPLOYURL,
});

const screenerStates = require(paths.docsDist('screenerStates.json'));
screenerConfig.states = screenerStates;

handlePromiseExit(screenerRunner(screenerConfig));
});

// ----------------------------------------
// Default
// ----------------------------------------

task('screener:build', series('build:docs'));
10 changes: 5 additions & 5 deletions scripts/screener/screener.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,13 @@ const baseBranch = 'master';
* @param {Object} options
* @param {string} options.screenerApiKey
* @param {string} options.sourceBranchName
* @returns
* @param {string} options.deployUrl
* @returns {import('./screener.types').ScreenerRunnerConfig}
*/
function getConfig({ screenerApiKey, sourceBranchName }) {
function getConfig({ screenerApiKey, sourceBranchName, deployUrl }) {
// https://github.com/screener-io/screener-runner
return {
baseUrl: `${deployUrl}/react-northstar-screener`,
apiKey: screenerApiKey,
projectRepo: 'microsoft/fluentui/fluentui',

Expand All @@ -54,9 +56,7 @@ function getConfig({ screenerApiKey, sourceBranchName }) {
minShiftGraphic: 1, // Optional threshold for pixel shifts in graphics.
compareSVGDOM: false, // Pass if SVG DOM is the same. Defaults to false.
},

// screenshot every example in maximized mode
states: require('./screener.states').default,
states: [],

alwaysAcceptBaseBranch: true,
baseBranch,
Expand Down
74 changes: 38 additions & 36 deletions scripts/screener/screener.states.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,42 +7,44 @@ import path from 'path';
import getScreenerSteps from './screener.steps';
import { ScreenerState } from './screener.types';

const baseUrl = `${process.env.DEPLOYURL}/react-northstar-screener`;
const examplePaths = glob.sync('packages/fluentui/docs/src/examples/**/*.tsx', {
ignore: ['**/index.tsx', '**/*.knobs.tsx', '**/BestPractices/*.tsx', '**/Playground.tsx'],
});

const pathFilter = process.env.SCREENER_FILTER;
const filteredPaths: string[] = minimatch.match(examplePaths, pathFilter || '*', {
matchBase: true,
});

if (pathFilter) {
console.log(chalk.bgGreen.black(' --filter '), pathFilter);
filteredPaths.forEach(filteredPath => console.log(`${_.repeat(' ', 10)} ${filteredPath}`));
}

const getStateForPath = (examplePath: string): ScreenerState => {
const { name: exampleNameWithoutExtension, base: exampleNameWithExtension, dir: exampleDir } = path.parse(
examplePath,
);

const rtl = exampleNameWithExtension.endsWith('.rtl.tsx');
const exampleUrl = _.kebabCase(exampleNameWithoutExtension);
const pageUrl = `${baseUrl}/maximize/${exampleUrl}/${rtl}`;

return {
url: pageUrl,
name: exampleNameWithExtension,

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const baseUrl = `${process.env.DEPLOYURL}/react-northstar-screener`;
const examplePaths = glob.sync('packages/fluentui/docs/src/examples/**/*.tsx', {
ignore: ['**/index.tsx', '**/*.knobs.tsx', '**/BestPractices/*.tsx', '**/Playground.tsx'],
});

const pathFilter = process.env.SCREENER_FILTER;
const filteredPaths: string[] = minimatch.match(examplePaths, pathFilter || '*', {
matchBase: true,
});

if (pathFilter) {
console.log(chalk.bgGreen.black(' --filter '), pathFilter);
filteredPaths.forEach(filteredPath => console.log(`${_.repeat(' ', 10)} ${filteredPath}`));
}

const getStateForPath = (examplePath: string): ScreenerState => {
const { name: exampleNameWithoutExtension, base: exampleNameWithExtension, dir: exampleDir } = path.parse(
examplePath,
);

const rtl = exampleNameWithExtension.endsWith('.rtl.tsx');
const exampleUrl = _.kebabCase(exampleNameWithoutExtension);
const pageUrl = `${baseUrl}/maximize/${exampleUrl}/${rtl}`;

return {
url: pageUrl,
name: exampleNameWithExtension,

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

const screenerStates = filteredPaths.reduce((states, examplePath) => {
states.push(getStateForPath(examplePath));
return states;
}, [] as ReturnType<typeof getStateForPath>[]);
const screenerStates = filteredPaths.reduce((states, examplePath) => {
states.push(getStateForPath(examplePath));
return states;
}, [] as ReturnType<typeof getStateForPath>[]);

export default screenerStates;
return screenerStates;
}
6 changes: 3 additions & 3 deletions scripts/screener/screener.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,22 @@ export type ScreenerRunnerConfig = {
apiKey: string;
projectRepo: string;

diffOptions: {
diffOptions?: {
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.
Copy link
Member Author

Choose a reason for hiding this comment

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

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

};

states: ScreenerState[];

alwaysAcceptBaseBranch: boolean;
baseBranch: string;
commit: string;
commit?: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

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

failureExitCode: number;

/** Base url of deployed storybook screener should test */
Expand Down