[N-Dekker]

Various tests had their own println(const char *) implementations, which appear very similar to std::puts. (std::puts also adds a newline character at the end.)

[hjmjohnson]

Nice improvement!

[N-Dekker]

Actually we had four different (but very similar) implementations of println(const char *)! This one here was properly placed in an unnamed namespace. But the one in itkConstNeighborhoodIteratorTest.cxx was entirely global, and had external linkage. The one in itkNeighborhoodTest.cxx was declared static, while the one in itkHashTableGTest.cxx was declared inline!

[N-Dekker]

Honestly I would rather not have such "unconditional" print statements in unit tests at all. (Not even by std::puts.) It produces distractive noise in the test output, IMO. So I would prefer to only see such informative output in case of a test failure. The GoogleTest macro SCOPED_TRACE(message) does so: it only produces output in case of a failure. Anyway, I think replacing print statements with SCOPED_TRACE calls is beyond the scope of this PR... 🤷

[dzenanz]

A nice follow-up PR?

[N-Dekker]

Could be 😃 At least it's good to consider using the GoogleTest SCOPED_TRACE(message) macro, whenever it is useful 😃

[hjmjohnson]

I think that SCOPED_TRACE should be used a lot more in GTests.

[N-Dekker]

Please note that this particular println implementation inserts two line breaks (one before and one after the message string), whereas the "global" println (implemented in itkConstNeighborhoodIteratorTest) only inserts a single line break, at the end:

ITK/Modules/Core/Common/test/itkConstNeighborhoodIteratorTest.cxx

Lines 24 to 27 in fc63666

	println(const char * s)
	{
	std::cout << s << std::endl;
	}

I think this is an extra reason to remove them. It's too confusing to have multiple println implementations behaving differently.

However, this PR will effectively remove some line breaks from the test output. I hope that's OK!

Looking at https://open.cdash.org/tests/2833926099

[ RUN      ] BoundaryCondition.ConstantBoundaryAtImageEdge

Creating some images

Initializing some images

Initializing smart neighborhood iterators
1327 1328 1329 0 0 

This PR will remove the empty lines. and make it as in https://open.cdash.org/tests/2832303956

[ RUN      ] BoundaryCondition.ConstantBoundaryAtImageEdge
Creating some images
Initializing some images
Initializing smart neighborhood iterators
1327 1328 1329 0 0 

OK? To me it's fine.

[greptile-apps]

Confidence Score: 4/5

[Low impact] The changes affect printed output from a bounded set of Core/Common tests.

The PR appears safe to merge, with a non-blocking issue that splits two tests' diagnostic output.

Findings

1.  Saved logs lose headings ▶

Summary

Replaces custom println helpers with std::puts across eight Core/Common test files and removes the shared declaration.

• Core/Common tests print progress messages with std::puts.

_{Reviews (1) · Last reviewed commit: "STYLE: Replace `println` functions in Co..." · Reviewed by Greptile}

[greptile-apps]

Saved logs lose headings

std::puts bypasses --redirectOutput, which both itkConstNeighborhoodIteratorTest and itkShapedNeighborhoodIteratorTest use. The driver redirects only std::cout.rdbuf(), so progress headings now go to stdout while iterator details go to the attached log file. This makes failure logs harder to follow. Keep these messages on std::cout, or update the driver to capture both output paths.

[N-Dekker]

OK, so it may not be such a good idea to use std::puts in this case. Addressed by:

• pull request Remove println functions from "Core/Common/test", use std::cout directly #7071

[hjmjohnson]

That's a good catch by greptile. I did not realize the redirect is not supported.

[hjmjohnson]

Given that redirect is not supported by std::puts, I think this should be superceeded by the other PR.

I also like your suggestion of only printing content when a failure occurs. I think inside gTest that the diagnostics should be minimal unless an error occurs.

[N-Dekker]

I also like your suggestion of only printing content when a failure occurs. I think inside gTest that the diagnostics should be minimal unless an error occurs.

@hjmjohnson Thanks Hans! I just encountered an extra motivation to minimize output on a successful pass, as https://open.cdash.org/tests/2835201106 says:

[This part of the test output was removed since it exceeds the threshold of 1024 bytes.]

I just wanted to see the output beyond the 1024 bytes boundary, to see the effect of removing println, but it wasn't shown, unfortunately!

• Anyway, I'll close this PR in favor of Remove println functions from "Core/Common/test", use std::cout directly #7071