Add tests for ceremony edge cases & improve type annotations

[MVrachev]

Description

Add:

• test we cannot add online key as one of root keys
• requirement that threshold > 2 & add test for that
• test that expiration days must be a positive number

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

Related to #533

Additional requirements

• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

Code of Conduct

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

• I agree to follow this project's Code of Conduct

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MVrachev]

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]

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

[MVrachev]

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

[lukpueh]

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?

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.

[lukpueh]

This assert would also pass, if there were less than threshold keys, which is probably not what you want. Also, given that you control that threshold and number of keys are equal, why not assert for that?

[MVrachev]

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]

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

[MVrachev]

Again, we need to have repository-service-tuf/repository-service-tuf#742 merged, before ft tests pass here.