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

Allow types as test section names #91

Conversation

AutonomicPerfectionist
Copy link
Contributor

@AutonomicPerfectionist AutonomicPerfectionist commented Sep 21, 2023

Is this pull request associated with an issue(s)?
Fixes #90

Description
This PR allows CMakePP types to be used as test section names, like bool.
Unfortunately, it does not appear possible to blanket allow all test names, since the
name is to also pass a function identifier back. A solution to this would be to add another
parameter to ct_add_section() to define the name of the return variable that is then set to the function ID.
Pros would be any name would be valid for a test section, but cons would be two different ways to create a test.

If we wanted to add the new parameter, it would be best to do it as soon as possible, before the 1.0 release. It could be considered that we should remove the old way of implicitly passing the id in the name, but that would require a much larger refactor across all current projects using CMakeTest.

TODOs

  • Add support for the same in top-level tests
  • Add documentation specifying in the user manual what names are and are not allowed

@ryanmrichard
Copy link
Contributor

Can't we have both APIs by defaulting to using the section name as the name of return variable? Something like:

# RETURN_ID is an optional keyword that can be used to specify the name of the variable to
# assign the function name to.
function(ct_add_section <name> RETURN_ID <return_name>)
if(return_name not set)
   # Probably need to sanitize the name or whatever here
   set(return_name "${name}")
endif()

# stuff

set(${return_name} ... PARENT_SCOPE)
endfunction()

@AutonomicPerfectionist
Copy link
Contributor Author

That would indeed be an option, and is exactly what I thought of first. It should work, only potential downside would be having two ways of naming the section that might be confusing. But so long as it's documented well I think we can mitigate the confusion.

I will need to do more tests to make sure this is possible though, as I believe the name is also used elsewhere for state tracking

@AutonomicPerfectionist
Copy link
Contributor Author

@ryanmrichard apparently functionality was added in CMake 3.1 that allows you to escape special characters in variable names, including spaces. It's not enabled by default but can be enabled by setting a policy. So there is potentially no need to add the RETURN_ID parameter to ct_add_section(), so long as the test authors understand that those names are dependent on the cmake version and the policy. Do you think we should just use that or should I continue implementing the new option?

https://cmake.org/cmake/help/v3.22/policy/CMP0053.html#policy:CMP0053

@ryanmrichard
Copy link
Contributor

@ryanmrichard apparently functionality was added in CMake 3.1 that allows you to escape special characters in variable names, including spaces. It's not enabled by default but can be enabled by setting a policy. So there is potentially no need to add the RETURN_ID parameter to ct_add_section(), so long as the test authors understand that those names are dependent on the cmake version and the policy. Do you think we should just use that or should I continue implementing the new option?

https://cmake.org/cmake/help/v3.22/policy/CMP0053.html#policy:CMP0053

Generally speaking, I dislike relying on CMake policies unless necessary. Even with the policy, it still is cumbersome:

ct_add_section("what will be an ugly function name")
function("${what\ will\ be\ an\ ugly\ function\ name}")
    # test goes here
endfunction()

Another thought I had was to modify the API (we'd need to unfortunately update our tests, but that's okay as it'd be an easy change). The idea would be that ct_add_test and ct_add_section would respectively set the CMAKETEST_TEST and CMAKETEST_SECTION variables with the name the user should use. So I'm thinking a test would look like:

# Assume file name test.cmake
ct_add_test(NAME "my test with an ug|y n@me")
function("${CMAKETEST_TEST}") # function name: test_cmake_1 
    
    ct_add_section("a!so an ugly n@me")
    function("${CMAKETEST_SECTION}") # function name: test_cmake_1_1

        ct_add_section("a_less_ugly_name")
        function("${CMAKETEST_SECTION}") # function name: test_cmake_1_1_1
        endfunction()

        ct_add_section("another subtest")
        function("${CMAKETEST_SECTION}") # function name: test_cmake_1_1_2
        endfunction()

    endfunction()
    
    ct_add_section("a!so an ugly n@me")
    function("${CMAKETEST_SECTION}") # function name: test_cmake_1_2
    endfunction()
endfunction()

For this to work we'd have to track nesting, but I think we do this already, right?

@ryanmrichard
Copy link
Contributor

An advantage of my proposal is that if a user changes a test's name they only have to do it once (in the call to ct_add_test() or ct_add_section()), not in both the call to CMakeTest and in the function declaration.

@ryanmrichard
Copy link
Contributor

I guess we do have outside users, so maybe an API break isn't the best idea. What if we continue the current behavior (setting the test/section name to the function name the user should use) AND also set CMAKETEST_TEST/CMAKETEST_SECTION to the function name? FWIW, we don't have to adhere to <file_name>_x_y_z for function names if whatever we're doing now would still work.

@AutonomicPerfectionist
Copy link
Contributor Author

I like the idea of setting both, I think that should be the least invasive solution. Requiring only one place to change the name of a test is definitely an improvement, I've forgotten to update the second one many times before.

I'll do some investigation when I have time to make sure there won't be any unintended side effects. Perhaps we should also mark the current way as deprecated in the documentation?

@ryanmrichard
Copy link
Contributor

I agree marking it as deprecated would be a good idea.

@AutonomicPerfectionist AutonomicPerfectionist marked this pull request as ready for review October 29, 2023 00:01
Copy link
Contributor

@ryanmrichard ryanmrichard left a comment

Choose a reason for hiding this comment

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

Looks great!

@ryanmrichard ryanmrichard merged commit 163e3df into master Oct 31, 2023
6 checks passed
@ryanmrichard ryanmrichard deleted the 90-bug-tests-with-names-corresponding-to-cmakepp-types-causes-a-crash branch October 31, 2023 20:47
Copy link

🚀 [bumpr] Bumped! New version:v0.1.8 Changes:v0.1.7...v0.1.8

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.

[BUG] Tests with names corresponding to CMakePP types causes a crash
2 participants