Skip to content

[feat] Log also results from sanity-only tests through the perflog handlers#3516

Merged
vkarak merged 10 commits intoreframe-hpc:developfrom
ekouts:feat/sanity_logging
Oct 28, 2025
Merged

[feat] Log also results from sanity-only tests through the perflog handlers#3516
vkarak merged 10 commits intoreframe-hpc:developfrom
ekouts:feat/sanity_logging

Conversation

@ekouts
Copy link
Contributor

@ekouts ekouts commented Jul 14, 2025

Fixes #3460 .

@codecov
Copy link

codecov bot commented Jul 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.25%. Comparing base (6faf9c7) to head (95f26be).
⚠️ Report is 11 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3516   +/-   ##
========================================
  Coverage    91.25%   91.25%           
========================================
  Files           62       62           
  Lines        13534    13534           
========================================
  Hits         12350    12350           
  Misses        1184     1184           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vkarak vkarak changed the title Add option to log sanity results in perfloggers [feat] Add option to log sanity results in perfloggers Jul 17, 2025
Copy link
Contributor

@vkarak vkarak left a 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.

@github-project-automation github-project-automation bot moved this from Todo to In Progress in ReFrame Backlog Jul 18, 2025
@ekouts
Copy link
Contributor Author

ekouts commented Jul 18, 2025

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.

@vkarak
Copy link
Contributor

vkarak commented Jul 18, 2025

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.

@ekouts
Copy link
Contributor Author

ekouts commented Jul 18, 2025

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 :)

@ekouts ekouts changed the title [feat] Add option to log sanity results in perfloggers [feat] Log all tests in perfloggers, not only performance tests Jul 18, 2025
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, just a couple of minor comments.

@vkarak vkarak force-pushed the feat/sanity_logging branch from 77021c9 to 95f26be Compare October 28, 2025 18:28
@vkarak vkarak changed the title [feat] Log all tests in perfloggers, not only performance tests [feat] Log also results from sanity-only tests through the perflog handlers Oct 28, 2025
@vkarak vkarak enabled auto-merge October 28, 2025 18:30
@vkarak vkarak merged commit 36afc5c into reframe-hpc:develop Oct 28, 2025
62 of 71 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in ReFrame Backlog Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Log results of tests that are not performance tests

2 participants