-
Notifications
You must be signed in to change notification settings - Fork 117
[feat] Use different stage and output directories for different retries of the same test #464
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
[feat] Use different stage and output directories for different retries of the same test #464
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.
Looks good, except for some refinement of the API. I don't like very much the current_run as a public name. See my suggestion.
reframe/core/runtime.py
Outdated
| self.timestamp) | ||
| else: | ||
| return os.path.join(self.outputdir, self.timestamp) | ||
| return os.path.join(os.path.normpath(self.outputdir) + |
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.
Why do you need to normalize the path here? The outputdir and stagedir are automatically converted to absolute paths, thus they are already normalized.
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 did not realize that is already guaranteed that self.outputdir and self.stagedir are normalized. I will remove it then.
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.
Done.
unittests/test_policies.py
Outdated
| self.assertEqual(1, stats.num_failures()) | ||
|
|
||
| # Retries tests are executed in a different runtime as they modify | ||
| # the global rt.runtime().current_run what makes subsequent tests 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.
This comment is not very accurate. You should say that the policy tests need a freshly initialized runtime, that's why we need to switch before every test.
|
Another thing is to augment the unit tests to check that the directory suffixes are created as expected. |
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.
lgtm.
reframe/core/runtime.py
Outdated
| :type: `integer` | ||
| """ | ||
| # Not publicly documented |
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 comment is not actually needed.
Codecov Report
@@ Coverage Diff @@
## master #464 +/- ##
==========================================
+ Coverage 91.34% 91.36% +0.01%
==========================================
Files 70 70
Lines 8784 8790 +6
==========================================
+ Hits 8024 8031 +7
+ Misses 760 759 -1
Continue to review full report at Codecov.
|
Includes the migration of the
current_runvariable to the class RuntimeContext and related changes.Closes #403.