InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

Convert no-argument tests to GTests

Open
#5,919 1 comment 1 reaction 0 assignees View on GitHub
type:Infrastructure
Dominant language
C++
Stars
1.7k
Forks
748
Avg merge
1d 1h
Merged PRs (30d)
64

Description

---
description: Convert no-argument ITK CTests in a directory to GoogleTest format, one test per commit.
allowed-tools: Bash, Read, Write, Edit, Glob, Grep
---

# Convert ITK CTests to GoogleTest

Convert all no-argument CTests in the specified directory to GoogleTest format.
Process **one test at a time**, creating a separate git commit for each conversion.

## Target Directory

$ARGUMENTS

If no file is given, then all files in current working directory's `test/` subdirectory will be considered.

## Core Philosophy: Mechanical Conversion Only

> "One Commit One Change" — N-Dekker, ITK reviewer

This conversion must be **strictly mechanical**. The GTest file should behave
identically to the old test file. Do **not**:
- Remove existing comments (especially scope-explaining comments)
- Add `[[maybe_unused]]` unless a variable is genuinely never read
- Refactor, simplify, or restructure logic beyond what conversion requires
- Add diagnostic output that was not in the original

The reviewer standard: "Please just do Test-to-GoogleTest conversion."

## Step 1: Discover No-Argument Tests

Read `CMakeLists.txt` in the target directory. Find tests that:
- Have their `.cxx` file listed in a `set(*Tests ...)` block
- Have a corresponding `itk_add_test(NAME ... COMMAND ...TestDriver testFunctionName)` block with **no additional arguments** after the test function name

A no-argument test looks like this in CMakeLists.txt:
```cmake
itk_add_test(
NAME itkFooTest
COMMAND
ITKThisModule1TestDriver
itkFooTest
)
```

Skip tests that pass arguments (files, numbers, paths) after the driver function name.
Skip tests whose `*GTest.cxx` file already exists.

Run this Python snippet to find candidates:
```bash
python3 - <<'EOF'
import re, sys
cmake = open('CMakeLists.txt').read()
# Find itk_add_test blocks with no arguments after the function name
pattern = r'itk_add_test\(\s*NAME\s+(\w+)\s*\n\s*COMMAND\s*\n\s*\S+\s*\n\s*(\w+)\s*\)'
for m in re.finditer(pattern, cmake):
name, fn = m.group(1), m.group(2)
if name == fn:
import os
gtest = fn.replace('Test', 'GTest') + '.cxx'
old = fn + '.cxx'
if not os.path.exists(gtest) and os.path.exists(old):
print(f"CANDIDATE: {fn}")
EOF
```

## Step 2: For Each Candidate — Convert One test file at a Time

Work through candidates **one at a time**. For each `itkFooTest`:

### 2a. Read the Old Test File

Read `itkFooTest.cxx` carefully. Understand **every** check, output statement,
and comment — all of them must be preserved in the new file.

### 2b. Create `itkFooGTest.cxx`

Create the new GTest file. Follow these conventions:
- `git mv itkFooTest.cxx itkFooGTest.cxx`
- Include the primary header being tested first (if identifiable)
- Use `#include "itkGTest.h"` (not ``)
- Use `ITK_GTEST_EXERCISE_BASIC_OBJECT_METHODS(ptr, ClassName, SuperclassName)` for ITK object boilerplate (requires a named variable, not an expression) in places where `ITK_EXERCISE_BASIC_OBJECT_METHODS` was previously used
- Wrap helper functions in an anonymous `namespace { }`
- Preserve all `std::cout` diagnostic output from the original **only** if they contain functions that would otherwise not be called. Otherwise attempt to minimize the redundant console outputs on passing tests.
- Preserve all comments, especially scope-explaining comments
- For legacy API tests: wrap in `#ifndef ITK_FUTURE_LEGACY_REMOVE` / `#endif`

#### Test Name Convention

Use `TEST(ClassName, ConvertedLegacyTest)` as the standard name for a converted
test that has no finer logical subdivision:

```cpp
TEST(Foo, ConvertedLegacyTest)
{
// converted body
}
```

If the original test has multiple clearly distinct logical sections, split them
into separate `TEST()` blocks with descriptive names — but only if those sections
were already distinct in the original.

#### Assertion Mapping

Translate the original's output/return-code checks into GTest assertions using
the **most specific** macro available:

| Original pattern | GTest equivalent |
|---|---|
| `if (!condition) return EXIT_FAILURE` | `EXPECT_TRUE(condition)` |
| `if (a != b) return EXIT_FAILURE` | `EXPECT_EQ(a, b)` |
| `if (a == b) return EXIT_FAILURE` | `EXPECT_NE(a, b)` |
| `if (a <= b) return EXIT_FAILURE` | `EXPECT_GT(a, b)` |
| `if (a >= b) return EXIT_FAILURE` | `EXPECT_LT(a, b)` |
| `if (ptr == nullptr) return EXIT_FAILURE` | `EXPECT_NE(ptr, nullptr)` |
| `if (ptr != nullptr) return EXIT_FAILURE` | `EXPECT_EQ(ptr, nullptr)` |
| try/catch expecting exception | `EXPECT_THROW(expr, ExceptionType)` or `ASSERT_THROW` |
| comparing ITK array-like objects | `ITK_EXPECT_VECTOR_NEAR(val1, val2, rmsError)` |

**Never use** `EXPECT_TRUE(a > b)` when `EXPECT_GT(a, b)` expresses the same
thing. **Never use** `EXPECT_TRUE(ptr == nullptr)` — use `EXPECT_EQ(ptr, nullptr)`.

Only add `EXPECT_*` assertions that correspond to a real check in the original
test (explicit `if (...) return EXIT_FAILURE`, or a function whose documented
contract is to throw on failure). Do not add assertions that were not present.

Template structure:
```cpp
/*=========================================================================
*
* Copyright NumFOCUS
*
* Licensed under the Apache License, Version 2.0 (the "License");
* ...
*=========================================================================*/

// First include the header file to be tested:
#include "itkFoo.h"
#include "itkGTest.h"

namespace
{
// helper functions here
} // namespace

TEST(Foo, ConvertedLegacyTest)
{
// test body with EXPECT_* assertions mirroring the original checks
}
```

### 2c. Update `CMakeLists.txt`

Make three edits:

**Remove** the `.cxx` filename from the `ITKThisModule1Tests` (or `ITKThisModule2Tests`) `set(...)` block:
```cmake
# Remove this line:
itkFooTest.cxx
```

**Remove** the entire `itk_add_test(...)` block:
```cmake
# Remove this block:
itk_add_test(
NAME itkFooTest
COMMAND
ITKThisModule1TestDriver
itkFooTest
)
```

**Add** the new GTest filename to the `ITKThisModuleGTests` `set(...)` block (before the closing `)`):
```cmake
itkFooGTest.cxx
) # <-- closing paren
```

The `ITKThisModuleGTests` set feeds `creategoogletestdriver(ITKThisModuleGTests ...)` which builds `ITKThisModuleGTestDriver`.

### 2d. Delete the Old File

```bash
git rm Modules/Core/Common/test/itkFooTest.cxx
```

### 2e. Commit

Stage and commit with message format:
```
ENH: Convert itkFooTest to itkFooGTest
```

Requirements enforced by hooks:
- Subject line must start with `ENH: Convert`
- Subject line must be ≤ 78 characters
- Always run pre-commit run -a on the entire source tree before staging changes and committing
- The clang-format pre-commit hook **will reformat** staged C++ and CMakeLists.txt files; if the first commit attempt fails, re-stage the reformatted files and commit again

```bash
git add Modules/Core/Common/test/itkFooGTest.cxx \
Modules/Core/Common/test/CMakeLists.txt
git commit -m "ENH: Convert itkFooTest to itkFooGTest"
# If hook reformats files, re-stage and recommit:
# git add Modules/Core/Common/test/itkFooGTest.cxx
# git commit -m "ENH: Convert itkFooTest to itkFooGTest"
```

After a successful commit, move to the next candidate.

## Step 3: Augmenting Existing GTest Files

If a `*GTest.cxx` already exists (e.g., `itkArrayGTest.cxx`), **append** new `TEST()` blocks rather than creating a new file. Read the existing file first to avoid duplicating coverage. Then follow steps 2c–2e treating it as a new conversion.

## Common Pitfalls

- **`ITK_GTEST_EXERCISE_BASIC_OBJECT_METHODS`** requires a named pointer variable in scope — not an inline `FooType::New()` expression
- **Double braces** for aggregate-initialized ITK structs: `itk::Size<3> sz{ { 10, 10, 10 } };`
- **`[[maybe_unused]]`** — only use when the variable is genuinely never read; do NOT add it to a variable used on the very next line
- **`std::hash`** behavior is implementation-defined — never assert `hash(x) == x`
- **Platform-specific assertions**: avoid assuming `std::hash` is identity; avoid assuming specific numeric output values that differ across platforms
- **Do not** add a `main()` function — GTest provides its own
- **Do not** use `EXIT_SUCCESS`/`EXIT_FAILURE` returns — use `EXPECT_*`/`ASSERT_*`
- **Do not** combine multiple ctest files into a single GTest.cxx file
- **Do not** remove scope-explaining comments (e.g., `// local scope to ensure ...`)
- The old test driver called `itkFooTest(int argc, char* argv[])` as a function — the new file is standalone and should not define that signature

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.