Flex tests #192
Flex tests #192
Conversation
| className: PropTypes.string, | ||
| gutterSize: PropTypes.oneOf(GUTTER_SIZES), | ||
| columns: PropTypes.oneOf(COLUMNS).isRequired, | ||
| */ |
| describe('props', () => { | ||
| test('children', () => { | ||
| const component = render( | ||
| <EuiFlexGrid {...requiredProps}> |
There was a problem hiding this comment.
We can remove the requiredProps from every test except for the first one. We just need to assert that they're passed through in one test and then we can forget about them. This applies to the other suites, too. This will reduce noise in the snapshots.
| afterAll(() => { | ||
| console.warn = consoleWarn; | ||
| console.error = consoleError; | ||
| }); |
There was a problem hiding this comment.
Why do we stub these? Where are we using warn and error?
There was a problem hiding this comment.
Ah, I should have explained. I have a couple of tests that try and pass in invalid property values and I want to ensure those result in a the default react warning. I've done this in the past, where I'll change console.warn and console.error to actually throw exceptions which are catchable and easy to handle in a testing environment.
Do you think there is value in this?
There was a problem hiding this comment.
Oh that makes a ton of sense! Neat solution! I think this would benefit from being abstracted into a service in src/test. Something like this...
beforeAll(() => {
startThrowingReactWarnings(console);
});
afterAll(() => {
stopThrowingReactWarnings(console);
});What do you think?
There was a problem hiding this comment.
Yea sounds great!
| afterAll(() => { | ||
| console.warn = consoleWarn; | ||
| console.error = consoleError; | ||
| }); | ||
|
|
||
| describe('EuiFlexGroup', () => { |
There was a problem hiding this comment.
We could probably update the first test in these suites to pass in some children and verify that they're rendered. Then we can remove the specific children test below.
| console.warn = consoleWarn; | ||
| console.error = consoleError; | ||
| }); | ||
|
|
||
| describe('EuiFlexItem', () => { |
There was a problem hiding this comment.
Can we update this test to verify that children are rendered?
| @@ -27,4 +39,31 @@ describe('EuiFlexItem', () => { | |||
| expect(propType({ grow: value }, `grow`) instanceof Error).toBe(true) | |||
| ); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
Can we add a block to test the grow prop, and create snapshot tests for the input values of [true, false, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]? We'll need to update the component to export validGrowNumbers so we can refer to that array when building this array of input values. And we should probably rename it GROW_SIZES for consistency.
There was a problem hiding this comment.
Should that value list match this list?
There was a problem hiding this comment.
Yeah I think that would make sense if they were the same.
src/test/react_warnings.js
Outdated
| We want our unit tests to verify that invalid property values will | ||
| trigger the default react warning message. It feels easier to capture | ||
| that scenario as an actual throwing error so these two utility functions | ||
| allow you to do that. |
There was a problem hiding this comment.
Maybe rephrase this as:
Use this utility to throw errors whenever React complains via the console about things like invalid propTypes. This lets us assert that a propType check works correctly with `toThrow`.
References #177
Adds tests for: