-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add tests from Reagent's test suite #6
Comments
@rafd I'm planning to address this issue, but if you'd be interested, let me know. |
@alysbrooks I added attribute conversion tests for my patch, which test the behaviour of Reagent plus React (b/c React also does additional stuff on top of Reagent). I don't have bandwidth atm to integrate the Reagent tests, so please go ahead. |
@rafd No problem, just thought I'd offer. |
Not sure where this falls on our priorities. It would be more of an effort to pre-emptively find gaps or bugs than addressing a specific need. On the other hand, it would be a relatively cheap way to increase our confidence we are compatible with Reagent. One way to look at it is, if we are already compatible, incorporating these tests will probably go relatively quickly since they'll pass. If we aren't, it will take longer, but that means we've found actual bugs. When I worked on #4 there were some edge cases to consider, but it went relatively quickly, and I was able to document those differences. The main delay was waiting for another PR to merge (and then having to rebase it, which I have yet to do). |
Since we're going for Reagent compatibility, it makes sense to leverage their test suite.
https://github.com/reagent-project/reagent/blob/master/src/reagent/impl/template.cljs
Follow up of #4 and #5.
The text was updated successfully, but these errors were encountered: