test: Add basic test for cli module#74
test: Add basic test for cli module#74Valentin-v-Todorov merged 2 commits intoTheDevOpsBlueprint:mainfrom
Conversation
Add a test for the cli module. This does not provide much cli coverage yet, but given the recent breakage I figured it was worth getting a start of cli tests.
skalwaghe-56
left a comment
There was a problem hiding this comment.
I request you to add tests for logic! Not asserting messages. @Valentin-v-Todorov I think there is no need for tests which assert messages. We need tests which test actual logic!
|
I suggest you to add logic checks too! I have no problem with your current checks but you should add logic checks too! They are very important, and they should be more focused on. |
skalwaghe-56
left a comment
There was a problem hiding this comment.
@amcintosh I can still see none logic checks. You can keep these output statements assetion logic there, but add core application logic checks. To check the actual application and not only the outputs. The current changes lgtm but please make the requested changes too. Thanks!
| assert "✔ Added alias: alix-test-echo = 'alix test working!'" in result.output | ||
| assert ( | ||
| "💡 Alias 'alix-test-echo' is now available in new shell sessions" | ||
| in result.output | ||
| ) | ||
| assert "For current session, run: source ~/.zshrc" in result.output |
There was a problem hiding this comment.
I see that you are still asserting the output messages only.
There was a problem hiding this comment.
The assertions are on lines 38-41. These are unit tests so I'm confining the scope of the test to the code in the cli.py module. The logic underneath would be tested in test_storage.py and presumably a test_shell_integrator.py.
|
@amcintosh LGTM! Nice job! |
|
@amcintosh Had you not run the tests locally? These are failing please fix them and open a new PR. Make sure they pass for now reverting these changes. |
|
@skalwaghe-56 I have run them all locally. I suspect it is due to other changes merged in. Hard to get test coverage up on a moving target. |
|
@skalwaghe-56, @Valentin-v-Todorov: PR to re-add here: #80 |
PR Checklist
What does this PR do?
Add a test for the cli module. This does not provide much cli coverage yet, but given the recent breakage I figured it was worth getting a start of cli tests.
Coverage:
Related Issue
Fixes #29
Type of change