Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: add Google Test and use it to test space-time-stack connector #237

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

romintomasetti
Copy link
Contributor

@romintomasetti romintomasetti commented Feb 6, 2024

This PR adds Google Test.

Google Test is then used in test_demangling.cpp with the space-time-stack tool.

@romintomasetti
Copy link
Contributor Author

romintomasetti commented Feb 6, 2024

@masterleinad Is this version better?

@romintomasetti romintomasetti force-pushed the add-google-test branch 3 times, most recently from edbef3c to 47fd4f6 Compare February 6, 2024 19:43
@romintomasetti
Copy link
Contributor Author

romintomasetti commented Feb 6, 2024

@vlkale @dalg24 @masterleinad

Copy link
Contributor

@vlkale vlkale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romintomasetti Thanks for this PR on Google Tests!

My main question/comment on this PR is about whether it serves for testing just the Kokkos Tools connector space time stack, or if some or all this PR can be generalized to work for any Kokkos Tools? It seems like the latter. It would be good to differentiate and factor out tests common across all tools.

@romintomasetti
Copy link
Contributor Author

@vlkale I think it should be used to test all the connectors. As I understand it, it seems the recurring pattern will probably be to have a common "unit test cpp file" that initializes Google Test, but not Kokkos. Because tests will likely check what happens when Kokkos finalizes, so each test is likely to initialize and finalize Kokkos, as I did in test_demangling.cpp in this PR.

I would propose that generalizations come at a later stage, when more connectors are tested. IMO it's not the purpose of this PR to add any additional test 😉

@vlkale
Copy link
Contributor

vlkale commented Feb 6, 2024

@romintomasetti I see now, looking again at the most recent comments of PR #236.

Maybe the title of the PR could say it's solely for space-time-stack (to avoid confusion to others with other future Google Test PRs). Anyway, I see what you're doing here.

Looking at your files in just this PR and from my understanding of Google Tests, it is good with me.

@romintomasetti romintomasetti changed the title tests: add Google Test tests: add Google Test and use it to test space-time-stack connector Feb 6, 2024
@romintomasetti
Copy link
Contributor Author

@vlkale Good! I've updated the title 😉

@vlkale
Copy link
Contributor

vlkale commented Feb 6, 2024

@vlkale I think it should be used to test all the connectors. As I understand it, it seems the recurring pattern will probably be to have a common "unit test cpp file" that initializes Google Test, but not Kokkos. Because tests will likely check what happens when Kokkos finalizes, so each test is likely to initialize and finalize Kokkos, as I did in test_demangling.cpp in this PR.

Yes, this is one good approach to the design.

Checking that this indeed makes sense for all other Kokkos Tools connectors (in existence at least) should certainly be a separate PR :)

# SOURCE_FILE : source file, defaults to <TARGET_NAME>.cpp
function(add_one_test)

cmake_parse_arguments(aot_args "" "TARGET_NAME;SOURCE_FILE" "" ${ARGN})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

Suggested change
cmake_parse_arguments(aot_args "" "TARGET_NAME;SOURCE_FILE" "" ${ARGN})
cmake_parse_arguments(TEST_ARGS "" "TARGET_NAME;SOURCE_FILE" "" ${ARGN})

to have consistent capitalization?

Copy link
Contributor Author

@romintomasetti romintomasetti Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention I usually follow for the PREFIX given to cmake_parse_arguments is the following:

  1. Take the first letter of each "word" in the function name (here *a*dd_*o*ne_*t*est leads to aot).
  2. Append _args.

I think there are 2 advantages using this convention:

  1. You reduce the probability that the chosen prefix matches a variable from the parent scope (note how easy it would be to get someone define TEST_ARGS somewhere in the parent scope for instance, while aot_args seems less probable IMO).
  2. You get a consistent and indisputable recipe for the PREFIX.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO having the PREFIX in lower case also helps reading the function's code 👓

Copy link
Member

@dalg24 dalg24 Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I also dislike the prefix you picked
(Not blocking though)

test_demangling.cpp
kp_add_executable_and_test(
TARGET_NAME test_space_time_stack_demangling
SOURCE_FILE test_demangling
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this need to be

Suggested change
SOURCE_FILE test_demangling
SOURCE_FILE test_demangling.cpp

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added .cpp. But for posterity, it seems CMake will try to find your file by testing several suffixes:

-- Configuring done (1.1s)
CMake Error at tests/CMakeLists.txt:22 (target_sources):
  Cannot find source file:

    test_demanglig

  Tried extensions .c .C .c++ .cc .cpp .cxx .cu .mpp .m .M .mm .ixx .cppm
  .ccm .cxxm .c++m .h .hh .h++ .hm .hpp .hxx .in .txx .f .F .for .f77 .f90
  .f95 .f03 .hip .ispc
Call Stack (most recent call first):
  tests/space-time-stack/CMakeLists.txt:2 (kp_add_executable_and_test)


-- Generating done (0.0s)

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me.

@dalg24 dalg24 merged commit f67e816 into kokkos:develop Feb 15, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants