-
-
Notifications
You must be signed in to change notification settings - Fork 638
Added some basic test coverage for the helpers #43
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
Conversation
mapreal19
commented
Sep 28, 2015
- Resolves Add Unit tests #5
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.
@samnang @justin808 am I missing something here? I'm getting console
is undefined on the specs
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.
be sure that you're working off of the latest. Rebase your changes on top of the latest.
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.
@mapreal19 We're doing a lot of reworking of the internals, so I'd first focus on the capybara tests. Especially important to get a test that verifies the created HTML for server rendering.
You might want to use the same driver we use in https://github.com/shakacode/react-webpack-rails-tutorial
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.
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.
@mapreal19 I totally agree with @justin808 . We start with integration level is better because a lot of changes haven't merged, but the functionality should be the same.
@mapreal19 Can you tune this up, and let's merge this if appropriate. |
@robwise Can you pull this into your own fork and see if this helps with coverage? Thanks. |
@justin808 Coverage is the same, but it slightly increases the hits per line. Also, two of the tests were failing because I believe they were calling the wrong method, so I fixed those. One was also expecting a message that actually had an additional line return and blank space in the result. I matched it to pass, but I'm not sure if that's what we want or not. The tests are not redundant with what we've already got, so I think they have value regardless. |
@robwise can you rebase this on top of master? |
@justin808 Done just now, it's up on #66 Do we have any more info about the whitespace thing? |
@robwise Regarding the whitspace thing, you'd want to look at the source. My guess is that there is just whitespace in the ERB file around the tag. |
* React on Rails Pro Prep for Production * JS evaluation caching * Advanced error handling * Many improvements to the test suite * Doc improvements * Based on React on Rails 11.0.7 and higher
* React on Rails Pro Prep for Production * JS evaluation caching * Advanced error handling * Many improvements to the test suite * Doc improvements * Based on React on Rails 11.0.7 and higher