-
Notifications
You must be signed in to change notification settings - Fork 111
[feat] Log all tests in perfloggers, not only performance tests #3516
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3516 +/- ##
===========================================
- Coverage 91.27% 91.26% -0.01%
===========================================
Files 62 62
Lines 13358 13367 +9
===========================================
+ Hits 12193 12200 +7
- Misses 1165 1167 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
lI think it should be even simpler. Since log_performance()
is already called in sanity failures for performance tests, what if we simply rename it to log_result()
and remove the check is_performance_check()
from the log_performance()
?
I think this should pretty much get us to what we want.
But could it be an issue for users if we add more entries in their logging, without giving the option to disable? For CSCS it's of course fine, but I added the option so that we keep compatibility. |
I don't it's such a big issue as it is not breaking anything. Besides, the perflogs are per test, so there will not be new entries in existing files. So I'd rather go with this change than adding yet another configuration parameter. |
Ok! will make the change :) |
Fixes #3460 .