From 9063df3ff5f4b230631b8dbb130a0a36adf05ca3 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 1 Apr 2024 14:19:35 -0500 Subject: [PATCH 1/3] [tests] Disallow unasserted console.log --- .../src/__tests__/ReactStrictMode-test.js | 3 + scripts/jest/matchers/toWarnDev.js | 95 ++++++++++--------- scripts/jest/setupTests.js | 25 ++++- 3 files changed, 76 insertions(+), 47 deletions(-) diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index 588b65f139ebe..44dcbc5e83283 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -35,6 +35,9 @@ describe('ReactStrictMode', () => { }); it('should appear in the client component stack', async () => { + // TODO REMOVE AFTER TESTING THIS FAILS ON PR + console.log('hit'); + function Foo() { return
; } diff --git a/scripts/jest/matchers/toWarnDev.js b/scripts/jest/matchers/toWarnDev.js index 70c7120ace965..0e02afdc8cdc2 100644 --- a/scripts/jest/matchers/toWarnDev.js +++ b/scripts/jest/matchers/toWarnDev.js @@ -178,52 +178,56 @@ const createMatcherFor = (consoleMethod, matcherName) => }; } - if (typeof withoutStack === 'number') { - // We're expecting a particular number of warnings without stacks. - if (withoutStack !== warningsWithoutComponentStack.length) { - return { - message: () => - `Expected ${withoutStack} warnings without a component stack but received ${warningsWithoutComponentStack.length}:\n` + - warningsWithoutComponentStack.map(warning => - this.utils.printReceived(warning) - ), - pass: false, - }; - } - } else if (withoutStack === true) { - // We're expecting that all warnings won't have the stack. - // If some warnings have it, it's an error. - if (warningsWithComponentStack.length > 0) { - return { - message: () => - `Received warning unexpectedly includes a component stack:\n ${this.utils.printReceived( - warningsWithComponentStack[0] - )}\nIf this warning intentionally includes the component stack, remove ` + - `{withoutStack: true} from the ${matcherName}() call. If you have a mix of ` + - `warnings with and without stack in one ${matcherName}() call, pass ` + - `{withoutStack: N} where N is the number of warnings without stacks.`, - pass: false, - }; - } - } else if (withoutStack === false || withoutStack === undefined) { - // We're expecting that all warnings *do* have the stack (default). - // If some warnings don't have it, it's an error. - if (warningsWithoutComponentStack.length > 0) { - return { - message: () => - `Received warning unexpectedly does not include a component stack:\n ${this.utils.printReceived( - warningsWithoutComponentStack[0] - )}\nIf this warning intentionally omits the component stack, add ` + - `{withoutStack: true} to the ${matcherName} call.`, - pass: false, - }; + if (consoleMethod === 'log') { + // We don't expect any console.log calls to have a stack. + } else if (typeof withoutStack === 'number') { + if (typeof withoutStack === 'number') { + // We're expecting a particular number of warnings without stacks. + if (withoutStack !== warningsWithoutComponentStack.length) { + return { + message: () => + `Expected ${withoutStack} warnings without a component stack but received ${warningsWithoutComponentStack.length}:\n` + + warningsWithoutComponentStack.map(warning => + this.utils.printReceived(warning) + ), + pass: false, + }; + } + } else if (withoutStack === true) { + // We're expecting that all warnings won't have the stack. + // If some warnings have it, it's an error. + if (warningsWithComponentStack.length > 0) { + return { + message: () => + `Received warning unexpectedly includes a component stack:\n ${this.utils.printReceived( + warningsWithComponentStack[0] + )}\nIf this warning intentionally includes the component stack, remove ` + + `{withoutStack: true} from the ${matcherName}() call. If you have a mix of ` + + `warnings with and without stack in one ${matcherName}() call, pass ` + + `{withoutStack: N} where N is the number of warnings without stacks.`, + pass: false, + }; + } + } else if (withoutStack === false || withoutStack === undefined) { + // We're expecting that all warnings *do* have the stack (default). + // If some warnings don't have it, it's an error. + if (warningsWithoutComponentStack.length > 0) { + return { + message: () => + `Received warning unexpectedly does not include a component stack:\n ${this.utils.printReceived( + warningsWithoutComponentStack[0] + )}\nIf this warning intentionally omits the component stack, add ` + + `{withoutStack: true} to the ${matcherName} call.`, + pass: false, + }; + } + } else { + throw Error( + `The second argument for ${matcherName}(), when specified, must be an object. It may have a ` + + `property called "withoutStack" whose value may be undefined, boolean, or a number. ` + + `Instead received ${typeof withoutStack}.` + ); } - } else { - throw Error( - `The second argument for ${matcherName}(), when specified, must be an object. It may have a ` + - `property called "withoutStack" whose value may be undefined, boolean, or a number. ` + - `Instead received ${typeof withoutStack}.` - ); } if (lastWarningWithMismatchingFormat !== null) { @@ -309,4 +313,5 @@ const createMatcherFor = (consoleMethod, matcherName) => module.exports = { toWarnDev: createMatcherFor('warn', 'toWarnDev'), toErrorDev: createMatcherFor('error', 'toErrorDev'), + toLogDev: createMatcherFor('log', 'toLogDev'), }; diff --git a/scripts/jest/setupTests.js b/scripts/jest/setupTests.js index 82797189c43f9..cd125de53eba5 100644 --- a/scripts/jest/setupTests.js +++ b/scripts/jest/setupTests.js @@ -116,18 +116,19 @@ if (process.env.REACT_CLASS_EQUIVALENCE_TEST) { .join('\n')}` ); + const type = methodName === 'log' ? 'log' : 'warning'; const message = `Expected test not to call ${chalk.bold( `console.${methodName}()` )}.\n\n` + - 'If the warning is expected, test for it explicitly by:\n' + + `If the ${type} is expected, test for it explicitly by:\n` + `1. Using the ${chalk.bold('.' + expectedMatcher + '()')} ` + `matcher, or...\n` + `2. Mock it out using ${chalk.bold( 'spyOnDev' )}(console, '${methodName}') or ${chalk.bold( 'spyOnProd' - )}(console, '${methodName}'), and test that the warning occurs.`; + )}(console, '${methodName}'), and test that the ${type} occurs.`; throw new Error(`${message}\n\n${messages.join('\n\n')}`); } @@ -135,9 +136,17 @@ if (process.env.REACT_CLASS_EQUIVALENCE_TEST) { const unexpectedErrorCallStacks = []; const unexpectedWarnCallStacks = []; + const unexpectedLogCallStacks = []; const errorMethod = patchConsoleMethod('error', unexpectedErrorCallStacks); const warnMethod = patchConsoleMethod('warn', unexpectedWarnCallStacks); + let logMethod; + + // Only assert console.log isn't called in CI so you can debug tests in DEV. + // The matchers will still work in DEV, so you can assert locally. + if (process.env.CI) { + logMethod = patchConsoleMethod('log', unexpectedLogCallStacks); + } const flushAllUnexpectedConsoleCalls = () => { flushUnexpectedConsoleCalls( @@ -152,6 +161,15 @@ if (process.env.REACT_CLASS_EQUIVALENCE_TEST) { 'toWarnDev', unexpectedWarnCallStacks ); + if (logMethod) { + flushUnexpectedConsoleCalls( + logMethod, + 'log', + 'toLogDev', + unexpectedLogCallStacks + ); + unexpectedLogCallStacks.length = 0; + } unexpectedErrorCallStacks.length = 0; unexpectedWarnCallStacks.length = 0; }; @@ -159,6 +177,9 @@ if (process.env.REACT_CLASS_EQUIVALENCE_TEST) { const resetAllUnexpectedConsoleCalls = () => { unexpectedErrorCallStacks.length = 0; unexpectedWarnCallStacks.length = 0; + if (logMethod) { + unexpectedLogCallStacks.length = 0; + } }; beforeEach(resetAllUnexpectedConsoleCalls); From 1e982d23ec4f998f47105d9e0dbbaddd996a1dc8 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 1 Apr 2024 14:55:53 -0500 Subject: [PATCH 2/3] remove test log --- packages/react/src/__tests__/ReactStrictMode-test.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index 44dcbc5e83283..588b65f139ebe 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -35,9 +35,6 @@ describe('ReactStrictMode', () => { }); it('should appear in the client component stack', async () => { - // TODO REMOVE AFTER TESTING THIS FAILS ON PR - console.log('hit'); - function Foo() { return
; } From b98343bedaab3ddaf20a184fe09544614467a2a4 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 1 Apr 2024 15:43:39 -0500 Subject: [PATCH 3/3] Fix withoutStack, add tests --- .../jest/matchers/__tests__/toWarnDev-test.js | 17 ++++ scripts/jest/matchers/toWarnDev.js | 90 +++++++++---------- 2 files changed, 61 insertions(+), 46 deletions(-) diff --git a/scripts/jest/matchers/__tests__/toWarnDev-test.js b/scripts/jest/matchers/__tests__/toWarnDev-test.js index a0ce02ccf663c..90b78d2cd4729 100644 --- a/scripts/jest/matchers/__tests__/toWarnDev-test.js +++ b/scripts/jest/matchers/__tests__/toWarnDev-test.js @@ -416,3 +416,20 @@ describe('toWarnDev', () => { }); } }); + +describe('toLogDev', () => { + it('does not fail if warnings do not include a stack', () => { + expect(() => { + if (__DEV__) { + console.log('Hello'); + } + }).toLogDev('Hello'); + expect(() => { + if (__DEV__) { + console.log('Hello'); + console.log('Good day'); + console.log('Bye'); + } + }).toLogDev(['Hello', 'Good day', 'Bye']); + }); +}); diff --git a/scripts/jest/matchers/toWarnDev.js b/scripts/jest/matchers/toWarnDev.js index 0e02afdc8cdc2..e724653d4ef31 100644 --- a/scripts/jest/matchers/toWarnDev.js +++ b/scripts/jest/matchers/toWarnDev.js @@ -181,53 +181,51 @@ const createMatcherFor = (consoleMethod, matcherName) => if (consoleMethod === 'log') { // We don't expect any console.log calls to have a stack. } else if (typeof withoutStack === 'number') { - if (typeof withoutStack === 'number') { - // We're expecting a particular number of warnings without stacks. - if (withoutStack !== warningsWithoutComponentStack.length) { - return { - message: () => - `Expected ${withoutStack} warnings without a component stack but received ${warningsWithoutComponentStack.length}:\n` + - warningsWithoutComponentStack.map(warning => - this.utils.printReceived(warning) - ), - pass: false, - }; - } - } else if (withoutStack === true) { - // We're expecting that all warnings won't have the stack. - // If some warnings have it, it's an error. - if (warningsWithComponentStack.length > 0) { - return { - message: () => - `Received warning unexpectedly includes a component stack:\n ${this.utils.printReceived( - warningsWithComponentStack[0] - )}\nIf this warning intentionally includes the component stack, remove ` + - `{withoutStack: true} from the ${matcherName}() call. If you have a mix of ` + - `warnings with and without stack in one ${matcherName}() call, pass ` + - `{withoutStack: N} where N is the number of warnings without stacks.`, - pass: false, - }; - } - } else if (withoutStack === false || withoutStack === undefined) { - // We're expecting that all warnings *do* have the stack (default). - // If some warnings don't have it, it's an error. - if (warningsWithoutComponentStack.length > 0) { - return { - message: () => - `Received warning unexpectedly does not include a component stack:\n ${this.utils.printReceived( - warningsWithoutComponentStack[0] - )}\nIf this warning intentionally omits the component stack, add ` + - `{withoutStack: true} to the ${matcherName} call.`, - pass: false, - }; - } - } else { - throw Error( - `The second argument for ${matcherName}(), when specified, must be an object. It may have a ` + - `property called "withoutStack" whose value may be undefined, boolean, or a number. ` + - `Instead received ${typeof withoutStack}.` - ); + // We're expecting a particular number of warnings without stacks. + if (withoutStack !== warningsWithoutComponentStack.length) { + return { + message: () => + `Expected ${withoutStack} warnings without a component stack but received ${warningsWithoutComponentStack.length}:\n` + + warningsWithoutComponentStack.map(warning => + this.utils.printReceived(warning) + ), + pass: false, + }; } + } else if (withoutStack === true) { + // We're expecting that all warnings won't have the stack. + // If some warnings have it, it's an error. + if (warningsWithComponentStack.length > 0) { + return { + message: () => + `Received warning unexpectedly includes a component stack:\n ${this.utils.printReceived( + warningsWithComponentStack[0] + )}\nIf this warning intentionally includes the component stack, remove ` + + `{withoutStack: true} from the ${matcherName}() call. If you have a mix of ` + + `warnings with and without stack in one ${matcherName}() call, pass ` + + `{withoutStack: N} where N is the number of warnings without stacks.`, + pass: false, + }; + } + } else if (withoutStack === false || withoutStack === undefined) { + // We're expecting that all warnings *do* have the stack (default). + // If some warnings don't have it, it's an error. + if (warningsWithoutComponentStack.length > 0) { + return { + message: () => + `Received warning unexpectedly does not include a component stack:\n ${this.utils.printReceived( + warningsWithoutComponentStack[0] + )}\nIf this warning intentionally omits the component stack, add ` + + `{withoutStack: true} to the ${matcherName} call.`, + pass: false, + }; + } + } else { + throw Error( + `The second argument for ${matcherName}(), when specified, must be an object. It may have a ` + + `property called "withoutStack" whose value may be undefined, boolean, or a number. ` + + `Instead received ${typeof withoutStack}.` + ); } if (lastWarningWithMismatchingFormat !== null) {