Feature/6 testing suite#11
Conversation
sidsun1
left a comment
There was a problem hiding this comment.
LGTM overall, though I had some comments and recommended changes before merging to main!
Great work on the github test workflow. Everything here looks great. This wasn't explicitly mentioned before, but I'd appreciate if you could add a linting/format test with ruff. Reference #12 for further context.
As for the cli changes and testing I did have a couple of questions. At this moment, I don't think that the is_falsy_value function is too helpful as I believe Typer automatically checks for null or bad input purely based on type hints. These checks can be parameterized in the test modules (more on this later).
For the test file, I think it is well written overall. I do think that you can do can do a couple of things to make it a bit more modular.
- monkeypatch is a good approach for mocking/patching, but given you modify attributes and run things at a granular level, is there perhaps a better approach? If not, that's totally okay.
- I think asserting all the unpacked values into just one tuple makes it easier and more compact. (For example
assert (total, used, free_mem) == ('764', '492', '144')). Similarly, since you are checking all of those lines in output in the TestCLI class, consider looping through a list of expected output strings. - Pytest offers very nice reusability. I think it would be a good choice to run a fixture like
@pytest.fixtureon the CliRunner obj if we are to change up the codebase to have multiple test files. We can also use the decorator@pytest.mark.parameterizefor all of those commands being run intest_monitor_default_outputs_sections. We would just need onemonkeypatch.setattrand would allow us to write more tests within that same function.
Again, great work, these are just small nitpicks that will direct the overall structure of the project!
…@pytest.mark.parametize, make assertions for values a tuple instead of separate assertions
sidsun1
left a comment
There was a problem hiding this comment.
New changes on both commits LGTM!
Features:
Fix:
Closes: #6