search

TheBrainFamily / wait-for-expect

Wait for expectation to be true, useful for integration and end to end testing. Integral part of react-testing-library.
MIT License
293 stars 31 forks source link

Return on reject #11

Closed iceberk closed 6 years ago

iceberk commented 6 years ago

Fix for #10

lgandecki commented 6 years ago

Looks good. @kentcdodds do you think this is something worth having a test for? I'm curious if you'd go the extra mile here with some kind of an integration test.

kentcdodds commented 6 years ago

I would because all of my published packages have 100% code coverage. But it's up to you and how you want to maintain this package.

iceberk commented 6 years ago

I'd love to write some tests, just don't know how to do it properly for this case. Please share your ideas and I will try to implement them.

lgandecki commented 6 years ago

@kentcdodds I'm just curious to learn. This is one of the gray areas for me. This return doesn't change anything in terms of "test coverage" - it's still 100% even if the test is not added. But what is that really telling us? For me, the real 100% coverage (in opposition to reported coverage) is when I can't change anything in my code without breaking tests.

@iceberk If we wanted to do an integration testing, you would have to build a test that let's say increments some kind of a counter, and then a verification that after the expect fails (in try/catch), the counter doesn't change anymore. I would recommend trying to write the test first and see it fail. (because it's easy to write a test like this that will always pass)

kentcdodds commented 6 years ago

@lgandecki, good point. Unless it's a pretty edgy case, then I generally put tests for things like this in a "bugs" test file which holds tests for bugs that have been found and fixed. I think this is a bit of an edge case and we probably don't need tests for it. Honestly the code should have been doing this the whole time and I don't expect this bug to show up again.