repository-service-tuf / repository-service-tuf-cli

Repository Service for TUF: Command Line Interface
https://pypi.org/project/repository-service-tuf/
MIT License
8 stars 15 forks source link

Add tests for ceremony edge cases & improve type annotations #607

Closed MVrachev closed 3 weeks ago

MVrachev commented 1 month ago

Description

Add:

Additionally, improve type annotations in a few places and move ceremony tests input inside conftest to be reused.

Related to #533

Additional requirements

Code of Conduct

By submitting this PR, you agree to follow our Code of Conduct.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.93%. Comparing base (47d0a69) to head (66a1bbc). Report is 90 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #607 +/- ## ========================================== + Coverage 98.90% 98.93% +0.03% ========================================== Files 20 26 +6 Lines 1367 1877 +510 ========================================== + Hits 1352 1857 +505 - Misses 15 20 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MVrachev commented 1 month ago

Not sure how this not covered line: https://app.codecov.io/gh/repository-service-tuf/repository-service-tuf-cli/pull/607/blob/repository_service_tuf/helpers/tuf.py#L272 is part of my pr, but will check if I can do something about it.

MVrachev commented 1 month ago

I decided to just ignore it as it's part of the old md update which will be soon refactored.

MVrachev commented 4 weeks ago

FT fix is required before merging this pr. Have a look at: https://github.com/repository-service-tuf/repository-service-tuf/pull/742

MVrachev commented 4 weeks ago

This looks good. I have one inline comment, which should be addressed, and two high level comments, which aren't blockers:

Generally, I think it's not feasible to test all code branches of the cli tool. Maybe it would be enough to just test this feature on the related helper?

Well, that's a discussion worth having. I am okay with having helper-specific tests instead of full-flow tests for those cases specific to the helpers. For example, for threshold, we could test the helper itself only. Still, given that the test is there and working I won't change it now.

If you do test the ceremony command, I recommend to store the complete list of inputs separately for each invocation, even if there is some redundancy. Re-using some parts (e.g input_step1, input_step3) and patching others (input_step2) makes the test a bit harder to read.

I disagree. I prefer reading what is changed as to have all of the inputs there and wonder are there any changes in any of the steps. When you change only for example input2, then you know that the change is ONLY related to the root keys configuration.

MVrachev commented 4 weeks ago

Rebased on top of main. The only new/changed commit is the last one addressing @lukpueh comment.

MVrachev commented 4 weeks ago

Again, we need to have https://github.com/repository-service-tuf/repository-service-tuf/pull/742 merged, before ft tests pass here.