Skip to content

Commit

Permalink
Warn about unsafe toWarnDev() nesting in tests (#12457)
Browse files Browse the repository at this point in the history
* Add lint run to warn about improperly nested toWarnDev matchers
* Updated tests to avoid invalid nesting
  • Loading branch information
bvaughn authored Aug 21, 2018
1 parent 026aa9c commit e106b8c
Show file tree
Hide file tree
Showing 8 changed files with 160 additions and 84 deletions.
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ module.exports = {
// CUSTOM RULES
// the second argument of warning/invariant should be a literal string
'react-internal/no-primitive-constructors': ERROR,
'react-internal/no-to-warn-dev-within-to-throw': ERROR,
'react-internal/warning-and-invariant-args': ERROR,
},

Expand Down
39 changes: 27 additions & 12 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -428,12 +428,13 @@ describe('ReactCompositeComponent', () => {
expect(() => {
expect(() => {
ReactDOM.render(<ClassWithRenderNotExtended />, container);
}).toWarnDev(
'Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, ' +
"but doesn't extend React.Component. This is likely to cause errors. " +
'Change ClassWithRenderNotExtended to extend React.Component instead.',
);
}).toThrow(TypeError);
}).toThrow(TypeError);
}).toWarnDev(
'Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, ' +
"but doesn't extend React.Component. This is likely to cause errors. " +
'Change ClassWithRenderNotExtended to extend React.Component instead.',
{withoutStack: true},
);

// Test deduplication
expect(() => {
Expand Down Expand Up @@ -1728,11 +1729,18 @@ describe('ReactCompositeComponent', () => {
expect(() => {
expect(() => {
ReactTestUtils.renderIntoDocument(<RenderTextInvalidConstructor />);
}).toWarnDev(
}).toThrow();
}).toWarnDev(
[
// Expect two errors because invokeGuardedCallback will dispatch an error event,
// Causing the warning to be logged again.
'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned component instance: ' +
'did you accidentally return an object from the constructor?',
);
}).toThrow();
'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned component instance: ' +
'did you accidentally return an object from the constructor?',
],
{withoutStack: true},
);
});

it('should return error if render is not defined', () => {
Expand All @@ -1741,10 +1749,17 @@ describe('ReactCompositeComponent', () => {
expect(() => {
expect(() => {
ReactTestUtils.renderIntoDocument(<RenderTestUndefinedRender />);
}).toWarnDev(
}).toThrow();
}).toWarnDev(
[
// Expect two errors because invokeGuardedCallback will dispatch an error event,
// Causing the warning to be logged again.
'Warning: RenderTestUndefinedRender(...): No `render` method found on the returned ' +
'component instance: you may have forgotten to define `render`.',
);
}).toThrow();
'Warning: RenderTestUndefinedRender(...): No `render` method found on the returned ' +
'component instance: you may have forgotten to define `render`.',
],
{withoutStack: true},
);
});
});
16 changes: 8 additions & 8 deletions packages/react-dom/src/__tests__/ReactDOMTextarea-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -359,10 +359,10 @@ describe('ReactDOMTextarea', () => {
{'there'}
</textarea>,
),
).toWarnDev(
'Use the `defaultValue` or `value` props instead of setting children on <textarea>.',
);
}).toThrow();
).toThrow('<textarea> can only have at most one child');
}).toWarnDev(
'Use the `defaultValue` or `value` props instead of setting children on <textarea>.',
);

let node;
expect(() => {
Expand All @@ -373,10 +373,10 @@ describe('ReactDOMTextarea', () => {
<strong />
</textarea>,
)),
).toWarnDev(
'Use the `defaultValue` or `value` props instead of setting children on <textarea>.',
);
}).not.toThrow();
).not.toThrow();
}).toWarnDev(
'Use the `defaultValue` or `value` props instead of setting children on <textarea>.',
);

expect(node.value).toBe('[object Object]');
});
Expand Down
13 changes: 7 additions & 6 deletions packages/react-dom/src/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -646,12 +646,13 @@ describe('ReactDOMServer', () => {
expect(() => {
expect(() =>
ReactDOMServer.renderToString(<ClassWithRenderNotExtended />),
).toWarnDev(
'Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, ' +
"but doesn't extend React.Component. This is likely to cause errors. " +
'Change ClassWithRenderNotExtended to extend React.Component instead.',
);
}).toThrow(TypeError);
).toThrow(TypeError);
}).toWarnDev(
'Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, ' +
"but doesn't extend React.Component. This is likely to cause errors. " +
'Change ClassWithRenderNotExtended to extend React.Component instead.',
{withoutStack: true},
);

// Test deduplication
expect(() => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

const rule = require('../no-to-warn-dev-within-to-throw');
const RuleTester = require('eslint').RuleTester;
const ruleTester = new RuleTester();

ruleTester.run('eslint-rules/no-to-warn-dev-within-to-throw', rule, {
valid: [
'expect(callback).toWarnDev("warning");',
'expect(function() { expect(callback).toThrow("error") }).toWarnDev("warning");',
],
invalid: [
{
code:
'expect(function() { expect(callback).toWarnDev("warning") }).toThrow("error");',
errors: [
{
message: 'toWarnDev() matcher should not be nested',
},
],
},
],
});
1 change: 1 addition & 0 deletions scripts/eslint-rules/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
module.exports = {
rules: {
'no-primitive-constructors': require('./no-primitive-constructors'),
'no-to-warn-dev-within-to-throw': require('./no-to-warn-dev-within-to-throw'),
'warning-and-invariant-args': require('./warning-and-invariant-args'),
},
};
33 changes: 33 additions & 0 deletions scripts/eslint-rules/no-to-warn-dev-within-to-throw.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

module.exports = function(context) {
return {
Identifier(node) {
if (node.name === 'toWarnDev') {
let current = node;
while (current.parent) {
if (current.type === 'CallExpression') {
if (
current &&
current.callee &&
current.callee.property &&
current.callee.property.name === 'toThrow'
) {
context.report(node, 'toWarnDev() matcher should not be nested');
}
}
current = current.parent;
}
}
},
};
};
Loading

0 comments on commit e106b8c

Please sign in to comment.