Skip to content

Convert FORM tests to use Cetmodules#168

Merged
greenc-FNAL merged 3 commits intoFramework-R-D:mainfrom
greenc-FNAL:maintenance/installation-verification
Dec 8, 2025
Merged

Convert FORM tests to use Cetmodules#168
greenc-FNAL merged 3 commits intoFramework-R-D:mainfrom
greenc-FNAL:maintenance/installation-verification

Conversation

@greenc-FNAL
Copy link
Contributor

  • Convert test/form tests to use cet_test().

  • Update writer.cpp and reader.cpp to accept filename argument.

  • Pass shared data file path to tests to support parallel execution.

Run cmake-format (FORM)

Copilot AI review requested due to automatic review settings December 5, 2025 23:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR converts the FORM test suite from manually configured executables to use the Cetmodules testing framework (cet_test()). The changes enable parallel test execution by accepting a configurable output file path as a command-line argument, and properly establishes test dependencies to ensure the writer test completes before the reader test runs.

  • Refactored writer.cpp and reader.cpp to accept filename as a command-line argument
  • Converted CMakeLists.txt to use cet_test() instead of add_executable() and add_test()
  • Configured tests to use CMAKE_CURRENT_BINARY_DIR for output files to support parallel execution

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
test/form/writer.cpp Modified main() to accept and use filename argument instead of hardcoded "toy.root"
test/form/reader.cpp Modified main() to accept and use filename argument instead of hardcoded "toy.root"
test/form/CMakeLists.txt Converted test configuration from manual add_executable/add_test to cet_test() framework with proper dependencies and arguments

@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
- Coverage   81.42%   81.13%   -0.30%     
==========================================
  Files         115      115              
  Lines        2046     2046              
  Branches      330      330              
==========================================
- Hits         1666     1660       -6     
- Misses        252      255       +3     
- Partials      128      131       +3     
Flag Coverage Δ
unittests 81.13% <ø> (-0.30%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b6b96c...5c73323. Read the comment docs.

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

aolivier23
aolivier23 previously approved these changes Dec 5, 2025
Copy link
Contributor

@aolivier23 aolivier23 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. Thank you for doing this! I'm trusting that cet_test() does roughly what the old CMake code did as I'm not familiar with how the cet macros work.

gemmeren
gemmeren previously approved these changes Dec 6, 2025
Copy link
Contributor

@gemmeren gemmeren left a comment

Choose a reason for hiding this comment

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

Thanks Chris, looks good.

greenc-FNAL and others added 3 commits December 8, 2025 11:56
- Convert test/form tests to use cet_test().

- Update writer.cpp and reader.cpp to accept filename argument.

- Pass shared data file path to tests to support parallel execution.

Run `cmake-format` (FORM)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@greenc-FNAL greenc-FNAL dismissed stale reviews from gemmeren and aolivier23 via 13b9738 December 8, 2025 18:01
@greenc-FNAL greenc-FNAL force-pushed the maintenance/installation-verification branch from 1d9f5fe to 13b9738 Compare December 8, 2025 18:01
@greenc-FNAL
Copy link
Contributor Author

@aolivier23 @gemmeren I have rebased against main and addressed the remaining (not auto-resolved) comment from Copilot; I'd appreciate a re-review when you have a moment.

@aolivier23 Yes, cet_test() is more versatile than raw add_test(), but the new calls are equivalent to the old ones.

aolivier23
aolivier23 previously approved these changes Dec 8, 2025
Copy link
Contributor

@aolivier23 aolivier23 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for confirming about what cet_test() does and great work!

gemmeren
gemmeren previously approved these changes Dec 8, 2025
Copy link
Contributor

@gemmeren gemmeren left a comment

Choose a reason for hiding this comment

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

Thanks Chris, looks good.

Copy link
Member

@knoepfel knoepfel left a comment

Choose a reason for hiding this comment

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

@greenc-FNAL, can the .devcontainer changes be backed out?

@greenc-FNAL
Copy link
Contributor Author

@greenc-FNAL, can the .devcontainer changes be backed out?

Absolutely (sorry), but I'll need one more approving review.

@greenc-FNAL greenc-FNAL dismissed stale reviews from gemmeren and aolivier23 via 5c73323 December 8, 2025 23:13
@greenc-FNAL greenc-FNAL force-pushed the maintenance/installation-verification branch from 13b9738 to 5c73323 Compare December 8, 2025 23:13
@greenc-FNAL greenc-FNAL merged commit 32e8a97 into Framework-R-D:main Dec 8, 2025
63 checks passed
@greenc-FNAL greenc-FNAL deleted the maintenance/installation-verification branch December 9, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants