canonical / craft-cli

https://canonical-craft-cli.readthedocs-hosted.com/en/latest/
GNU Lesser General Public License v3.0
9 stars 15 forks source link

tests: move test module from unit/ to integration/ #167

Closed tigarmo closed 1 year ago

tigarmo commented 1 year ago

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

codecov[bot] commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

tigarmo commented 1 year ago

@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