Put in tests for sampler

[vlkale]

This PR addresses the broader Github Issue #191, which is important for the open-source development of Kokkos Tools.

The purpose of this PR is to enhance and ensure the Kokkos Tools sampler robustness by adding tests for it. Testing the sampler is important given that:

(1) It is a parent tool to other tools connectors, i.e., other tools using it will not work if this tool does not call them correctly;
(2) It uses the parts of Kokkos Tools infrastructure that the sampler must exercise to maintain low overhead, i.e., tool-invoked fencing.

This PR adds a directory in tests for the sampler utility and tests that the sampler works correctly. Specifically, it checks that:

1. every begin callback from the sampler has a matching end callback; and
2. the sampler chooses the correct kernel IDs to sample based on the skip rate

The tests use CTest, and tries to follow the demangling tests for space time stack, but there are important changes needed to the general CMakeLists.txt file to facilitate testing of the sampler.

Note that corresponding testing will be needed for the kernel filter, which would be a separate PR. Testing for all tool connectors and this needs to be a separate or multiple separate PRs.

[vlkale]

Your test is really weak. It will pass if the sampler does nothing and just always forward to the underlying tool.

@dalg24 Thanks. However, I do not believe this is the case: It will not pass if the sampler does nothing. In my test code, all matchers must be seen for this to pass. Also, the EXPECT_THAT function I have will cause a test to fail if the regular expression is not seen. This is the pertinent Google Tests documentation

https://google.github.io/googletest/reference/matchers.html

@dalg24 On second thought, you may be right - I re-read in the documentation that the matcher matches a single argument. The safest way to fix this is to just match the exact and full string I want to match in one matcher.

Assume we merge your test as it is proposed now and a follow PR includes this change.

diff --git a/common/kokkos-sampler/kp_sampler_skip.cpp b/common/kokkos-sampler/kp_sampler_skip.cpp

index 78cbbff..86b55e3 100644

--- a/common/kokkos-sampler/kp_sampler_skip.cpp

+++ b/common/kokkos-sampler/kp_sampler_skip.cpp

@@ -193,7 +193,7 @@ void kokkosp_begin_parallel_for(const char* name, const uint32_t devID,

                                 uint64_t* kID) {

   *kID                          = uniqID++;

   static uint64_t invocationNum = 0;

-  ++invocationNum;

+  invocationNum;

   if ((invocationNum % kernelSampleSkip) == 0) {

     if (tool_verbosity > 0) {

       std::cout << "KokkosP: sample " << *kID

Would you not want your test to fail?

I see you are showing a mistake by the programmer where they have not incremented the invocation number.

The ideal way to test to cover that is to create a regular expression in RE2 that captures the fact that there exists no samples between 0 and 149 that aren't 51 or 102.

However, as I was reading about RE2 last week in documentation and stackoverflow, there is a known problem with regular expression lookahead on some architectures and this would not be portable.

I think one easier way for now is to literally write an EXPECT_THAT for every single string that should not be in the output because sampling eliminated them, using Not(HasSubstr()). I had done that for three of them in my commit on Friday. Maybe even easier: reduce the number of invocations to 15, still taking two samples, and test the above by writing just 13 EXPECT_THAT's that test that those 13 samples haven't been taken. I will create this alternate simpler test to handle this. We can use this even easier one to test what you say.

I have a broader question here: What role should tests for Kokkos Tools play?

In the code you show above, the tool connector programmer shouldn't have written that in the first place.

I think the tests for Kokkos Tools should emphasize more catching unexpected problems at runtime within the CI environments that are hard for tools connectors programmers to grasp without understanding Kokkos Tools and infrastructure. The null fence function pointer is just one example of this. The bug you bring up is important too, but it is another category of bugs involving a simpler programming error.

I think that testing of Kokkos Tools should follow testing in Kokkos core. A difference between Kokkos Tools and Kokkos core is that Kokkos core has more externally contributed libraries/connectors rather than contributions by Kokkos developers. That's why testing is very important in Kokkos Tools. I would say documentation for a Kokkos Tools connector developers (on how to write any Kokkos Tools connector) to complement testing Kokkos Tools may be a good thing to do.

[dalg24]

I have a broader question here: What role should tests for Kokkos Tools play?

Ensure that the tools behave as they are supposed to and are compatible with Kokkos Core.

If the tests we provide do not fail for the change I suggested above, then these tests are no good.
There are other ways to ensure that the tool output is right that do not involve complex regular expressions.

[vlkale]

I have a broader question here: What role should tests for Kokkos Tools play?

Ensure that the tools behave as they are supposed to and are compatible with Kokkos Core.

OK. This is consistent with the way I see it at a high-level, and this captures it concisely. I will make sure the tests address these two aspects.

By the way, I think Kokkos Tools connector documentation should also lay out this high-level requirement of testing for Kokkos Tools connectors developers so they can write out their own tests - this can be looked at later though.

If the tests we provide do not fail for the change I suggested above, then these tests are no good.

That makes sense and sounds good. I guess you were showing that example for simplicity. Maybe another possibility is that the code is correct but the variable invocationNum gets overwritten due to memory corruption from system-level/OS events. Anyway, it makes sense to create a test that invocationNum is indeed getting incremented appropriately.

There are other ways to ensure that the tool output is right that do not involve complex regular expressions.

Yes, I will do without. A regex can simplify code and repetition, but it's looking like not here.

[vlkale]

@dalg24 @masterleinad I think I have answered most of your questions. The CI tests are now passing with the changes. Please let me know if there is still anything unresolved. I have left all comments unresolved until you have looked at it.

@masterleinad I am keeping std::cout for this PR for the fatal error, see comment above on why.