catch_discover_test PROPERTIES does not work with lists

[mxmlnkn]

Describe the bug

When still using ParseAndAddCatchTests, I had code like this to set multiple environment variables for running the tests:

list(APPEND _environment_vars foo=bar)
list(APPEND _environment_vars f00=b4r)
ParseAndAddCatchTests(${test})
get_target_property(sub_tests ${test} ParseAndAddCatchTests_TESTS)
set_tests_properties(${sub_tests} PROPERTIES ENVIRONMENT "${_environment_vars}")

When using catch_discover_tests, I tried to replace the last three lines with:

catch_discover_tests(${test} PROPERTIES ENVIRONMENT "${_environment_vars_list}")

But after observing some indirect errors a while later, I noticed that only the very first environment variable was actually exported.

Looking under the hood, the problem seems to be that PROPERTIES are stored as a list itself, so that lists as value / semicolons in values will lead to problems.

My workaround currently is to add ENVIRONMENT; for each environment variable, resulting in:

string(REPLACE ";" ";ENVIRONMENT;" _environment_vars_list "${_environment_vars}")
catch_discover_tests(${test} PROPERTIES ENVIRONMENT "${_environment_vars_list}")

Expected behavior

catch_discover_tests(${test} PROPERTIES ENVIRONMENT "${_environment_vars_list}") should work even if there are multiple environment variables specified. This interface would be consistent with set_tests_properties.

Alternatively, this differing behavior for PROPERTIES should be documented, e.g., inside docs/cmake-integration.md, if possible with a suggested workaround.

Platform information:

• Catch version: v2.13.9

[ferdnyc]

@mxmlnkn

Looking under the hood, the problem seems to be that PROPERTIES are stored as a list itself, so that lists as value / semicolons in values will lead to problems.

Yeah, the bigger "problem" (meaning, barrier to using an environment list the way you'd like, which I agree is a reasonable expectation) is that catch_discover_tests just generates a big add_custom_command() call, as a post-build target attached to the testrunner, which re-invokes ${CMAKE_COMMAND} to execute a different Catch script with a bunch of variables defined. One of them is TEST_PROPERTIES, which is set to the list of arguments you provide to PROPERTIES in the call to catch_discover_tests():

https://github.com/catchorg/Catch2/blob/165647abbcbf27dcdae074bdb9d5fe2eaffceb67/extras/Catch.cmake#L149-L159

Then, through a whole series of events, that argument gets transformed into a set_tests_properties() call for the test in question. It actually doesn't even process the arguments as pairs, it just splits out every argument and tacks it on to the end of the call. That, then, looks something like this:

set_tests_properties( [==[Test name]==] PROPERTIES WORKING_DIRECTORY /path/to/it 
LABELS Labels ENVIRONMENT [==[VAR=value]==])

Each separate PROPERTIES arg gets wrapped as a bracket argument if it contains any special chars. (As all of the ENVIRONMENT args always will, even when not trying to pass a list, since = is a special char.) So if you pass a semicolon-separated list to ENVIRONMENT, it invariably gets exploded along the way and you end up with something like,

set_tests_properties( [==[Test name]==] PROPERTIES WORKING_DIRECTORY /path/to/it 
LABELS Labels ENVIRONMENT [==[VAR1=val1]==] [==[VAR2=val2]==] [==[VAR3=val3]==])

I'm actually a bit surprised to learn (as you claim) that a call (effectively) like this, works:

catch_discover_tests(...
  PROPERTIES
    ENVIRONMENT VAR1=val1
    ENVIRONMENT VAR2=val2
)

Since that's going to end up transformed into this:

set_tests_properties( [==[Test name]==] PROPERTIES WORKING_DIRECTORY /path/to/it 
LABELS Labels ENVIRONMENT [==[VAR1=val1]==] ENVIRONMENT [==[VAR2=val2]==] 
ENVIRONMENT [==[VAR3=val3]==])

...and, while ENVIRONMENT is documented (by CMake) as taking a list of properties, it's not documented as auto-list-concatenating multiple values set on it separately, and Catch2 isn't using APPEND. (But I'm now beginning to suspect that all of the set_foo_properties() commands behave that way as a convenience. It would make sense, as APPEND is only for use in set_property() (singular).)

...Aaaanyway, point is, threading a "nested" list through the eye of that needle would be quite the undertaking. But at the very least, I agree, the restriction should be documented.

[mheimlich]

As this does not work for lists of lists e.g. PATH, I created the pull request #2467.