Use a proper test runner and actually assert expected behavior #18
Conversation
|
Any feedback on this PR? |
There was a problem hiding this comment.
Hey @lo1tuma this is awesome!
I'm working on #17 and I was going to add a test runner myself since I needed to do proper testing there.
Code looks good. I have only one question.
Also, for a future PR we could add a more extended set of test cases to cover different types of reported errors. But that would be for a follow up PR. If you want to do it feel free, otherwise I'll do it.
| const result = PrimitiveType.decode(''); | ||
|
|
||
| t.deepEqual(reporter(result), []); | ||
| t.end(); |
There was a problem hiding this comment.
[Q]: I'm not familiar with tape. Is t.end required for synchronous tests? Otherwise we could remove it.
There was a problem hiding this comment.
It depends. If you are using t.plan() to plan the amount of assertions then t.end() is not required.
See the documentation here.
I’m also open for other test runners, personally I use ava quite often. But since this project is quite small and doesn’t need things like async testing I thought a lightweight and fast test runner might be enough.
There was a problem hiding this comment.
I've been working with Jest for 4 years already. So that's all that I know. But I'm fine with new stuff, specially if it's lightweight. We can change it later, it's not a big project.
There was a problem hiding this comment.
this is already past, but @gillchristian fyi ava is near the same as tape in the style of its API, doesn't require the t.end() call, and has cleaner output - so if you'd prefer that I could drop that in in like 3 minutes. although this doesn't and won't do anything async at least you dont need to do that :)
though I recommend against jest since it's huge and includes a bunch of stuff that this project doesn't need like automocking and whatever, as well as relying on globals and babel transforms and stuff which makes configuration end up way more complex. tape and ava are both super simple and do anything this library will probably ever need.
There was a problem hiding this comment.
I'm fine with AVA as well. Used it when it just came out and I liked it.
@lo1tuma wdyt?
There was a problem hiding this comment.
AVA is my favourite test runner anyway, so I’m 👍 for this change.
I agree. Actually I did this PR because I was planning to make a follow-up PR to solve an issue when using intersections, but I want to make sure I don’t break the existing functionality. |
The current test setup only printed some log messages, without checking the result. So even if a breaking change is introduced the tests wouldn’t fail. That’s why I think it is a good idea to use a proper test runner and do proper assertions.