Skip to content

Commit e106b8c

Browse files
authored
Warn about unsafe toWarnDev() nesting in tests (facebook#12457)
* Add lint run to warn about improperly nested toWarnDev matchers * Updated tests to avoid invalid nesting
1 parent 026aa9c commit e106b8c

8 files changed

Lines changed: 160 additions & 84 deletions

File tree

.eslintrc.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ module.exports = {
6565
// CUSTOM RULES
6666
// the second argument of warning/invariant should be a literal string
6767
'react-internal/no-primitive-constructors': ERROR,
68+
'react-internal/no-to-warn-dev-within-to-throw': ERROR,
6869
'react-internal/warning-and-invariant-args': ERROR,
6970
},
7071

packages/react-dom/src/__tests__/ReactCompositeComponent-test.js

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -428,12 +428,13 @@ describe('ReactCompositeComponent', () => {
428428
expect(() => {
429429
expect(() => {
430430
ReactDOM.render(<ClassWithRenderNotExtended />, container);
431-
}).toWarnDev(
432-
'Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, ' +
433-
"but doesn't extend React.Component. This is likely to cause errors. " +
434-
'Change ClassWithRenderNotExtended to extend React.Component instead.',
435-
);
436-
}).toThrow(TypeError);
431+
}).toThrow(TypeError);
432+
}).toWarnDev(
433+
'Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, ' +
434+
"but doesn't extend React.Component. This is likely to cause errors. " +
435+
'Change ClassWithRenderNotExtended to extend React.Component instead.',
436+
{withoutStack: true},
437+
);
437438

438439
// Test deduplication
439440
expect(() => {
@@ -1728,11 +1729,18 @@ describe('ReactCompositeComponent', () => {
17281729
expect(() => {
17291730
expect(() => {
17301731
ReactTestUtils.renderIntoDocument(<RenderTextInvalidConstructor />);
1731-
}).toWarnDev(
1732+
}).toThrow();
1733+
}).toWarnDev(
1734+
[
1735+
// Expect two errors because invokeGuardedCallback will dispatch an error event,
1736+
// Causing the warning to be logged again.
17321737
'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned component instance: ' +
17331738
'did you accidentally return an object from the constructor?',
1734-
);
1735-
}).toThrow();
1739+
'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned component instance: ' +
1740+
'did you accidentally return an object from the constructor?',
1741+
],
1742+
{withoutStack: true},
1743+
);
17361744
});
17371745

17381746
it('should return error if render is not defined', () => {
@@ -1741,10 +1749,17 @@ describe('ReactCompositeComponent', () => {
17411749
expect(() => {
17421750
expect(() => {
17431751
ReactTestUtils.renderIntoDocument(<RenderTestUndefinedRender />);
1744-
}).toWarnDev(
1752+
}).toThrow();
1753+
}).toWarnDev(
1754+
[
1755+
// Expect two errors because invokeGuardedCallback will dispatch an error event,
1756+
// Causing the warning to be logged again.
17451757
'Warning: RenderTestUndefinedRender(...): No `render` method found on the returned ' +
17461758
'component instance: you may have forgotten to define `render`.',
1747-
);
1748-
}).toThrow();
1759+
'Warning: RenderTestUndefinedRender(...): No `render` method found on the returned ' +
1760+
'component instance: you may have forgotten to define `render`.',
1761+
],
1762+
{withoutStack: true},
1763+
);
17491764
});
17501765
});

packages/react-dom/src/__tests__/ReactDOMTextarea-test.js

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -359,10 +359,10 @@ describe('ReactDOMTextarea', () => {
359359
{'there'}
360360
</textarea>,
361361
),
362-
).toWarnDev(
363-
'Use the `defaultValue` or `value` props instead of setting children on <textarea>.',
364-
);
365-
}).toThrow();
362+
).toThrow('<textarea> can only have at most one child');
363+
}).toWarnDev(
364+
'Use the `defaultValue` or `value` props instead of setting children on <textarea>.',
365+
);
366366

367367
let node;
368368
expect(() => {
@@ -373,10 +373,10 @@ describe('ReactDOMTextarea', () => {
373373
<strong />
374374
</textarea>,
375375
)),
376-
).toWarnDev(
377-
'Use the `defaultValue` or `value` props instead of setting children on <textarea>.',
378-
);
379-
}).not.toThrow();
376+
).not.toThrow();
377+
}).toWarnDev(
378+
'Use the `defaultValue` or `value` props instead of setting children on <textarea>.',
379+
);
380380

381381
expect(node.value).toBe('[object Object]');
382382
});

packages/react-dom/src/__tests__/ReactServerRendering-test.js

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -646,12 +646,13 @@ describe('ReactDOMServer', () => {
646646
expect(() => {
647647
expect(() =>
648648
ReactDOMServer.renderToString(<ClassWithRenderNotExtended />),
649-
).toWarnDev(
650-
'Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, ' +
651-
"but doesn't extend React.Component. This is likely to cause errors. " +
652-
'Change ClassWithRenderNotExtended to extend React.Component instead.',
653-
);
654-
}).toThrow(TypeError);
649+
).toThrow(TypeError);
650+
}).toWarnDev(
651+
'Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, ' +
652+
"but doesn't extend React.Component. This is likely to cause errors. " +
653+
'Change ClassWithRenderNotExtended to extend React.Component instead.',
654+
{withoutStack: true},
655+
);
655656

656657
// Test deduplication
657658
expect(() => {
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/**
2+
* Copyright (c) 2015-present, Facebook, Inc.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @emails react-core
8+
*/
9+
10+
'use strict';
11+
12+
const rule = require('../no-to-warn-dev-within-to-throw');
13+
const RuleTester = require('eslint').RuleTester;
14+
const ruleTester = new RuleTester();
15+
16+
ruleTester.run('eslint-rules/no-to-warn-dev-within-to-throw', rule, {
17+
valid: [
18+
'expect(callback).toWarnDev("warning");',
19+
'expect(function() { expect(callback).toThrow("error") }).toWarnDev("warning");',
20+
],
21+
invalid: [
22+
{
23+
code:
24+
'expect(function() { expect(callback).toWarnDev("warning") }).toThrow("error");',
25+
errors: [
26+
{
27+
message: 'toWarnDev() matcher should not be nested',
28+
},
29+
],
30+
},
31+
],
32+
});

scripts/eslint-rules/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
module.exports = {
44
rules: {
55
'no-primitive-constructors': require('./no-primitive-constructors'),
6+
'no-to-warn-dev-within-to-throw': require('./no-to-warn-dev-within-to-throw'),
67
'warning-and-invariant-args': require('./warning-and-invariant-args'),
78
},
89
};
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* Copyright (c) 2015-present, Facebook, Inc.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @emails react-core
8+
*/
9+
10+
'use strict';
11+
12+
module.exports = function(context) {
13+
return {
14+
Identifier(node) {
15+
if (node.name === 'toWarnDev') {
16+
let current = node;
17+
while (current.parent) {
18+
if (current.type === 'CallExpression') {
19+
if (
20+
current &&
21+
current.callee &&
22+
current.callee.property &&
23+
current.callee.property.name === 'toThrow'
24+
) {
25+
context.report(node, 'toWarnDev() matcher should not be nested');
26+
}
27+
}
28+
current = current.parent;
29+
}
30+
}
31+
},
32+
};
33+
};

0 commit comments

Comments
 (0)