-
Notifications
You must be signed in to change notification settings - Fork 117
[bugfix] Fix retry report when test fails before the setup phase #535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
vkarak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks good, but we need a unit test that triggers the bug. We have a special regression test in the frontend_checks.py for emulating this behaviour: the BadSetupCheckEarly.
|
I could trigger the bug with the |
|
@jenkins-cscs retry daint |
unittests/test_policies.py
Outdated
| self.assertEqual(max_retries, rt.runtime().current_run) | ||
| self.assertEqual(1, self.runner.stats.num_failures()) | ||
|
|
||
| def test_retries_bad_check_early(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this test be combined with the previous one? Just make it run both bad checks. The tests are almost identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I merged the two tests. Done.
unittests/test_policies.py
Outdated
| # Ensure that the report succeeds and that the output is as expected. | ||
| report = self.runner.stats.retry_report() | ||
| self.assertIn("Test BadSetupCheckEarly was retried 2 time(s) and " | ||
| "failed.", report) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is quite restrictive. We will have to update the unit test every time we change either the automatic test naming scheme or message. I think we were getting a stack trace. You could then check that no Traceback is found in the output as we do with other similar cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the check is even easier here. I think retry_report() was raising an exception. So you may simply call it. If it raises an exception, the unit test will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually wanted to do something like self.assertNoExcepetion(self.runner.stats.retry_report()) at that point but such an assert function does not exist, nor any similar. So I just do self.runner.stats.retry_report() instead, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you simply call. If an unexpected exception is thrown, the unit tests will fail.
vkarak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @omlins, lgtm
Fixes #531.