-
Notifications
You must be signed in to change notification settings - Fork 15
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
tests: move test module from unit/ to integration/ #167
Conversation
Codecov Report
@@ Coverage Diff @@
## main #167 +/- ##
==========================================
+ Coverage 92.91% 94.54% +1.62%
==========================================
Files 7 7
Lines 1044 1044
Branches 192 192
==========================================
+ Hits 970 987 +17
+ Misses 66 53 -13
+ Partials 8 4 -4 |
@lengau one side-effect of this change is that some code became uncovered because our coverage only considers unit tests (no integration tests). I'd like to ask your opinion on how to fix this. We have two straightforward options:
Disclaimer: the uncovered code is all related to the new "streaming brief" feature that I recently added. I actually only added bigger, "integration-ish" tests on purpose, to try and test the public API as a regular app would. I thought about adding unit tests for the individually-touched classes but with the mocking that would entail it felt like a lot of "busy work" with little gain - it felt like it would just make future refactorings/changes harder for no "real" gain in the presence of the bigger tests that I did add. I do realize that this opinion is kind of subjective and many people would disagree. What do you think? |
@tigarmo I think for |
The test_messages_integration module, as the name implies, contains a lot of integration-level tests with little-to-no mocking. For clarity's sake and to keep consistent with the other Craft libraries, move the module to the integration/ folder (which is under-represented anyway).
It makes sense to include the integration tests in the total coverage metric because in craft-cli the integration tests are more about testing the actual public api end-to-end. Often, these are the tests that better reflect the real-world use of the package and the tests that have a better chance of surviving true internal refactorings without changes.
83bf912
to
911dcff
Compare
@lengau OK, I added another commit adding coverage to integration tests. This meant that I had to disable the "local" coverage "fail_under" check because well, the integration tests don't cover all code and I don't know if we can get that check to take into account both unit and integration tests, in tox. |
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.
@tigarmo I think that's totally reasonable.
I've made a note to reconsider our coverage strategy overall at canonical/starbase#116
The test_messages_integration module, as the name implies, contains a lot of integration-level tests with little-to-no mocking. For clarity's sake and to keep consistent with the other Craft libraries, move the module to the integration/ folder (which is under-represented anyway).
tox
?