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.

Sign up for GitHub

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

Jump to bottom

Cmake labels (take-over #2832) #2871

Open
wants to merge 10 commits into
base: devel
Choose a base branch
from
Open

Cmake labels (take-over #2832) #2871

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9a541e2
Create a CMake listing reporter
Bidski May 8, 2024
27e7468
Extract tags from CMake reporter for CTest
Bidski May 8, 2024
48eeaab
Update test baselines
Bidski May 8, 2024
a37e134
Use long form arguments rather than quoting
Bidski May 10, 2024
79334e2
Update test approvals
Bidski May 10, 2024
36689b0
Update amalgamated files
Bidski May 10, 2024
9693065
Add functions to generate brackets
Bidski May 14, 2024
6403650
Add function to remove brackets
Bidski May 14, 2024
907e20e
Remove unneeded strip
Bidski May 14, 2024
c69ca3d
Add test case and update approvals
Bidski May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 55 additions & 17 deletions extras/CatchAddTests.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ function(add_command NAME)
endfunction()

function(catch_discover_tests_impl)
# Don't ignore empty elements in a list
cmake_policy(SET CMP0007 NEW)

cmake_parse_arguments(
""
Expand Down Expand Up @@ -61,8 +63,8 @@ function(catch_discover_tests_impl)
endif()

execute_process(
COMMAND ${_TEST_EXECUTOR} "${_TEST_EXECUTABLE}" ${spec} --list-tests --verbosity quiet
OUTPUT_VARIABLE output
COMMAND ${_TEST_EXECUTOR} "${_TEST_EXECUTABLE}" ${spec} --list-tests --reporter cmake
OUTPUT_VARIABLE test_cases
RESULT_VARIABLE result
WORKING_DIRECTORY "${_TEST_WORKING_DIR}"
)
Expand All @@ -74,11 +76,8 @@ function(catch_discover_tests_impl)
)
endif()

# Make sure to escape ; (semicolons) in test names first, because
# that'd break the foreach loop for "Parse output" later and create
# wrongly splitted and thus failing test cases (false positives)
string(REPLACE ";" "\;" output "${output}")
string(REPLACE "\n" ";" output "${output}")
# Remove any leading or trailing whitespace from the output
string(STRIP "${test_cases}" test_cases)
Bidski marked this conversation as resolved.
Show resolved Hide resolved

# Prepare reporter
if(reporter)
Expand Down Expand Up @@ -121,24 +120,38 @@ function(catch_discover_tests_impl)
endforeach()
endif()

# Parse output
foreach(line ${output})
set(test "${line}")
foreach(test_case ${test_cases})
# Remove long form brackets
# [[...test case list...]]
string(LENGTH "${test_case}" test_case_length)
math(EXPR test_case_length "${test_case_length} - 4")
string(SUBSTRING "${test_case}" 2 ${test_case_length} test_case)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking more along the lines of set or if that doesn't work, eval. Something built-in to handle this.

Better yet, can't the reporter spit out a full cmake script with all the relevant set, list instructions etc.? Technically it is a security vulnerability, but I think there are better ways to attack the test script itself rather than test names

Copy link
Author

Choose a reason for hiding this comment

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

Generating code to be eval'd just sounds like a horrible idea, and as you point out, it's a security issue waiting to happen and I don't think it would be wise to take this approach.

Copy link
Contributor

@LecrisUT LecrisUT May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The only reason I suggested the block syntax was to take advantage of the built-in cmake handling. If we don't plan on using it I would prefer the original list of list approach with the escaping mechanism described in this file.

    # Escape characters in test case names that would be parsed by Catch2
    # Note that the \ escaping must happen FIRST! Do not change the order.
    foreach(char \\ , [ ])
      string(REPLACE ${char} "\\${char}" test_name "${test_name}")
    endforeach(char)

Security wise I don't think it's a significant attack vector, but you can still minimize it by disallowing ]=] syntax in the name, tag, file, etc.

Copy link
Author

Choose a reason for hiding this comment

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

By "block syntax" I assume you mean [=[/]=]? Unfortunately, CMake is seeing those and assuming that is the content of the string and never parses further than than (which I believe is how these strings are intended to work), which I why I have to strip the brackets off in order to process the list within.

We can revert to what I had before I opened this PR (I only escaped the the ; originally) and not use the extended bracket syntax. I admit that syntax is pretty clunky when it doesn't really work the way we would like. I left the escaping of \, ,, [, and ] in the CMake file since that escaping is more specific to putting it into the CTest file and may not be a necessary thing for others you may want to use the CMake reporter for a different purpose (I am assuming we should make the CMake reporter slightly more generic -- if we were to make it with the explicit purpose of using it in this CMake script then we may as well skip the CMake script entirely and just have the reporter dump the CTest script, but this is just as much of a security issue as generating CMake set commands)

Square brackets can't appear in tags already (I tested this, Catch2 forbids it). I'm not sure it is worth forbidding them from the test names.

It may not be a significant attack vector, but willingly introducing an attack vector just seems like a bad idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

By "block syntax" I assume you mean [=[/]=]? Unfortunately, CMake is seeing those and assuming that is the content of the string and never parses further

Indeed, the block/bracket syntax is only meant to be used within a CMake script. You only see it used in generated files to be parsed or eval.

I left the escaping of \, ,, [, and ] in the CMake file since that escaping is more specific to putting it into the CTest file

For the reporter you would have to be even more thorough unfortunately, escaping $ and other special symbols. This escape might also be needed with the manual handling of brackets here.

you may want to use the CMake reporter for a different purpose

The reporter should do only one thing, generate a CMake list of list. Which format is more appropriate is debatable. The choice is between the complexity of escaping list of lists and the potential security threats.

I personally think we can address the later quite easily in this case and having a cmake script would be more readable.

if we were to make it with the explicit purpose of using it in this CMake script then we may as well skip the CMake script entirely and just have the reporter dump the CTest script

I'm ok with either. The reusability of such a script outside ctest file generation is super limited since you have to finish building the whole project at that point. The only usage of this splitting is readability, but at that point the ctest file itself is more readable.

Hooking into the ctest file generation directly could be very useful for customizing the test properties, e.g. setting PROCESSOR test property for individual tests. It would require expanding the macro syntax though.

Copy link
Author

Choose a reason for hiding this comment

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

Do we just want to replace the cmake reporter with a utility (a reporter??) that just generates the ctest file then?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would vote for that approach. @horenmar can you chime in on this? Unfortunately I don't have a reference for other test-suites to see how they implement this, gtest for example doesn't have tight CMake support (gtest_discover_tests is defined in CMake).

For simplicity I would export all input variables like ${TEST_PROPERTIES} to environment variables like $ENV{CATCH_TEST_PROPERTIES} instead of defining additional --flags.

I don't think it needs to be a separate utility, but I didn't dig deep enough in the code to know what pitfalls I am overlooking. Maybe the confusion would be that "reporter" actually outputs the output of the run, and not just the structure, but we seem to be using the JSON one in a similar way.

Copy link
Author

Choose a reason for hiding this comment

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

@horenmar do you have an opinion on this? Would really like to get this sorted out


# Extract test name and tags
list(GET test_case 0 raw_test_name)
list(GET test_case 4 tags)

# Remove long form brackets from test name
# [=[...test name...]=]
string(LENGTH "${raw_test_name}" test_name_length)
math(EXPR test_name_length "${test_name_length} - 6")
string(SUBSTRING "${raw_test_name}" 3 ${test_name_length} test_name)

# Escape characters in test case names that would be parsed by Catch2
# Note that the \ escaping must happen FIRST! Do not change the order.
set(test_name "${test}")
foreach(char \\ , [ ])
string(REPLACE ${char} "\\${char}" test_name "${test_name}")
endforeach(char)

# ...add output dir
if(output_dir)
string(REGEX REPLACE "[^A-Za-z0-9_]" "_" test_name_clean "${test_name}")
set(output_dir_arg "--out ${output_dir}/${output_prefix}${test_name_clean}${output_suffix}")
endif()

# ...and add to script
# ... add to script
add_command(add_test
"${prefix}${test}${suffix}"
"${prefix}${raw_test_name}${suffix}"
${_TEST_EXECUTOR}
"${_TEST_EXECUTABLE}"
"${test_name}"
Expand All @@ -147,20 +160,45 @@ function(catch_discover_tests_impl)
"${output_dir_arg}"
)
add_command(set_tests_properties
"${prefix}${test}${suffix}"
"${prefix}${raw_test_name}${suffix}"
PROPERTIES
WORKING_DIRECTORY "${_TEST_WORKING_DIR}"
${properties}
)

# ... add any environment modifications
if(environment_modifications)
add_command(set_tests_properties
"${prefix}${test}${suffix}"
"${prefix}${raw_test_name}${suffix}"
PROPERTIES
ENVIRONMENT_MODIFICATION "${environment_modifications}")
ENVIRONMENT_MODIFICATION "${environment_modifications}"
)
endif()

# Remove long form brackets from tags list
# [=[...tags list...]=]
string(LENGTH "${tags}" tags_length)
if("${tags_length}" GREATER 0)
math(EXPR tags_length "${tags_length} - 6")
string(SUBSTRING "${tags}" 3 ${tags_length} tags)

# ... and any tags as labels
foreach(tag ${tags})
# Remove long form brackets from tag
# [==[...tags list...]==]
string(LENGTH "${tag}" tag_length)
math(EXPR tag_length "${tag_length} - 8")
string(SUBSTRING "${tag}" 4 ${tag_length} tag)

add_command(set_tests_properties
"${prefix}${raw_test_name}${suffix}"
PROPERTIES
LABELS "${tag}"
)
endforeach()
endif()

list(APPEND tests "${prefix}${test}${suffix}")
list(APPEND tests "${prefix}${raw_test_name}${suffix}")
endforeach()

# Create a list of all discovered tests, which users may use to e.g. set
Expand Down
106 changes: 105 additions & 1 deletion extras/catch_amalgamated.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
// SPDX-License-Identifier: BSL-1.0

// Catch v3.6.0
// Generated: 2024-05-05 20:53:27.562886
// Generated: 2024-05-10 11:52:28.385302
// ----------------------------------------------------------
// This file is an amalgamation of multiple different files.
// You probably shouldn't edit it directly.
Expand Down Expand Up @@ -5127,6 +5127,8 @@ namespace Catch {
// we have to add the elements manually
m_impl->factories["Automake"] =
Detail::make_unique<ReporterFactory<AutomakeReporter>>();
m_impl->factories["CMake"] =
Detail::make_unique<ReporterFactory<CMakeReporter>>();
m_impl->factories["compact"] =
Detail::make_unique<ReporterFactory<CompactReporter>>();
m_impl->factories["console"] =
Expand Down Expand Up @@ -8461,6 +8463,108 @@ namespace Catch {



#include <ostream>

namespace Catch {

namespace {
std::ostream& tagWriter( std::ostream& out,
std::vector<Tag> const& tags ) {
out << ';';
if ( tags.empty() ) { return out; }

out << "[=[" << "[==[" << tags.front().original << "]==]";
Copy link
Contributor

@LecrisUT LecrisUT May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can the number of equal signs be a variable, just in case? The brackets should also be limited to where we want to escape the strings, i.e. name, tag, etc. If we go with writing a full script it would be even more minimal.

An example script could be

set(test_data)
set(test_name [[<insert_test_name>]])
set(test_file [[<insert_test_file>]])
set(test_tags [[<insert_test_tag_0>]] ...)
list(APPEND test_data "${test_name};${test_file};${test_tags}")

You might need to play around with join/split and the need for escaping or not for list parameters like test_tags

for ( auto it = std::next( tags.begin() ); it != tags.end();
++it ) {
out << ';' << "[==[" << it->original << "]==]";
}
out << "]=]";
return out;
}

std::ostream& testWriter( std::ostream& out,
TestCaseHandle const& test ) {
auto const& info = test.getTestCaseInfo();
out << "[[" << "[=[" << info.name << "]=]" << ';' << "[=["
<< info.className << "]=]" << ';' << "[=[" << info.lineInfo.file
<< "]=]" << ';' << "[=[" << info.lineInfo.line << "]=]";
tagWriter( out, info.tags );
out << "]]";

return out;
}

} // namespace

std::string CMakeReporter::getDescription() {
return "Outputs listings as a CMake list";
}

void CMakeReporter::listReporters(
std::vector<ReporterDescription> const& descriptions ) {

if ( descriptions.empty() ) { return; }

m_stream << "[[" << "[=[" << descriptions.front().name << "]=]" << ';'
<< "[=[" << descriptions.front().description << "]=]" << "]]";
for ( auto it = std::next( descriptions.begin() );
it != descriptions.end();
++it ) {
m_stream << ';' << "[[" << "[=[" << it->name << "]=]" << ';'
<< "[=[" << it->description << "]=]" << "]]";
}

m_stream << '\n';
}

void CMakeReporter::listListeners(
std::vector<ListenerDescription> const& descriptions ) {

if ( descriptions.empty() ) { return; }

m_stream << "[[" << "[=[" << descriptions.front().name << "]=]" << ';'
<< "[=[" << descriptions.front().description << "]=]" << "]]";
for ( auto it = std::next( descriptions.begin() );
it != descriptions.end();
++it ) {
m_stream << ';' << "[[" << "[=[" << it->name << "]=]" << ';'
<< "[=[" << it->description << "]=]" << "]]";
}

m_stream << '\n';
}

void CMakeReporter::listTests( std::vector<TestCaseHandle> const& tests ) {
if ( tests.empty() ) { return; }

testWriter( m_stream, tests.front() );

for ( auto it = std::next( tests.begin() ); it != tests.end(); ++it ) {
m_stream << ';';
testWriter( m_stream, *it );
}

m_stream << '\n';
}

void CMakeReporter::listTags( std::vector<TagInfo> const& tags ) {
if ( tags.empty() ) { return; }

m_stream << "[[" << "[=[" << tags.front().count << "]=]" << ';' << "[=["
<< tags.front().all() << "]=]" << "]]";

for ( auto it = std::next( tags.begin() ); it != tags.end(); ++it ) {
m_stream << ';' << "[[" << "[=[" << it->count << "]=]" << ';'
<< "[=[" << it->all() << "]=]" << "]]";
}

m_stream << '\n';
}

} // end namespace Catch






Expand Down
27 changes: 26 additions & 1 deletion extras/catch_amalgamated.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
// SPDX-License-Identifier: BSL-1.0

// Catch v3.6.0
// Generated: 2024-05-05 20:53:27.071502
// Generated: 2024-05-10 11:52:28.378012
// ----------------------------------------------------------
// This file is an amalgamation of multiple different files.
// You probably shouldn't edit it directly.
Expand Down Expand Up @@ -13152,6 +13152,31 @@ namespace Catch {
#endif // CATCH_REPORTER_AUTOMAKE_HPP_INCLUDED


#ifndef CATCH_REPORTER_CMAKE_HPP_INCLUDED
#define CATCH_REPORTER_CMAKE_HPP_INCLUDED


namespace Catch {

class CMakeReporter final : public StreamingReporterBase {
public:
using StreamingReporterBase::StreamingReporterBase;

static std::string getDescription();

void listReporters(
std::vector<ReporterDescription> const& descriptions ) override;
void listListeners(
std::vector<ListenerDescription> const& descriptions ) override;
void listTests( std::vector<TestCaseHandle> const& tests ) override;
void listTags( std::vector<TagInfo> const& tags ) override;
};

} // end namespace Catch

#endif // CATCH_REPORTER_CMAKE_HPP_INCLUDED


#ifndef CATCH_REPORTER_COMPACT_HPP_INCLUDED
#define CATCH_REPORTER_COMPACT_HPP_INCLUDED

Expand Down
2 changes: 2 additions & 0 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ set(MATCHER_FILES ${MATCHER_HEADERS} ${MATCHER_SOURCES})

set(REPORTER_HEADERS
${SOURCES_DIR}/reporters/catch_reporter_automake.hpp
${SOURCES_DIR}/reporters/catch_reporter_cmake.hpp
${SOURCES_DIR}/reporters/catch_reporter_common_base.hpp
${SOURCES_DIR}/reporters/catch_reporter_compact.hpp
${SOURCES_DIR}/reporters/catch_reporter_console.hpp
Expand All @@ -308,6 +309,7 @@ set(REPORTER_HEADERS
)
set(REPORTER_SOURCES
${SOURCES_DIR}/reporters/catch_reporter_automake.cpp
${SOURCES_DIR}/reporters/catch_reporter_cmake.cpp
${SOURCES_DIR}/reporters/catch_reporter_common_base.cpp
${SOURCES_DIR}/reporters/catch_reporter_compact.cpp
${SOURCES_DIR}/reporters/catch_reporter_console.cpp
Expand Down
3 changes: 3 additions & 0 deletions src/catch2/internal/catch_reporter_registry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <catch2/internal/catch_move_and_forward.hpp>
#include <catch2/internal/catch_reporter_registry.hpp>
#include <catch2/reporters/catch_reporter_automake.hpp>
#include <catch2/reporters/catch_reporter_cmake.hpp>
#include <catch2/reporters/catch_reporter_compact.hpp>
#include <catch2/reporters/catch_reporter_console.hpp>
#include <catch2/reporters/catch_reporter_json.hpp>
Expand All @@ -34,6 +35,8 @@ namespace Catch {
// we have to add the elements manually
m_impl->factories["Automake"] =
Detail::make_unique<ReporterFactory<AutomakeReporter>>();
m_impl->factories["CMake"] =
Detail::make_unique<ReporterFactory<CMakeReporter>>();
m_impl->factories["compact"] =
Detail::make_unique<ReporterFactory<CompactReporter>>();
m_impl->factories["console"] =
Expand Down
2 changes: 2 additions & 0 deletions src/catch2/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ internal_sources = files(

reporter_headers = [
'reporters/catch_reporter_automake.hpp',
'reporters/catch_reporter_cmake.hpp',
'reporters/catch_reporter_common_base.hpp',
'reporters/catch_reporter_compact.hpp',
'reporters/catch_reporter_console.hpp',
Expand All @@ -302,6 +303,7 @@ reporter_headers = [

reporter_sources = files(
'reporters/catch_reporter_automake.cpp',
'reporters/catch_reporter_cmake.cpp',
'reporters/catch_reporter_common_base.cpp',
'reporters/catch_reporter_compact.cpp',
'reporters/catch_reporter_console.cpp',
Expand Down
Loading