ci: add problem matchers (cataclysmbnteam#2660)

* feat: display multi-line error messages with github action Co-authored-by: Qrox <qrox@sina.com> * Add debugmsg problem matcher This problem matcher is intended to highlight errors coming from hitting debugmsg in CI. Co-authored-by: John Bytheway <jbytheway@gmail.com> * 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 <jbytheway@gmail.com> * Add Catch2 problem matcher This is intended to highlight test failures in the Catch2 tests as GitHub annotations. Co-authored-by: John Bytheway <jbytheway@gmail.com> * ci: add msvc problem matchers uses https://github.com/marketplace/actions/msvc-problem-matcher * style: false positive for `\r` --------- Co-authored-by: Qrox <qrox@sina.com> Co-authored-by: John Bytheway <jbytheway@gmail.com>
Coolthulhu · Apr 18, 2023 · b32f622 · b32f622
1 parent 3389862
commit b32f622
Show file tree

Hide file tree

Showing 12 changed files with 222 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -17,6 +17,7 @@
 #include <map>
 #include <memory>
 #include <optional>
+#include <ostream>
 #include <set>
 #include <sstream>
 #include <sys/stat.h>
@@ -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
@@ -18,6 +18,7 @@
 #include <vector>
 
 #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<std::string> 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 : "<unknown source file>";
+    const std::string &name = escape_property( path ? normalize_relative_path( *path )
+                              : "<unknown source file>" );
     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,26 +1837,49 @@ 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 );
     size_t pos = tell();
     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
@@ -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
@@ -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<error_log_format_t> 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<error_log_format_t> 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<error_log_format_t> 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
@@ -280,15 +280,28 @@ 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=<mod1,mod2,…>         Loads the list of mods before executing tests.\n" );
         cata_printf( "  --user-dir=<dir>             Set user dir (where test world will be created).\n" );
         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=<value>       Format of error messages.  Possible values are:\n" );
+        cata_printf( "                                   human-readable (default)\n" );
+        cata_printf( "                                   github-action\n" );
         return result;
     }