tests: fix missing mock in attach unit test #2785
Conversation
|
Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference) GitHub Issues: No GitHub issues are fixed by this PR. (No commits have Fixes: #### references) Launchpad Bugs: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references) Documentation: The changes in this PR do not require documentation changes. 👍 this comment to confirm that this is correct. |
| @@ -241,8 +241,10 @@ def test_lock_file_exists( | |||
| @mock.patch(M_PATH + "contract.UAContractClient.add_contract_machine") | |||
| @mock.patch("uaclient.actions.status", return_value=("", 0)) | |||
| @mock.patch("uaclient.status.format_tabular") | |||
| @mock.patch(M_PATH + "daemon") | |||
There was a problem hiding this comment.
Would it be better to mock _post_cli_attach like we do on the other tests ? The concern is that for some reason we add new functionality to that method, we might forget to mock it again here
There was a problem hiding this comment.
yes, but in this case another test needs to be added to cover _post_cli_attach, as it is being tested as part of this test.
I added this as a quick path, but I can add the extra test if that suits better
There was a problem hiding this comment.
I think that would be better tbh, since we it will be more explicit that we have a dedicated test just for that function
_post_cli_attach must be mocked to avoid leaks. The JSON part of the test removed in this commit is covered in test_event_logger. Signed-off-by: Renan Rodrigo <[email protected]>
testing only the relevant output Signed-off-by: Renan Rodrigo <[email protected]>
renanrodrigo
force-pushed
the
leak-file-rm
branch
from
224d520
to
1dfa00f
Compare
|
@lucasmoura done there |
| with contextlib.redirect_stdout(fake_stdout): | ||
| main_error_handler(action_attach)(args, cfg) | ||
|
|
||
| expected = { |
There was a problem hiding this comment.
With this new setup we are no longer testing the event_logger part of the command. Should we add an explicit test on for this as well ?
There was a problem hiding this comment.
Not sure - there are tests in tests/event_logger.py to cover that, and the text output covers the infos.
I would even go further and mock the event logger to be honest - the unit test should not be responsible for that after all.
There was a problem hiding this comment.
We have integration tests that cover json output for attach, so I think this is okay to remove.
|
CI errors are unrelated to this change |
Why is this needed?
This PR solves all of our problems because it fixes a unit test leak in
_post_cli_attach.Test Steps
Unit tests runs
Checklist
Does this PR require extra reviews?