From b32f622148794240844fd4a6fe69474defc57c7e Mon Sep 17 00:00:00 2001 From: scarf Date: Wed, 19 Apr 2023 04:38:01 +0900 Subject: [PATCH] ci: add problem matchers (#2660) * feat: display multi-line error messages with github action Co-authored-by: Qrox * Add debugmsg problem matcher This problem matcher is intended to highlight errors coming from hitting debugmsg in CI. Co-authored-by: John Bytheway * Make path appear in __FILE__ for tests The DebugLog printing of filenames relies on __FILE__. This is based on the filename passed to the compiler. For tests, that lacked a directory name. So, __FILE__ lacked the context for the filename, which meant the GitHub problem matcher couldn't find the file causing the error. Add '../tests/' to the filename passed to the compiler to work around this. Co-authored-by: John Bytheway * Add Catch2 problem matcher This is intended to highlight test failures in the Catch2 tests as GitHub annotations. Co-authored-by: John Bytheway * ci: add msvc problem matchers uses https://github.com/marketplace/actions/msvc-problem-matcher * style: false positive for `\r` --------- Co-authored-by: Qrox Co-authored-by: John Bytheway --- .github/workflows/msvc-full-features.yml | 1 + build-scripts/build.sh | 2 +- build-scripts/problem-matchers/catch2.json | 25 ++++ build-scripts/problem-matchers/debugmsg.json | 16 +++ build-scripts/requirements.sh | 5 + src/cached_options.cpp | 1 + src/cached_options.h | 12 ++ src/debug.cpp | 2 + src/json.cpp | 140 +++++++++++++++++-- tests/Makefile | 7 +- tests/json_test.cpp | 10 ++ tests/test_main.cpp | 15 +- 12 files changed, 222 insertions(+), 14 deletions(-) create mode 100644 build-scripts/problem-matchers/catch2.json create mode 100644 build-scripts/problem-matchers/debugmsg.json diff --git a/.github/workflows/msvc-full-features.yml b/.github/workflows/msvc-full-features.yml index ef58c156f771..1c93d6e9ae17 100644 --- a/.github/workflows/msvc-full-features.yml +++ b/.github/workflows/msvc-full-features.yml @@ -78,6 +78,7 @@ jobs: run: | vcpkg integrate install + - uses: ammaraskar/msvc-problem-matcher@master - name: Build run: | cd msvc-full-features diff --git a/build-scripts/build.sh b/build-scripts/build.sh index 01481c2e56ff..1fa34f1e4fcc 100755 --- a/build-scripts/build.sh +++ b/build-scripts/build.sh @@ -9,7 +9,7 @@ num_jobs=3 function run_tests { # The grep suppresses lines that begin with "0.0## s:", which are timing lines for tests with a very short duration. - $WINE "$@" -d yes --use-colour yes --rng-seed time $EXTRA_TEST_OPTS | grep -Ev "^0\.0[0-9]{2} s:" + $WINE "$@" -d yes --use-colour yes --rng-seed time --error-format=github-action $EXTRA_TEST_OPTS | grep -Ev "^0\.0[0-9]{2} s:" } # We might need binaries installed via pip, so ensure that our personal bin dir is on the PATH diff --git a/build-scripts/problem-matchers/catch2.json b/build-scripts/problem-matchers/catch2.json new file mode 100644 index 000000000000..c93e16f84e7e --- /dev/null +++ b/build-scripts/problem-matchers/catch2.json @@ -0,0 +1,25 @@ +{ + "problemMatcher": [ + { + "owner": "cata-catch2", + "pattern": [ + { + "regexp": "^(?:.\\[[0-9;]+m)*(?:\\.\\./)*([^:]*):(\\d+): .*FAILED:.*$", + "file": 1, + "line": 2 + }, + { + "regexp": "^(?:.\\[[0-9;]+m)* (.+)$", + "code": 1 + }, + { + "regexp": "^(?:.\\[[0-9;]+m)*(with expansion|due to a fatal error condition):$" + }, + { + "regexp": "^(?:.\\[[0-9;]+m)* (.+)$", + "message": 1 + } + ] + } + ] +} diff --git a/build-scripts/problem-matchers/debugmsg.json b/build-scripts/problem-matchers/debugmsg.json new file mode 100644 index 000000000000..3e0232b6a984 --- /dev/null +++ b/build-scripts/problem-matchers/debugmsg.json @@ -0,0 +1,16 @@ +{ + "problemMatcher": [ + { + "owner": "cata-clang", + "pattern": [ + { + "regexp": "^(?:[0-9.:]+|\\(continued from above\\)) (ERROR) : (?:(?:\\.\\./)*)([^:]*):(\\d+) (.+)$", + "severity": 1, + "file": 2, + "line": 3, + "message": 4 + } + ] + } + ] +} diff --git a/build-scripts/requirements.sh b/build-scripts/requirements.sh index 7fa5b17060d4..a8d3fb9c36ae 100644 --- a/build-scripts/requirements.sh +++ b/build-scripts/requirements.sh @@ -3,6 +3,11 @@ set -e set -x +# Enable GitHub actions problem matchers +# (See https://github.com/actions/toolkit/blob/master/docs/problem-matchers.md) +echo "::add-matcher::build-scripts/problem-matchers/catch2.json" +echo "::add-matcher::build-scripts/problem-matchers/debugmsg.json" + if [ -n "$CATA_CLANG_TIDY" ]; then pip install --user wheel --upgrade pip install --user 'lit==0.11.1' 'click==7.1.2' diff --git a/src/cached_options.cpp b/src/cached_options.cpp index c9b1b2f798d3..c3e39fd16a58 100644 --- a/src/cached_options.cpp +++ b/src/cached_options.cpp @@ -17,3 +17,4 @@ int fov_3d_z_range; bool tile_iso; bool pixel_minimap_option = false; int PICKUP_RANGE; +error_log_format_t error_log_format = error_log_format_t::human_readable; diff --git a/src/cached_options.h b/src/cached_options.h index 265c060c096e..3409b9e7fd8d 100644 --- a/src/cached_options.h +++ b/src/cached_options.h @@ -84,4 +84,16 @@ extern int PICKUP_RANGE; */ extern bool dont_debugmsg; +enum class error_log_format_t { + human_readable, + // Output error messages in github action command format (currently json only) + // See https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-commands-for-github-actions#setting-an-error-message + github_action, +}; +#ifndef CATA_IN_TOOL +extern error_log_format_t error_log_format; +#else +constexpr error_log_format_t error_log_format = error_log_format_t::human_readable; +#endif + #endif // CATA_SRC_CACHED_OPTIONS_H diff --git a/src/debug.cpp b/src/debug.cpp index 1531b56dad0a..3969046f34c1 100644 --- a/src/debug.cpp +++ b/src/debug.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -1200,6 +1201,7 @@ detail::DebugLogGuard detail::realDebugLog( DL lev, DC cl, const char *filename, // Cool down for 60s between backtrace emissions. next_backtrace = after + 60; out << "Backtrace emission took " << after - now << " seconds." << std::endl; + out << "(continued from above) " << io::enum_to_string( lev ) << ": "; } #endif diff --git a/src/json.cpp b/src/json.cpp index d4dfc3057adc..e488643aa8d7 100644 --- a/src/json.cpp +++ b/src/json.cpp @@ -18,6 +18,7 @@ #include #include "cached_options.h" +#include "cata_utility.h" #include "debug.h" #include "string_formatter.h" #include "string_utils.h" @@ -1701,16 +1702,114 @@ bool JsonIn::read( JsonDeserializer &j, bool throw_on_error ) } } +/** + * Get the normal form of a relative path. Does not work on absolute paths. + * Slash and backslash are both treated as path separators and replaced with + * slash. Trailing slashes are always removed. + * TODO: figure out how to use std::filesystem on android ndk + */ +static std::string normalize_relative_path( const std::string &path ) +{ + if( path.empty() ) { + // normal form of an empty path is an empty path + return path; + } + std::vector names; + for( size_t name_start = 0; name_start < path.size(); ) { + const size_t name_end = std::min( path.find_first_of( "\\/", name_start ), + path.size() ); + if( name_start < name_end ) { + const std::string name = path.substr( name_start, name_end - name_start ); + if( name == "." ) { + // do nothing + } else if( name == ".." ) { + if( names.empty() || names.back() == ".." ) { + names.emplace_back( name ); + } else { + names.pop_back(); + } + } else { + names.emplace_back( name ); + } + } + name_start = std::min( path.find_first_not_of( "\\/", name_end ), + path.size() ); + } + if( names.empty() ) { + return "."; + } else { + std::string normpath; + for( auto it = names.begin(); it != names.end(); ++it ) { + if( it != names.begin() ) { + normpath += "/"; + } + normpath += *it; + } + return normpath; + } +} + +/** + * Escape special chars in github action command properties. + * See https://github.com/actions/toolkit/blob/main/packages/core/src/command.ts + */ +static std::string escape_property( std::string str ) +{ + switch( error_log_format ) { + case error_log_format_t::human_readable: + break; + case error_log_format_t::github_action: + replace_all( str, "%", "%25" ); + // NOLINTNEXTLINE(cata-text-style) + replace_all( str, "\r", "%0D" ); + replace_all( str, "\n", "%0A" ); + replace_all( str, ":", "%3A" ); + replace_all( str, ",", "%2C" ); + break; + } + return str; +} + +/** + * Escape special chars in github action command messages. + * See https://github.com/actions/toolkit/blob/main/packages/core/src/command.ts + */ +static std::string escape_data( std::string str ) +{ + switch( error_log_format ) { + case error_log_format_t::human_readable: + break; + case error_log_format_t::github_action: + replace_all( str, "%", "%25" ); + // NOLINTNEXTLINE(cata-text-style) + replace_all( str, "\r", "%0D" ); + replace_all( str, "\n", "%0A" ); + break; + } + return str; +} + /* error display */ // WARNING: for occasional use only. std::string JsonIn::line_number( int offset_modifier ) { - const std::string &name = path ? *path : ""; + const std::string &name = escape_property( path ? normalize_relative_path( *path ) + : "" ); if( stream && stream->eof() ) { - return name + ":EOF"; + switch( error_log_format ) { + case error_log_format_t::human_readable: + return name + ":EOF"; + case error_log_format_t::github_action: + return "file=" + name + ",line=EOF"; + } } else if( !stream || stream->fail() ) { - return name + ":???"; + switch( error_log_format ) { + case error_log_format_t::human_readable: + return name + ":???"; + case error_log_format_t::github_action: + return "file=" + name + ",line=???"; + } } // else stream is fine int pos = tell(); int line = 1; @@ -1738,18 +1837,41 @@ std::string JsonIn::line_number( int offset_modifier ) } seek( pos ); std::stringstream ret; - ret << name << ":" << line << ":" << offset; + switch( error_log_format ) { + case error_log_format_t::human_readable: + ret << name << ":" << line << ":" << offset; + break; + case error_log_format_t::github_action: + ret.imbue( std::locale::classic() ); + ret << "file=" << name << ",line=" << line << ",col=" << offset; + break; + } return ret.str(); } void JsonIn::error( const std::string &message, int offset ) { - std::ostringstream err; - err << "Json error: " << line_number( offset ) << ": " << message; + std::ostringstream err_header; + switch( error_log_format ) { + case error_log_format_t::human_readable: + err_header << "Json error: " << line_number( offset ) << ": "; + break; + case error_log_format_t::github_action: + err_header << "::error " << line_number( offset ) << "::"; + break; + } // if we can't get more info from the stream don't try if( !stream->good() ) { - throw JsonError( err.str() ); + throw JsonError( err_header.str() + escape_data( message ) ); } + // Seek to eof after throwing to avoid continue reading from the incorrect + // location. The calling code of json error methods is supposed to restore + // the stream location if it wishes to recover from the error. + on_out_of_scope seek_to_eof( [this]() { + stream->seekg( 0, std::istream::end ); + } ); + std::ostringstream err; + err << message; // also print surrounding few lines of context, if not too large err << "\n\n"; stream->seekg( offset, std::istream::cur ); @@ -1757,7 +1879,7 @@ void JsonIn::error( const std::string &message, int offset ) rewind( 3, 240 ); size_t startpos = tell(); std::string buffer( pos - startpos, '\0' ); - stream->read( &buffer[0], pos - startpos ); + stream->read( buffer.data(), pos - startpos ); auto it = buffer.begin(); for( ; it < buffer.end() && ( *it == '\r' || *it == '\n' ); ++it ) { // skip starting newlines @@ -1819,7 +1941,7 @@ void JsonIn::error( const std::string &message, int offset ) if( !msg.empty() && msg.back() != '\n' ) { msg.push_back( '\n' ); } - throw JsonError( msg ); + throw JsonError( err_header.str() + escape_data( msg ) ); } void JsonIn::string_error( const std::string &message, const int offset ) diff --git a/tests/Makefile b/tests/Makefile index 773c783c49ad..75a5705ffb7c 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -23,7 +23,7 @@ DEFINES += -DCATCH_CONFIG_ENABLE_BENCHMARKING # Add no-sign-compare to fix MXE issue when compiling # Catch also uses "#pragma gcc diagnostic", which is not recognized on some supported compilers. # Clang and mingw are warning about Catch macros around perfectly normal boolean operations. -CXXFLAGS += -I../src -Wno-unused-variable -Wno-sign-compare -Wno-unknown-pragmas -Wno-parentheses -MMD -MP +CXXFLAGS += -I../src -Wno-unused-variable -Wno-sign-compare -Wno-unknown-pragmas -Wno-parentheses -MMD -MP CXXFLAGS += -Wall -Wextra \ -Wno-range-loop-analysis # TODO: Fix warnings instead of disabling @@ -79,12 +79,13 @@ clean: #Unconditionally create object directory on invocation. $(shell mkdir -p $(ODIR)) +# Adding ../tests/ so that the directory appears in __FILE__ for log messages $(ODIR)/%.o: %.cpp $(PCH_P) ifeq ($(VERBOSE),1) - $(CXX) $(CPPFLAGS) $(DEFINES) $(CXXFLAGS) $(subst main-pch,tests-pch,$(PCHFLAGS)) -c $< -o $@ + $(CXX) $(CPPFLAGS) $(DEFINES) $(CXXFLAGS) $(subst main-pch,tests-pch,$(PCHFLAGS)) -c ../tests/$< -o $@ else @echo $(@F) - @$(CXX) $(CPPFLAGS) $(DEFINES) $(CXXFLAGS) $(subst main-pch,tests-pch,$(PCHFLAGS)) -c $< -o $@ + @$(CXX) $(CPPFLAGS) $(DEFINES) $(CXXFLAGS) $(subst main-pch,tests-pch,$(PCHFLAGS)) -c ../tests/$< -o $@ endif .PHONY: clean check tests precompile_header diff --git a/tests/json_test.cpp b/tests/json_test.cpp index 71b19b7f7f61..caf72280245e 100644 --- a/tests/json_test.cpp +++ b/tests/json_test.cpp @@ -5,6 +5,8 @@ #include "bodypart.h" #include "json.h" +#include "cached_options.h" +#include "cata_utility.h" #include "string_formatter.h" #include "type_id.h" #include "colony.h" @@ -88,6 +90,8 @@ TEST_CASE( "translation_text_style_check", "[json][translation]" ) { // this test case is mainly for checking the caret position. // the text style check itself is tested in the lit test of clang-tidy. + restore_on_out_of_scope restore_error_log_format( error_log_format ); + error_log_format = error_log_format_t::human_readable; // string, ascii test_translation_text_style_check( @@ -171,6 +175,9 @@ TEST_CASE( "translation_text_style_check", "[json][translation]" ) TEST_CASE( "translation_text_style_check_error_recovery", "[json][translation]" ) { + restore_on_out_of_scope restore_error_log_format( error_log_format ); + error_log_format = error_log_format_t::human_readable; + SECTION( "string" ) { const std::string json = R"([)" "\n" @@ -270,6 +277,9 @@ static void test_string_error_throws_matches( Matcher &&matcher, const std::stri TEST_CASE( "jsonin_get_string", "[json]" ) { + restore_on_out_of_scope restore_error_log_format( error_log_format ); + error_log_format = error_log_format_t::human_readable; + // read plain text test_get_string( "foo", R"("foo")" ); // ignore starting spaces diff --git a/tests/test_main.cpp b/tests/test_main.cpp index e1754285d2aa..ab8debd0ea3e 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -280,8 +280,18 @@ int main( int argc, const char *argv[] ) std::string user_dir = extract_user_dir( arg_vec ); + std::string error_fmt = extract_argument( arg_vec, "--error-format=" ); + if( error_fmt == "github-action" ) { + error_log_format = error_log_format_t::github_action; + } else if( error_fmt == "human-readable" || error_fmt.empty() ) { + error_log_format = error_log_format_t::human_readable; + } else { + printf( "Unknown format %s", error_fmt.c_str() ); + return EXIT_FAILURE; + } + // Note: this must not be invoked before all DDA-specific flags are stripped from arg_vec! - int result = session.applyCommandLine( arg_vec.size(), &arg_vec[0] ); + int result = session.applyCommandLine( arg_vec.size(), arg_vec.data() ); if( result != 0 || session.configData().showHelp ) { cata_printf( "CataclysmDDA specific options:\n" ); cata_printf( " --mods= Loads the list of mods before executing tests.\n" ); @@ -289,6 +299,9 @@ int main( int argc, const char *argv[] ) cata_printf( " -D, --drop-world Don't save the world on test failure.\n" ); cata_printf( " --option_overrides=n:v[,…] Name-value pairs of game options for tests.\n" ); cata_printf( " (overrides config/options.json values)\n" ); + cata_printf( " --error-format= Format of error messages. Possible values are:\n" ); + cata_printf( " human-readable (default)\n" ); + cata_printf( " github-action\n" ); return result; }