Add cmake option/function to be able to skip mimick based tests

[ivanpauno]

• Export cmake function that can be used in downstream packages to selectively disable mimick based tests.
• Add cmake option to skip building mimick.

Related to ros2/rcl#923.

[clalancette]

I think it is worthwhile to step back and think about what we are trying to accomplish here.

From my understanding, the problem is that the Mimick library only supports a limited number of systems. For any system that is not on that supported list, we want to both skip building the mimick vendor library, and also skip building/running any downstream tests that rely on Mimick. Does that properly describe the issue?

Assuming that is the case, I think it is worthwhile to see if we can do the following:

1. Automatically detect whether this system is on the supported list, with a possible override to force mimick support on.
2. Export both a CMake variable and a header define that exports whether Mimick is supported (this PR, though I'd prefer the positive HAS_MIMICK_SUPPORT rather than the negative SKIP_MIMICK).
3. Refactor downstream code to use those defines/cmake variables. I will say that I don't love Add option to skip mimick based tests rcl#923 ; there's a lot of ifndef SKIP_MIMICK in there, and we'll never test the !MIMICK case in CI to make sure it works. One way to improve it might be to refactor tests so that Mimick-related tests are in the separate files, in which case we can use the CMake variable to skip building the test completely.

[ivanpauno]

mimick vendor library, and also skip building/running any downstream tests that rely on Mimick. Does that properly describe the issue?

We want to skip building mimick, if we build mimick_vendor or not isn't important.

Automatically detect whether this system is on the supported list, with a possible override to force mimick support on.

We could do it automatically.
I preferred to add a cmake option so the user is consciously disabling the tests, but there's not much reason for that.

this PR, though I'd prefer the positive HAS_MIMICK_SUPPORT rather than the negative SKIP_MIMICK

I used the negative, because by default we're building mimick tests and I didn't want IDEs to show all the code enclosed by an #ifdef HAS_MIMICK_SUPPORT as disabled.
IDEs don't know what compile definitions we're passing through cmake, at least mine wasn't figuring that out.

I will say that I don't love Add option to skip mimick based tests ros2/rcl#923 ; there's a lot of ifndef SKIP_MIMICK in there

I also don't love how it looks, but currently it's imposible to build rcl tests in platforms that don't support mimick, which isn't great.
We can also simple disable all tests for that package, that's what I ended up doing in rcljava Android CI, but I think it's better to be able to disable only the mimick based tests.

and we'll never test the !MIMICK case in CI to make sure it works.

That's true.
I guess if all our mimick tests are using the #ifndef SKIP_MIMICK block, people adding new tests will copy that style.
It can be a best effort thing, and external users relying on the option could report when we break it.
We could also have a CI job for it 😃 .

One way to improve it might be to refactor tests so that Mimick-related tests are in the separate files, in which case we can use the CMake variable to skip building the test completely.

Moving the mimick tests to separate files might create some unnecessary code duplication, e.g. test_node and test_node_mimick might need the same setup() work. I'm not sure if this is actually the case.
Doing that is also much more work than ros2/rcl#923.

[hidmic]

Patch looks good to me.

---

About concerns discussed above:

Automatically detect whether this system is on the supported list, with a possible override to force mimick support on.

I'm inclined to agree with @ivanpauno: disabling tests on a development workspace should be a conscious decision. Alas, that's not what we do with pytest, for instance. So it may be worth reflecting on that a bit? Why is that the case @clalancette?

I'd prefer the positive HAS_MIMICK_SUPPORT rather than the negative SKIP_MIMICK
I didn't want IDEs to show all the code enclosed by an #ifdef HAS_MIMICK_SUPPORT as disabled.
[...] there's a lot of ifndef SKIP_MIMICK in there [...]

It seems the issue here is mostly cosmetic. How about introducing a block macro? Several tests using patch.hpp use block statements already to scope mocks. Something like:

(MIMICK|PATCH)_BLOCK({
   ...
});

would obscure all conditionals and fool smart IDEs (AFAIK).

to refactor tests so that Mimick-related tests are in the separate files, in which case we can use the CMake variable to skip building the test completely.

I have to disagree here. It'd result in code duplication or force a refactor to avoid it, making tests more complex.

[hidmic]

@ivanpauno nit

Suggested change

	"When ON, mimick is not going to be build and mimick based tests are not going to be run"
	"When ON, mimick is not going to be built"

The second half of the sentence jumps ahead of itself a bit.

[ivanpauno]

@clalancette friendly ping, do you think that this change and ros2/rcl#923 would be acceptable?

[clalancette]

Automatically detect whether this system is on the supported list, with a possible override to force mimick support on.

I'm inclined to agree with @ivanpauno: disabling tests on a development workspace should be a conscious decision. Alas, that's not what we do with pytest, for instance. So it may be worth reflecting on that a bit? Why is that the case @clalancette?

The thing is, we know that Mimick only works on certain platforms (whatever those may be). So there is no chance that it is going to compile for other platforms. Having to have the user specify that seems unfriendly; I feel like we should automatically detect that and skip the tests. I guess the counter-point is that it "hides" the fact that the tests aren't running.

I'd prefer the positive HAS_MIMICK_SUPPORT rather than the negative SKIP_MIMICK
I didn't want IDEs to show all the code enclosed by an #ifdef HAS_MIMICK_SUPPORT as disabled.
[...] there's a lot of ifndef SKIP_MIMICK in there [...]

It seems the issue here is mostly cosmetic. How about introducing a block macro? Several tests using patch.hpp use block statements already to scope mocks. Something like:

(MIMICK|PATCH)_BLOCK({
   ...
});

would obscure all conditionals and fool smart IDEs (AFAIK).

Yeah, I think I would like that better. At least that way we don't have so much #ifdef stuff in the tests themselves.

to refactor tests so that Mimick-related tests are in the separate files, in which case we can use the CMake variable to skip building the test completely.

I have to disagree here. It'd result in code duplication or force a refactor to avoid it, making tests more complex.

That's fair.

Overall, I think I'd be happier with this if we had the MIMICK_BLOCK like @ivanpauno suggested.