Skip to content
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

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

tigarmo
Copy link
Collaborator

@tigarmo tigarmo commented Aug 11, 2023

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).

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Merging #167 (911dcff) into main (deeb6de) will increase coverage by 1.62%.
The diff coverage is n/a.

@@            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     

see 1 file with indirect coverage changes

@tigarmo
Copy link
Collaborator Author

tigarmo commented Aug 15, 2023

@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:

  1. I can add some unit tests for the now-uncovered code;
  2. We can take the integration tests into account for the coverage too.

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?

@lengau
Copy link
Contributor

lengau commented Aug 15, 2023

@tigarmo I think for craft-cli and possibly some of our other libraries it makes sense to include integration tests in coverage. I'm more hesitant on the application layer, but I agree that unit testing this would likely not gain us anything other than more unit tests to update when we make changes.

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.
@tigarmo
Copy link
Collaborator Author

tigarmo commented Aug 15, 2023

@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.
But I think that's fine, we mostly rely on the github integration anyway

@tigarmo tigarmo marked this pull request as ready for review August 15, 2023 19:00
Copy link
Contributor

@lengau lengau left a 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

@tigarmo tigarmo merged commit 56ad8e7 into main Aug 15, 2023
9 checks passed
@tigarmo tigarmo deleted the move-integration-test branch August 15, 2023 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants