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

BREAKING(react-conformance): update test type and add new option disableTypeTests to disables tests that requires type information #28988

Merged
merged 12 commits into from
Sep 6, 2023
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "feat: add option `disableTypeTests` to disable tests that require TypeScript information",
"packageName": "@fluentui/react-conformance",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "chore: internal type update with @fluentui/react-conformance version bump",
"packageName": "@fluentui/react-conformance-griffel",
"email": "[email protected]",
"dependentChangeType": "none"
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const parseDocblock = (docblock: string) => {
*/
export const extraConformanceTests: TestObject = {
/** Component has a docblock with 5 to 25 words */
'has-docblock': (componentInfo, testInfo) => {
'has-docblock': (_testInfo, componentInfo) => {
const maxWords = 25;
const minWords = 5;

Expand All @@ -37,7 +37,7 @@ export const extraConformanceTests: TestObject = {
},

/** If the component is a subcomponent, ensure its parent has the subcomponent as static property */
'is-static-property-of-parent': (componentInfo, testInfo) => {
'is-static-property-of-parent': testInfo => {
const { componentPath, displayName, Component } = testInfo;
const componentFolder = componentPath.replace(path.basename(componentPath) + path.extname(componentPath), '');
const dirName = path.basename(componentFolder).replace(path.extname(componentFolder), '');
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { IsConformantOptions, ConformanceTest, TestOptions } from '@fluentui/react-conformance';
import type { IsConformantOptions, BaseConformanceTest, TestOptions } from '@fluentui/react-conformance';
import './matchers/index';

export const OVERRIDES_WIN_TEST_NAME = 'make-styles-overrides-win';
Expand Down Expand Up @@ -28,7 +28,7 @@ async function getReactComponent(
* A conformance test for mergeClasses() that ensures that a classname from props is passed as a last param,
* i.e. ensures that user's overrides have higher priority.
*/
export const overridesWin: ConformanceTest = (componentInfo, testInfo) => {
export const overridesWin: BaseConformanceTest = testInfo => {
const testOptions = testInfo.testOptions as
| (TestOptions & { [OVERRIDES_WIN_TEST_NAME]?: OverridesWinTestOptions })
| undefined;
Expand Down
8 changes: 6 additions & 2 deletions packages/react-conformance/etc/react-conformance.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ import { render } from '@testing-library/react';
import * as ts from 'typescript';

// @public (undocumented)
export type ConformanceTest<TProps = {}> = (componentInfo: ComponentDoc, testInfo: IsConformantOptions<TProps>, tsProgram: ts.Program) => void;
export type BaseConformanceTest<TProps = {}> = (testInfo: IsConformantOptions<TProps>) => void;

// @public (undocumented)
export type ConformanceTest<TProps = {}> = (testInfo: IsConformantOptions<TProps>, componentInfo: ComponentDoc, tsProgram: ts.Program) => void;

// @public (undocumented)
export function isConformant<TProps = {}>(...testInfo: Partial<IsConformantOptions<TProps>>[]): void;
Expand All @@ -20,6 +23,7 @@ export interface IsConformantOptions<TProps = {}> {
Component: React_2.ComponentType<TProps>;
componentPath: string;
disabledTests?: string[];
disableTypeTests?: boolean;
displayName: string;
elementRefName?: string;
extraTests?: TestObject<TProps>;
Expand All @@ -41,7 +45,7 @@ export interface IsConformantOptions<TProps = {}> {
// @public (undocumented)
export interface TestObject<TProps = {}> {
// (undocumented)
[key: string]: ConformanceTest<TProps>;
[key: string]: BaseConformanceTest<TProps> | ConformanceTest<TProps>;
}

// @public
Expand Down
36 changes: 18 additions & 18 deletions packages/react-conformance/src/defaultTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as _ from 'lodash';
import * as path from 'path';
import { render } from '@testing-library/react';

import { TestObject, IsConformantOptions } from './types';
import { IsConformantOptions, DefaultTestObject } from './types';
import { defaultErrorMessages } from './defaultErrorMessages';
import { ComponentDoc } from 'react-docgen-typescript';
import { getPackagePath, getCallbackArguments, validateCallbackArguments } from './utils/index';
Expand Down Expand Up @@ -32,9 +32,9 @@ function getTargetElement(

/* eslint-disable @typescript-eslint/naming-convention */

export const defaultTests: TestObject = {
export const defaultTests: DefaultTestObject = {
/** Component file exports a valid React element type */
'exports-component': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'exports-component': (testInfo: IsConformantOptions) => {
it(`exports component from file under correct name (exports-component)`, () => {
const { componentPath, Component, displayName } = testInfo;
const componentFile = require(componentPath);
Expand All @@ -52,7 +52,7 @@ export const defaultTests: TestObject = {
},

/** Component file exports a valid React element and can render it */
'component-renders': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'component-renders': (testInfo: IsConformantOptions) => {
it(`renders (component-renders)`, () => {
try {
const { requiredProps, Component, renderOptions } = testInfo;
Expand All @@ -67,7 +67,7 @@ export const defaultTests: TestObject = {
* If functional component: component has a displayName
* Else: component's constructor is a named function and matches displayName
*/
'component-has-displayname': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'component-has-displayname': (testInfo: IsConformantOptions) => {
const { Component } = testInfo;

it(`has a displayName or constructor name (component-has-displayname)`, () => {
Expand All @@ -86,7 +86,7 @@ export const defaultTests: TestObject = {
},

/** Component handles ref */
'component-handles-ref': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'component-handles-ref': (testInfo: IsConformantOptions) => {
it(`handles ref (component-handles-ref)`, () => {
// This test simply verifies that the passed ref is applied to an element *anywhere* in the DOM
const { Component, requiredProps, elementRefName = 'ref', renderOptions } = testInfo;
Expand All @@ -108,7 +108,7 @@ export const defaultTests: TestObject = {
},

/** Component has ref applied to the root component DOM node */
'component-has-root-ref': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'component-has-root-ref': (testInfo: IsConformantOptions) => {
it(`applies ref to root element (component-has-root-ref)`, () => {
const { renderOptions, Component, requiredProps, elementRefName = 'ref', primarySlot = 'root' } = testInfo;

Expand Down Expand Up @@ -153,7 +153,7 @@ export const defaultTests: TestObject = {
* (In the extremely unlikely event that someone has a compelling need for the native functionality
* in the future, it can be added under an `htmlSize` prop.)
*/
'omits-size-prop': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'omits-size-prop': (testInfo: IsConformantOptions, componentInfo: ComponentDoc) => {
const sizeType = componentInfo.props.size?.type?.name;
if (!sizeType || componentInfo.props.htmlSize) {
return;
Expand Down Expand Up @@ -188,7 +188,7 @@ export const defaultTests: TestObject = {
},

/** Component file handles classname prop */
'component-handles-classname': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'component-handles-classname': (testInfo: IsConformantOptions) => {
const { Component, requiredProps, renderOptions } = testInfo;
const testClassName = 'testComponentClassName';
let handledClassName = false;
Expand Down Expand Up @@ -253,10 +253,10 @@ export const defaultTests: TestObject = {
},

/** Component file has assigned and exported static classnames object */
'component-has-static-classnames-object': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'component-has-static-classnames-object': (testInfo: IsConformantOptions) => {
const { componentPath, Component, testOptions = {}, requiredProps, renderOptions } = testInfo;

const componentName = componentInfo.displayName;
const componentName = testInfo.displayName;
const classNamePrefix = testOptions?.['component-has-static-classname']?.prefix ?? 'fui';
const componentClassName = `${classNamePrefix}-${componentName}`;
const exportName = `${componentName[0].toLowerCase()}${componentName.slice(1)}ClassNames`;
Expand Down Expand Up @@ -353,7 +353,7 @@ export const defaultTests: TestObject = {
},

/** Constructor/component name matches filename */
'name-matches-filename': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'name-matches-filename': (testInfo: IsConformantOptions) => {
it(`Component/constructor name matches filename (name-matches-filename)`, () => {
try {
const { componentPath, displayName } = testInfo;
Expand All @@ -367,7 +367,7 @@ export const defaultTests: TestObject = {
},

/** Ensures component is exported at top level allowing `import { Component } from 'packageName'` */
'exported-top-level': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'exported-top-level': (testInfo: IsConformantOptions) => {
if (testInfo.isInternal) {
return;
}
Expand All @@ -385,7 +385,7 @@ export const defaultTests: TestObject = {
},

/** Ensures component has top level file in package/src/componentName */
'has-top-level-file': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'has-top-level-file': (testInfo: IsConformantOptions) => {
if (testInfo.isInternal) {
return;
}
Expand All @@ -403,7 +403,7 @@ export const defaultTests: TestObject = {
},

/** Ensures aria attributes are kebab cased */
'kebab-aria-attributes': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'kebab-aria-attributes': (testInfo: IsConformantOptions, componentInfo: ComponentDoc) => {
it(`uses kebab-case for aria attributes (kebab-aria-attributes)`, () => {
const invalidProps = Object.keys(componentInfo.props).filter(
prop => prop.startsWith('aria') && !/^aria-[a-z]+$/.test(prop),
Expand All @@ -418,7 +418,7 @@ export const defaultTests: TestObject = {

// TODO: Test last word of callback name against list of valid verbs
/** Ensures that components have consistent custom callback names i.e. on[Part][Event] */
'consistent-callback-names': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'consistent-callback-names': (testInfo: IsConformantOptions, componentInfo: ComponentDoc) => {
it(`has consistent custom callback names (consistent-callback-names)`, () => {
const { testOptions = {} } = testInfo;
const propNames = Object.keys(componentInfo.props);
Expand All @@ -445,7 +445,7 @@ export const defaultTests: TestObject = {
},

/** Ensures that components have consistent callback arguments (ev, data) */
'consistent-callback-args': (componentInfo, testInfo, tsProgram) => {
'consistent-callback-args': (testInfo, componentInfo, tsProgram) => {
it('has consistent custom callback arguments (consistent-callback-args)', () => {
const { testOptions = {} } = testInfo;

Expand Down Expand Up @@ -501,7 +501,7 @@ export const defaultTests: TestObject = {
},

/** If the primary slot is specified, it receives native props other than 'className' and 'style' */
'primary-slot-gets-native-props': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => {
'primary-slot-gets-native-props': (testInfo: IsConformantOptions) => {
it(`applies correct native props to the primary and root slots (primary-slot-gets-native-props)`, () => {
try {
const { Component, requiredProps, primarySlot = 'root', renderOptions } = testInfo;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-conformance/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
export { isConformant } from './isConformant';
export type { ConformanceTest, IsConformantOptions, TestObject, TestOptions } from './types';
export type { BaseConformanceTest, ConformanceTest, IsConformantOptions, TestObject, TestOptions } from './types';
49 changes: 46 additions & 3 deletions packages/react-conformance/src/isConformant.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as fs from 'fs';

import { IsConformantOptions } from './types';
import { BaseConformanceTest, ConformanceTest, DefaultTestObject, IsConformantOptions } from './types';
import { defaultTests } from './defaultTests';
import { merge } from './utils/merge';
import { createTsProgram } from './utils/createTsProgram';
Expand All @@ -17,6 +17,7 @@ export function isConformant<TProps = {}>(...testInfo: Partial<IsConformantOptio
tsConfig,
// eslint-disable-next-line deprecation/deprecation
tsconfigDir,
disableTypeTests,
} = mergedOptions;

const mergedTsConfig = {
Expand All @@ -29,6 +30,11 @@ export function isConformant<TProps = {}>(...testInfo: Partial<IsConformantOptio
throw new Error(`Path ${componentPath} does not exist`);
}

if (disableTypeTests) {
runNonTypeTests(mergedOptions);
return;
}

const tsProgram = createTsProgram(componentPath, mergedTsConfig);

const components = getComponentDoc(componentPath, tsProgram);
Expand All @@ -39,15 +45,15 @@ export function isConformant<TProps = {}>(...testInfo: Partial<IsConformantOptio

for (const test of Object.keys(defaultTests)) {
if (!disabledTests.includes(test)) {
defaultTests[test](componentInfo, mergedOptions, tsProgram);
defaultTests[test as keyof DefaultTestObject](mergedOptions, componentInfo, tsProgram);
}
}

if (extraTests) {
describe('extraTests', () => {
for (const test of Object.keys(extraTests)) {
if (!disabledTests.includes(test)) {
extraTests[test](componentInfo, mergedOptions, tsProgram);
extraTests[test](mergedOptions, componentInfo, tsProgram);
}
}
});
Expand All @@ -63,3 +69,40 @@ export function isConformant<TProps = {}>(...testInfo: Partial<IsConformantOptio
}
});
}

/**
* Run default and extra tests that don't require TypeScript information
* @param mergedOptions
*/
function runNonTypeTests(mergedOptions: IsConformantOptions) {
const { disabledTests = [], extraTests } = mergedOptions;

for (const test of Object.keys(defaultTests)) {
if (!disabledTests.includes(test)) {
const func = defaultTests[test as keyof DefaultTestObject];
if (isNonTypeTest(func)) {
func(mergedOptions);
}
}
}

if (extraTests) {
describe('extraTests', () => {
for (const test of Object.keys(extraTests)) {
if (!disabledTests.includes(test)) {
const func = extraTests[test];
if (isNonTypeTest(func)) {
func(mergedOptions);
}
}
}
});
}
}

/**
* Verifies that a test function has only one parameter. If so, this test does not require TS info.
*/
function isNonTypeTest<TProps = {}>(func: ConformanceTest<TProps>): func is BaseConformanceTest<TProps> {
return func.length === 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not the most robust or explicit API to have for such a critical functionality. looks like missed opportunity to design it properly while introducing breaking change. would be great to bring this to v-build sync to discuss things for better outcome. ty

}
31 changes: 29 additions & 2 deletions packages/react-conformance/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ export interface IsConformantOptions<TProps = {}> {
* If there are tests that aren't supposed to run on a component, this allows to opt out of any test.
*/
disabledTests?: string[];

/**
* Disable tests that verify the component's prop types.
* These tests require TypeScript information.
* It is recommended to keep these tests enabled, but they can be disabled in a large repo to improve test performance and memory consumption.
*/
disableTypeTests?: boolean;

/**
* Optional flag that means the component is not exported at top level.
* @defaultvalue false
Expand Down Expand Up @@ -104,12 +112,31 @@ export interface IsConformantOptions<TProps = {}> {
tsConfig?: Partial<{ configName: string; configDir: string }>;
}

export type BaseConformanceTest<TProps = {}> = (testInfo: IsConformantOptions<TProps>) => void;
export type ConformanceTest<TProps = {}> = (
componentInfo: ComponentDoc,
testInfo: IsConformantOptions<TProps>,
componentInfo: ComponentDoc,
tsProgram: ts.Program,
) => void;

export interface TestObject<TProps = {}> {
[key: string]: ConformanceTest<TProps>;
[key: string]: BaseConformanceTest<TProps> | ConformanceTest<TProps>;
}

export interface DefaultTestObject<TProps = {}> {
'exports-component': BaseConformanceTest<TProps>;
'component-renders': BaseConformanceTest<TProps>;
'component-has-displayname': BaseConformanceTest<TProps>;
'component-handles-ref': BaseConformanceTest<TProps>;
'component-has-root-ref': BaseConformanceTest<TProps>;
'omits-size-prop': ConformanceTest<TProps>;
'component-handles-classname': BaseConformanceTest<TProps>;
'component-has-static-classnames-object': ConformanceTest<TProps>;
'name-matches-filename': BaseConformanceTest<TProps>;
'exported-top-level': BaseConformanceTest<TProps>;
'has-top-level-file': BaseConformanceTest<TProps>;
'kebab-aria-attributes': ConformanceTest<TProps>;
'consistent-callback-names': ConformanceTest<TProps>;
'consistent-callback-args': ConformanceTest<TProps>;
'primary-slot-gets-native-props': BaseConformanceTest<TProps>;
}