Bug: some integration tests are passing without any assertion

victorperin / qr-scanner-cli

A CLI tool to read QR Code from images

https://victorperin.github.io/qr-scanner-cli/

MIT License

73 stars 24 forks source link

Bug: some integration tests are passing without any assertion #72

Closed victorperin closed 4 years ago

victorperin commented 4 years ago

I noticed some integration tests are not being executed until end (mostly when you must trow an error or something like that).

Yurickh commented 4 years ago

I'm into it, but I have a couple of questions. The last two integration tests are not actually passing when you fix the described issue. Should I also fix them in the same PR, or should I just ignore them for the time being?

The problem is that by fixing those, the unit tests for scanFromFile start to fail. The reason for that is that the scanFromFile tests assume it will just bubble up whatever error message it receives from its dependencies, whereas the integration tests assume they will have nice formatted messages (described in the ERROR constant).

Would love to have your input here :)~

Feel free to check the current state of the branch here: https://github.com/Yurickh/qr-scanner-cli/tree/fix-async-tests

victorperin commented 4 years ago

Yes, we noticed it when I was testing the other PR. Please, let them fail (because they already are). I'm working on a way to fix them.

Yurickh commented 4 years ago

I'm not sure if I understood correctly, but I can't create commits with failing tests due to the pre-commit hooks. I see six possible ways going forward:

I skip the tests and open the PR as it is right now;
I comment the fix for the tests, so you can just uncomment them once fixed;
I change the integration tests so they assert against error messages created by dependencies;
I change the unit tests so they don't expect to just log whatever message is sent by deps;
Move error handling to outside of scanFromFile, so it just throws when errors. execution would be the one handling errors. That makes it possible to keep integration tests with custom messages, and removes the need for the error tests in the unit tests for scanFromFile, adding them to execution instead;
I wait for you to do your preferred fix for the error message problem and merge the fix for the tests afterwards.

What's your take on this? :)~

victorperin commented 4 years ago

Hey @Yurickh, thanks for remind me of those hooks.

This is my opinion, but feel free to do what you think is the best.

[x] update those tests to fail. and ignore those hooks. (using --no-verify on commit command)
[x] I merge those failing tests to show people we are failing. (also, this will prevent any release so we are safer that way)
[x] We fix those tests in a new PR
[ ] We move those error handling (I've already having this problem to implement #61) outside scanFromFile, to a new file, inside CLI folder. (to prevent making execution file too complex.)

Yurickh commented 4 years ago

Opened the PR with the failing tests, as suggested :)~

victorperin commented 4 years ago

Reopened because pr automatically closed this issue.

Yurickh commented 4 years ago

Arguably the "some integration tests are passing without any assertion" has been fixed, since now they're failing 😅 Usually, I would open another issue with the description of the mismatch of the logging, so whoever takes it will have more context to fix it.

victorperin commented 4 years ago

Finnaly fixed. :)