Improve test coverage in key.py

[mccalluc]

• Towards Enhance coverage #30

This improves test coverage in key.py: It does not bring it to 100%.

• The largest missing block is in relative(): The logic here is complicated and I felt a little out of my depth.
• Capturing stdout to test the print_*() methods is possible... but before investing time there, wondering if it would make sense to replace these with methods that return formatted string? But maybe for the sake of convenience, format and print in one function is better...
• Tests now hit a line that produces a warning... It would be possible to mock warnings to make sure the warning happens where we expect, but not emit the actual warning during the test run.

_scale_intervals() brings up a couple questions:

• I have thought that assert is not the best tool for input validation, because it may not be evaluated in all environments, but this is a stylistic preference.
• Beyond that, there are useful scales with larger intervals or fewer notes... but since this is a private method, maybe it makes sense to be strict, and throw an error if this code has generated a scale with a weird structure.

This has been a good refresher on python and music theory, but I'm not blocked: I'm happy to revise if you have comments, but I don't want to create work for you either.

[zmoon]

_scale_intervals() brings up a couple questions:

I have thought that assert is not the best tool for input validation, because it may not be evaluated in all environments, but this is a stylistic preference.

I often start with assert when developing because of the concise syntax and then depending on the case usually try to move the check to the test suite or to raising appropriate exception. So feel free to change to one of these when you see that around.

Beyond that, there are useful scales with larger intervals or fewer notes... but since this is a private method, maybe it makes sense to be strict, and throw an error if this code has generated a scale with a weird structure.

Yeah, that is a good point, though all the modes we have so far have 7, so I think I probably had that as a sanity check. Useful validation checks here might be that the first one is 0 and the last one is less than 12 (so that include_upper works properly).

[zmoon]

• Capturing stdout to test the print_*() methods is possible... but before investing time there, wondering if it would make sense to replace these with methods that return formatted string? But maybe for the sake of convenience, format and print in one function is better...

Yeah I think it is not too bad with pytest. I think that is better since user already has access to the lists.

[zmoon]

• Tests now hit a line that produces a warning... It would be possible to mock warnings to make sure the warning happens where we expect, but not emit the actual warning during the test run.

Looks like pytest.warns would work in this case?

[mccalluc]

Sorry to be slow; Thanks for the feedback! -- I'll try to get some fixes in and ask for re-review before the weekend.

[mccalluc]

Some fixes. Decided not to loosen the definition of a scale: The checks currently in place are good for the scope of this project, but whatever you think best. Not up to 100% coverage of key.py, but it's closer.

[zmoon]

Thanks @mccalluc , looks like total coverage is > 90% with this.

[mccalluc]

... latest commits introduced errors. Back in my court.

[zmoon]

@mccalluc do you have time to finish this PR? I could do it if not.

[zmoon]

Hi @mccalluc just wanted to check again if you were interested in working more on this PR. If not, I may start editing soon. I want to get this in and do an official v0.1 release this summer.

[mccalluc]

If you want to take it from here, feel free! I haven't thought about this in a while, and would need to re-learn the code base. Thanks, and sorry for not seeing it to the finish line.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.00%. Comparing base (fbbe51b) to head (9a4fc63).
⚠️ Report is 91 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #44      +/-   ##
==========================================
+ Coverage   90.02%   92.00%   +1.98%     
==========================================
  Files           9        9              
  Lines        1333     1339       +6     
==========================================
+ Hits         1200     1232      +32     
+ Misses        133      107      -26     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[zmoon]

Thanks a lot @mccalluc for your help here!