cmake: tests that are not .cpp standalones

[Lestropie]

@daljit46 I'll need your input on this one.

I believe that this was a failure on my part to fully resolve #2437 against #2689.

Whatever is the solution here, will also be applicable to #2678, where I'm trying to add a test of the Python CLI changes.

Relates to #2834, but is sufficiently different to justify a separate thread.

---

Previously, the "unit tests", as distinct from the "binaries" (ie. C++) and "scripts" (ie. Python) tests, were all just .cpp files, that could be compiled by invoking the build script in the testing/ directory. A set of text files then defined the tests, each of which literally just invoked the compiled binary with no arguments.

I think in #2437 (and now coming in #2678), I was exploiting this design to achieve testing in a different way, and now with the move to cmake perhaps it no longer works.

There, I added a more complex test structure:

• testing/bin/testing_*_npy are written in Python (though do not use the MRtrix3 Python API), and are used to generate and check data for evaluating the C++ NPY support;
• testing/tools/testing_unit_tests_npy*.cpp result in compilation of binaries that perform functionalities to be evaluated by the Python commands above;
• testing/tests/npy executes the C++ and Python functions above in sequence as necessary to evaluate the proper function of the C++ NPY support.

This is however being overlooked by the new cmake test suite construction, which I am naive to.

On naive inspection, I think what's happening now on dev is that:

• The set of unit tests is defined explicitly in testing/unit_tests/CMakeLists.txt
• For each test name:
  • The .cpp file with that name is compiled;
  • A test with that name that executes the compiled binary is constructed.

Trouble here is that my NPY tests don't conform to this structure.

Files testing/tools/testing_unit_tests_npy*.cpp are I think in the correct location, though "unit_tests_" should probably be stripped from their names. The problem is that I want to be able to define a unit test (ie. it's test of neither a specific binary nor a Python command), where the commands to be invoked to complete that test is more complex than just the name of a compiled binary. That will I think require more complex cmake directives than what currently exists on dev.

I'm hoping that if dev is modified such that the NPY tests are performed as intended, then #2678 will re-use that structure.

[daljit46]

To define a command line test (i.e. a chain of bash commands that return 0 on success), we use the add_bash_tests function defined in cmake/BashTests.cmake (see the scripts/CMakeLists.txt as an example). If I understand correctly, this is what will be required to execute an NPY "unit test" (BTW I find this terminology very confusing in the context of our codebase).
We would also need to make sure that these tests are able to execute the python files defined in bin (or wherever they belong).
One thing I don't quite though is why aren't the NPY tests under scripts?

Generally speaking, I think we should change and simplify the structure of the testing directory. I find it to be, even before our switch to CMake, confusing. Here are some thoughts on this:

• We should separate cpp and python tests in their own subdirectories. A good reason to do so is that the latter depend on the former but not vice versa. Not sure whether this will lead to some duplication of folder structure and CMake logic and whether that would be worth it.
• Consider moving lib under tools.
• Reconsider the name binaries.
• Reconsider unit_test in naming of tests and directories.
• Reconsider working directory and handling of data for tests (to be discussed in Connectome tool #283)
• Document the purpose of each test (or at least the purpose of the "category" they belong to)

[Lestropie]

NPY "unit test" (BTW I find this terminology very confusing in the context of our codebase).

For us it was probably a case of "unit test" == "smaller in scale than a per-command-input-to-output regression test". If there's more suitable terminology, happy to change.

One thing I don't quite though is why aren't the NPY tests under scripts?

Because the addition of NPY support was in the C++ code. "scripts" in the context of the testing sub-directory is in reference to "the Python executable script commands" as opposed to "the C++ compiled binary commands". So it's not "scripts that are used for testing", but "content for testing of scripts". As per #2824, changing to cpp/ and python/ may be preferable, particularly if reflected in both the source code and the testing subdirectories.

We should separate cpp and python tests in their own subdirectories.

In the repository this is already the case. Is this maybe supposed to be in reference to the way that cmake then arranges them?

A good reason to do so is that the latter depend on the former but not vice versa.

Certainly the Python executables depend on the C++ executables; the Python tests don't depend on the C++ tests. But if testing can only be done following a cmake build, I'm not sure if this dependency is all that crucial? Previously we sort of exploited this within run_tests to be able to quickly run all tests of only C++ binaries vs. run all tests of only Python scripts (vs. run all unit tests only). Maybe there's a more cmake-like way of doing the same thing?

Document the purpose of each test

Where I eventually landed on #2789 was to take each file containing multiple tests, and turn each line into an appropriately named bash script in a sub-directory belonging to that command. This would give more sensible test names when run in ctest, and would allow inclusion of comments in each file.

[daljit46]

Because the addition of NPY support was in the C++ code. "scripts" in the context of the testing sub-directory is in reference to "the Python executable script commands" as opposed to "the C++ compiled binary commands". So it's not "scripts that are used for testing", but "content for testing of scripts". As per #2824, changing to cpp/ and python/ may be preferable, particularly if reflected in both the source code and the testing subdirectories.

Ok so would the more appropriate place for files in bin be tools? Since these files are meant to be executed by the actual "tests".

the Python tests don't depend on the C++ tests

Yeah sorry, I'm using "tests" wrongly here. What I was referring to is to tools (e.g. under tools) that are needed to run Python commands. Presumably, a Python test may need those (although I'm not sure if anyone in particular does atm), but a C++ wouldn't need a "Python tool".

[Lestropie]

C++ wouldn't need a "Python tool".

Assuming you meant "A C++ test wouldn't need a Python tool", yes, that's precisely what the NPY tests do need. I'm generating NPY data using Python and then reading it in a C++ command, and vice versa.

Part of where I'm stumbling I think is that the port to cmake has made a certain assumption about the structure of the unit tests, but this new use case violates that assumption, and so it's going to take modification on the cmake side of things.
Take for example the unit test for erfinv. Pre cmake, there were two files: testing/tests/erfinv and testing/cmd/testing_unit_tests_erfinv.cpp. Script run_tests would potentially identify the former as one of the tests to run (depending on command-line arguments passed to run_tests); run_tests would also run build from inside the testing/ directory, which would compile the latter into testing/bin/testing_unit_tests_erfinv.
What it looks to me has happened is that you've assumed that all unit tests follow this design. As such, function add_unit_test within testing/unit_tests/CMakeLists.txt combines these two steps into one, both registering the .cpp file as a compilation target and registering a test with the same name. The NPY tests don't obey this structure. Further, there are other existing or hypothetical future tests that should do the same. Eg. Evaluation of PNG support is currently done as part of the mrconvert tests, but realistically this should be a "unit test" (or, at least, not ascribed specifically to mrconvert, at least in the way that PNG is currently handled). Same could be said for something like the multi-numbered-file support.

So what I think we need is:

• Move testing/bin/testing_check_npy and testing/bin/testing_gen_npy to testing/tools
• Move testing/tests/npy to testing/unit_tests/
• Change content of testing/unit_tests/CMakeLists.txt:
  • For .cpp files, do as is currently done: compile .cpp into executable and register the test
  • For other files, just register the test
• Change contents of testing/tools/CMakeLists.txt:
  • For .cpp files, do as is currently done: Compile the tool
  • For non-.cpp files, just put the file wherever it needs to go in the build directory

[daljit46]

Ok, yes that makes sense to me.

[Lestropie]

Closed by #2842.