search

psyinfra / onyo

text-based inventory system on top of git
ISC License
3 stars 5 forks source link

BF: `onyo get` fixes #607

Closed aqw closed 3 months ago

aqw commented 4 months ago

This PR fixes the following bugs:

As a brief digression... the test test_onyo_get_reserved_keys() ended up running into bug #583. Printing path twice exceeded the 80 char limit that Rich detects when running in a virtual TTY.

I looked into how to increase the faux-terminal width, but I could not find a reasonable way. This rich bug was infuriatingly closed with no explanation, and the rich docs make no mention of capsys, and only pytest once...

Other suggestions involve monkey patching Popen, but it wasn't clear to me the right way to mock this through fixtures.

And at that point... I decided that if we don't want output to be interfered with, then we should use machine-readable=True.

codecov[bot] commented 4 months ago

Codecov Report

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

Project coverage is 95.71%. Comparing base (da03fc8) to head (3e78b70).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #607 +/- ## ========================================== + Coverage 95.70% 95.71% +0.01% ========================================== Files 71 71 Lines 5608 5625 +17 Branches 1097 1100 +3 ========================================== + Hits 5367 5384 +17 Misses 141 141 Partials 100 100 ```

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

aqw commented 3 months ago

Overall LGTM.

Great. :-)

One thing, though: Is there a reason to not add directory to PSEUDO_KEYS?

That was not intentional. Though, my general frame of mind was to fix a few get bugs, and the entire PSEUDO vs RESERVED key thing was a separate task.

Which leads to the next question: I'm not sure why there aren't test failures. A bunch of assertions should differ when a key is or isn't a PSEUDO_KEY.

Yeah; I agree that doesn't smell right. And judging from what I've seen in the get tests, I'm not too surprised. Lots of leaking assumptions.

For kicks, I made the change locally, to correct RESERVED_KEYS and PSEUDO_KEYS, and indeed there are failures. So some tests are likely making faulty assumptions.

I think the question is whether that cleanup should happen in this PR or a separate one. I'm happy with either. Preferences?

bpoldrack commented 3 months ago

For kicks, I made the change locally, to correct RESERVED_KEYS and PSEUDO_KEYS, and indeed there are failures. So some tests are likely making faulty assumptions.

Ok. So, if it is indeed this way around, let's turn it into an issue that needs consideration when turning our attention to tests rather than figuring it out in this PR.

aqw commented 3 months ago

Sounds good. :+1: