suite: Do not send emails with --dry-run at schedule_fail #1778
Conversation
Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
|
Overall looks good, just need to add a unit test. |
| return suite_repo_path | ||
|
|
||
|
|
||
| def schedule_fail(message, name=''): | ||
| def schedule_fail(message, name='', dry_run=None): |
There was a problem hiding this comment.
Maybe we can add a unit test in teuthology/suite/test/test_util.py, for testing different scenarios of schedule failures.
teuthology/suite/test/test_init.py
Outdated
| def test_machine_type_multi_error(self): | ||
| with pytest.raises(ScheduleFailError) as exc: | ||
| main([ | ||
| '--ceph', 'main', | ||
| '--suite', 'suite_name', | ||
| '--throttle', '3', | ||
| '--machine-type', 'multi', | ||
| '--dry-run' | ||
| ]) | ||
| assert str(exc.value) == "Scheduling failed: 'multi' is not a valid machine_type. \ | ||
| Maybe you want 'gibba,smithi,mira' or similar" | ||
|
|
||
| def test_machine_type_none_error(self): | ||
| with pytest.raises(ScheduleFailError) as exc: | ||
| main([ | ||
| '--ceph', 'main', | ||
| '--suite', 'suite_name', | ||
| '--throttle', '3', | ||
| '--machine-type', 'None', | ||
| '--dry-run' | ||
| ]) | ||
| assert str(exc.value) == "Scheduling failed: Must specify a machine_type" |
There was a problem hiding this comment.
As well as unit testing these specific exceptions and making sure that the exception output is correct. We also want to make sure that with --dry-run, the function util.schedule_fail()
is not hitting smtp.sendmail() in util.schedule_fail(). We can do this by importing teuthology.suite.util.schedule_fail, patching it, and add something like assert m_schedule_fail.smtp.sendmail.called == False. You might have to mock more stuff but this should get you started.
There was a problem hiding this comment.
Perfect, thank you! Will add this!
teuthology/suite/test/test_run_.py
Outdated
| def test_teuthology_branch_nonexistent( | ||
| self, | ||
| m_git_ls_remote | ||
| ): | ||
| config.teuthology_path = None | ||
| m_git_ls_remote.return_value = None | ||
| self.args.teuthology_branch = 'no_branch' | ||
| with pytest.raises(ScheduleFailError): | ||
| self.klass(self.args) | ||
|
|
teuthology/suite/test/test_util.py
Outdated
| def test_schedule_fail(self): | ||
| with pytest.raises(ScheduleFailError) as exc: | ||
| util.schedule_fail(message="error msg") | ||
| assert str(exc.value) == "Scheduling failed: error msg" | ||
|
|
||
| def test_fetch_repo_no_branch(self): | ||
| with pytest.raises(ScheduleFailError) as exc: | ||
| util.fetch_repos("no-branch", "test1", None) | ||
| assert str(exc.value) == "Scheduling test1 failed: \ | ||
| Branch 'no-branch' not found in repo: https://github.com/ceph/ceph-ci.git!" |
There was a problem hiding this comment.
same thing here, but might not have to mock as much stuff.
VallariAg
force-pushed
the
no-mails-with-dryrun
branch
from
90e79ed to
8ab4131
Compare
|
Saw your new push, looks good! I should have addressed this in my last comment but I think we also want a unit-test case where |
Sounds good! I'll add that as well! |
VallariAg
force-pushed
the
no-mails-with-dryrun
branch
from
8ab4131 to
526dc37
Compare
|
I grep -nr "schedule_fail" and I found that: you're missing teuthology/teuthology/suite/run.py Line 649 in 526dc37 |
Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
VallariAg
force-pushed
the
no-mails-with-dryrun
branch
from
526dc37 to
2d57263
Compare
My bad, added the parameter now! |
Do not send emails with
--dry-runon schedule failures.Signed-off-by: Vallari Agrawal val.agl002@gmail.com