Skip to content

ENH: Convert 4 Common CTests to GoogleTest#6071

Merged
hjmjohnson merged 5 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:convert-common-gtests
Apr 16, 2026
Merged

ENH: Convert 4 Common CTests to GoogleTest#6071
hjmjohnson merged 5 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:convert-common-gtests

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Mechanical conversion of 4 no-argument CTests in Modules/Core/Common/test/ to GoogleTest format: CompensatedSummation, PriorityQueue, RealTimeInterval, RealTimeStamp.

Conversion details
Old Test New GTest TEST() blocks
itkCompensatedSummationTest itkCompensatedSummationGTest 1 (ConvertedLegacyTest)
itkPriorityQueueTest itkPriorityQueueGTest 1 (ConvertedLegacyTest)
itkRealTimeIntervalTest itkRealTimeIntervalGTest 5 (DefaultConstructor, AccumulationAndSubtraction, SetWithNormalization, Addition, ComparisonOperators)
itkRealTimeStampTest itkRealTimeStampGTest 6 (DefaultConstructor, NegativeIntervalThrows, AccumulationAndSubtraction, IntervalSetWithNormalization, IntervalAddition, ComparisonOperators)
  • Every return EXIT_FAILURE path mapped to a corresponding EXPECT_* assertion
  • CHECK_FOR_VALUE / CHECK_FOR_BOOLEAN macros replaced with EXPECT_LE / EXPECT_TRUE / EXPECT_FALSE
  • ITK_TRY_EXPECT_EXCEPTION replaced with EXPECT_THROW(..., itk::ExceptionObject)
  • Rename commits preserve git log --follow history tracking
Local verification
  • Build: ITKCommonGTestDriver links successfully (1476 targets)
  • Tests: 14/14 passed (1 CompensatedSummation + 1 PriorityQueue + 5 RealTimeInterval + 6 RealTimeStamp + 1 CompensatedSummationTest2 unchanged)
  • Pre-commit: clang-format, gersemi, all hooks passed

@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Apr 15, 2026
Copy link
Copy Markdown
Member

@dzenanz dzenanz 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 on a glance.

@hjmjohnson hjmjohnson marked this pull request as ready for review April 15, 2026 19:35
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 15, 2026

Greptile Summary

Mechanical conversion of four Modules/Core/Common CTests (CompensatedSummation, PriorityQueue, RealTimeInterval, RealTimeStamp) from legacy int main() style to GoogleTest, with corresponding CMakeLists.txt plumbing updates. All original assertion paths are faithfully preserved: CHECK_FOR_VALUE/CHECK_FOR_BOOLEAN macros become EXPECT_LE/EXPECT_TRUE/EXPECT_FALSE, and ITK_TRY_EXPECT_EXCEPTION becomes EXPECT_THROW(..., itk::ExceptionObject).

Confidence Score: 5/5

Safe to merge — all original test coverage is preserved, logic conversions are correct, and no functional regressions were identified.

All remaining findings are P2 style suggestions (duplicated helper, test-suite name mismatch). No correctness, data-integrity, or reliability issues were found. The assertion mappings are faithful and the CMakeLists.txt plumbing is consistent with the existing GTest driver pattern.

itkRealTimeStampGTest.cxx — ComparisonOperators test exercises RealTimeInterval objects under a RealTimeStamp suite name (inherited issue, not blocking).

Important Files Changed

Filename Overview
Modules/Core/Common/test/CMakeLists.txt Removes four legacy CTest source files and their itk_add_test entries; adds four new GTest sources to the GTest driver target. Plumbing is consistent with the rest of the file.
Modules/Core/Common/test/itkCompensatedSummationGTest.cxx Faithful rename + conversion: EXIT_FAILURE guards replaced by EXPECT_* macros; original two-condition check (`vanillaError <= accumulatorError
Modules/Core/Common/test/itkPriorityQueueGTest.cxx Faithful rename + conversion; min-queue and max-queue loop invariants preserved; sequence is not mutated by the min-queue pass so the max-queue sequence.back() / sequence.size() checks remain valid.
Modules/Core/Common/test/itkRealTimeIntervalGTest.cxx New GTest file with 5 well-scoped test cases; all original CHECK_FOR_VALUE / CHECK_FOR_BOOLEAN paths accounted for; minor: identical CheckForValue helper also lives in itkRealTimeStampGTest.cxx.
Modules/Core/Common/test/itkRealTimeStampGTest.cxx New GTest file with 6 test cases; both ITK_TRY_EXPECT_EXCEPTION sites mapped correctly; ComparisonOperators test is labelled RealTimeStamp but exercises only RealTimeInterval objects (inherited issue, noted as P2).
Modules/Core/Common/test/itkRealTimeIntervalTest.cxx Deleted — content migrated to itkRealTimeIntervalGTest.cxx; no residual references remain.
Modules/Core/Common/test/itkRealTimeStampTest.cxx Deleted — content migrated to itkRealTimeStampGTest.cxx; no residual references remain.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph Before["Before (legacy CTest)"]
        A1[itkCompensatedSummationTest.cxx] --> D1[ITKCommon2TestDriver]
        A2[itkPriorityQueueTest.cxx] --> D2[ITKCommon1TestDriver]
        A3[itkRealTimeIntervalTest.cxx] --> D2
        A4[itkRealTimeStampTest.cxx] --> D2
    end

    subgraph After["After (GoogleTest)"]
        B1[itkCompensatedSummationGTest.cxx] --> G[ITKCommonGTestDriver]
        B2[itkPriorityQueueGTest.cxx] --> G
        B3[itkRealTimeIntervalGTest.cxx] --> G
        B4[itkRealTimeStampGTest.cxx] --> G
    end

    G --> T1[RealTimeInterval: 5 TEST blocks]
    G --> T2[RealTimeStamp: 6 TEST blocks]
    G --> T3[CompensatedSummation: 1 TEST block]
    G --> T4[PriorityQueue: 1 TEST block]
Loading

Comments Outside Diff (2)

  1. Modules/Core/Common/test/itkRealTimeStampGTest.cxx, line 731-766 (link)

    P2 Test suite name vs. tested type mismatch

    TEST(RealTimeStamp, ComparisonOperators) only constructs and compares itk::RealTimeInterval objects (t1, t2, t3), so the GoogleTest output will label this as a RealTimeStamp test even though it never exercises any RealTimeStamp comparison operators. This is inherited from the original monolithic test, but the mechanical split into named GTest blocks is a good opportunity to either rename this to TEST(RealTimeInterval, ComparisonOperators) or replace t1/t2/t3 with actual itk::RealTimeStamp objects to match the suite name.

  2. Modules/Core/Common/test/itkRealTimeIntervalGTest.cxx, line 304-313 (link)

    P2 Duplicated CheckForValue helper

    The same CheckForValue function (including the ITK_GCC_PRAGMA_PUSH/ITK_GCC_SUPPRESS_Wfloat_equal block) is copy-pasted verbatim into both itkRealTimeIntervalGTest.cxx and itkRealTimeStampGTest.cxx. If the tolerance logic ever needs to change, both files must be updated in sync. Extracting this into a small shared test-utility header (e.g. itkGTestUtilities.h) would keep it DRY. This is a minor concern since anonymous-namespace placement prevents ODR issues, but worth noting for future maintainability.

Reviews (1): Last reviewed commit: "ENH: Convert itkRealTimeStampTest to GTe..." | Re-trigger Greptile

@hjmjohnson
Copy link
Copy Markdown
Member Author

The ARM build CI was stalled for 18 hours yesterday, and still is not working today. This GTest has already been tested on ARM in other builds.

@hjmjohnson hjmjohnson merged commit 0060d7b into InsightSoftwareConsortium:main Apr 16, 2026
20 of 25 checks passed
@hjmjohnson hjmjohnson added this to the ITK 6.0.0 milestone May 5, 2026
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
…vert-common-gtests

ENH: Convert 4 Common CTests to GoogleTest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants