-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Adds button component tests. #926
Conversation
Adds basic button component tests. Covers cases where the tag name or other properties are conditionally rendered. Related to progress on #641. This PR adds the components directory to the list of entry points configured for tests by webpack.
I think we should merge this, it's a really large and important area of our code that we've neglected to test so far. Separately, we should consider making |
Are the changes you made the test conventions I should use moving forward with using .be? |
I think the components directory as an entry point is only used via tests really, there is no "entry" point persee in the components directory it is just a bunch of directories? I am probably not understanding what you mean, so if you could clarify it would help me. How does this duplicate between built files, and what does that mean? I am not a webpack guru yet. |
It's not incredibly important either way, but I do prefer the shorter assertion form where possible. Normally in Chai these are just property assertions like In this repo we are using the Unintentionally (but fortunately), the property-based assertions are flagged as errors in our linting setup. |
Yeah I noticed that it expects an assignment. I guess that works in our favor! |
For example, we import In any case, this is a pre-existing issue and should be handled separately. Probably @aduth and @youknowriad have already discussed it at some point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
Okay, I can do that. Did not know things duplicate, if a separate build is not provided, somewhat makes sense though. |
Not going to merge until I resolve the webpack thing, then I will rebase this. |
I would actually prefer that we go with the Webpack approach you took here, then resolve that separately. It's not an issue with this PR. |
I tried to split out |
@@ -50,6 +51,7 @@ | |||
"pegjs-loader": "^0.5.1", | |||
"postcss-loader": "^1.3.3", | |||
"raw-loader": "^0.5.1", | |||
"react-test-renderer": "^15.5.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enzyme won't work without this since we are on the latest version of React. As we bump our React version we will need to bump this as well to match. This could be one of many reasons why testing component rendering will be more of a hassle than a positive. For now I think we can continue forward and just yank them if needed.
Adds basic button component tests. Covers cases where the tag name or
other properties are conditionally rendered. Related to progress on
#641. This PR adds the components directory to the list of entry points
configured for tests by webpack.
This introduces Enzyme and the react-test-renderer library as dev dependencies.
Testing Instructions
Run
npm i && npm run test-unit
ensure tests pass. Change Button logic to ensure tests fail as they should.