From 8822ade49d03fa1bac1e3ae38693fe1818821329 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 14:50:09 +0200 Subject: [PATCH 01/15] [Tests] Register each test case with CTest instead of each test binary (#7399) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Tests] Register each test case with CTest instead of each test binary Registering a test cost four edits across a dozen CMakeLists files: add_test, add_executable, a conditional add_dependencies on gtest, and target_link_libraries. The conditional branch was the trap -- it is only needed when GTest was fetched and add_subdirectory'd, and omitting it yields undefined gtest symbols at link time -- so it was easy to leave out and impossible to notice until the link failed. Fold all four steps into cockatrice_add_test() and migrate every test directory onto it, with a cockatrice_add_manual_test() wrapper for the three benchmarks that are built but deliberately kept out of ctest. The calls now read as a declaration of what a test is rather than a transcript of how to register one. While rewriting the registrations, replace add_test(NAME x COMMAND x) with gtest_discover_tests(). Naming the binary as a single CTest entry meant a CI failure reported only which binary failed, and ctest -R Suite.Case could not select an individual case. Discovery raises the suite from 35 opaque entries to 469 real ones: ctest -R ThemeMigrationTest.IsIdempotent now runs exactly that case, and ctest -L server / -LE bench work off labels applied at registration. Give every test a 60s TIMEOUT. CTest's implicit default is 1500s, so one hung socket wait would stall CI for 25 minutes, and single_instance_manager_test already does real QLocalSocket waits. Dropping per-case granularity costs wall-clock time, since the suite becomes process-spawn bound rather than CPU bound: serial ctest went from 3.8s to 30.8s. Run it with -j in CI to recover it -- 7.6s at -j4, and 3.5s at -j8, both at or below the old serial time. The binaries are parallel-safe: the only test claiming a fixed resource scopes its local socket name to its own pid, so concurrent copies cannot collide. Also delete dummy_test, which asserted 1 == 1 and only proved the harness was wired up -- something the other 468 cases already establish. No test is added, removed, or changed by this commit; the 469 cases are the same ones the 35 binaries contained. * [Tests] Make QtTest available to test targets QtTest was not among the modules test targets could link, so the Qt model and signal contracts had no way to be verified from a test: QAbstractItemModelTester and QSignalSpy both live there. Add Test to _TEST_NEEDED in cmake/FindQtRuntime.cmake. That is the single place the union of test-only Qt modules is declared, so this is the only file that needs to change to make the module reachable from every test target. Nothing uses it yet. This lands the capability ahead of the model-layer tests that need it, so those can be added as tests alone rather than as test plus build-system plumbing. * [Tests] Correct the note on the oracle test compression defines add_definitions() populates the directory's COMPILE_DEFINITIONS property, which is consumed at generate time, so it reaches every target in the directory no matter where it is called relative to add_executable(). The previous note claimed the flags stayed confined to the benchmark because the find_package block came after it; oracle_importer_test, created above that block, carries both defines too. Verified harmless rather than a live bug: HAS_ZLIB and HAS_LZMA gate code only in oracle/src/pages.cpp and oracle/src/pagetemplates.cpp, and no test target compiles either. The ordering and the leaked defines are both pre-existing, so this only corrects the explanation. * [Tests] Carry one CTest label per test instead of several gtest_discover_tests() can hand CTest exactly one label. GoogleTestAddTests.cmake accumulates its generated script with `foreach(arg ${ARGN})`, so a label list reaches the CTestTestfile as bare `LABELS ci server`, and CTest reads only the first value of a property and drops the rest without a word. The `ci` label that is appended to every registered test was therefore absent from every test that also asked for `server` or `bench`: `ctest -L ci` reported 400 of 469 cases, and `ctest --print-labels` never listed a second label anywhere in the build. Reduce each test to exactly one of `ci`, `server`, or `bench`, which is all the selections need. The three now partition the suite exactly, 400 + 64 + 5 = 469, and `ctest -LE bench` still means everything but benchmarks. A second label is now a configure-time FATAL_ERROR rather than a line that quietly means something else. Anything orthogonal to a test's kind belongs in a helper flag beside the label, so it composes with the group instead of competing for the single slot. Also raise DISCOVERY_TIMEOUT off GoogleTestAddTests' 5s default. A plain debug binary answers --gtest_list_tests in 0.04s, but that budget is thin once several binaries discover concurrently behind a -j build, and being over it fails at the *link* step with a bare "Process terminated due to timeout" that points nowhere near the cause. * [Tests] Link gtest_main on the fetched-gtest path The downloaded-gtest branch set GTEST_BOTH_LIBRARIES to `gtest` alone, while FindGTest sets it to both halves. Every test target links that list rather than picking an entry point, so on this path nothing provides main() and any test source that does not define its own fails at link with `undefined reference to main` pointing at crt1.o, naming neither gtest nor the line responsible. That stayed latent while every test carried its own four-line main(). A test that omits it now fails, and the error points at the C runtime rather than at gtest, which is a long way from the cause. Restoring both halves makes the two paths agree and drops the boilerplate requirement on new test sources. The 18 sources that still define main() keep linking: gtest_main.cc defines nothing but main, so the linker leaves its object in the archive unused rather than reporting a duplicate. * [Build] Run the test suite under ASan and UBSan (#7400) * [Build] Add an ENABLE_SANITIZERS option for ASan and UBSan Adds -DENABLE_SANITIZERS, off by default, building with AddressSanitizer and UndefinedBehaviorSanitizer. The bulk of the work is in the three ways the obvious implementation silently does the wrong thing. Probe the flags on the link step, not just the compile step. check_cxx_compiler_flag() only compiles, so every sanitizer flag comes back unsupported, because linking an ASan object without -fsanitize=address leaves __asan_init undefined. The result is not a build failure but a build that appears instrumented and is not, which is worse. Use check_cxx_source_compiles() with CMAKE_REQUIRED_LINK_OPTIONS instead, and treat an unsupported sanitizer pair as fatal since the cosmetic flags are allowed to degrade. Opt vendored sources out per source, through a cockatrice_disable_sanitizers_for() helper rather than a list of set_source_files_properties() calls at the root. The helper has to exist because CMake resolves a source-file property against the directory scope its target was declared in: setting the property from the root with absolute paths is a no-op, silently, which is why the call sites sit next to the add_library and add_executable that pull the vendored code in. SFMT, minizip, the LZMA SDK and Qxt SMTP are excluded. oracle/src/qt-json is in the history but not in the tree, removed in #7214. Also mark those sources SKIP_PRECOMPILE_HEADERS. The PCH is built with the sanitizer flags, so it defines __SANITIZE_ADDRESS__; a translation unit compiled with -fno-sanitize=all does not and GCC then refuses the PCH, which is a fatal -Werror=invalid-pch in the servatrice and oracle builds. -fno-sanitize=vptr is not optional. Qt constructs QObject subclasses into raw storage and writes the vptr afterwards, so the vptr check reports on essentially every QObject instantiation, and it has no value here because nothing outside moc-generated code inherits QObject. Verified on the resulting build: our own translation units carry ASan and UBSan calls, SFMT.o carries neither, and the full suite of 464 cases passes clean under ASAN_OPTIONS=detect_leaks=0:abort_on_error=1 with UBSAN_OPTIONS=halt_on_error=0. The test suite needs two adjustments to make this option usable at all, both of which land here because they are consequences of COCKATRICE_SANITIZERS existing rather than independent changes. DISCOVERY_TIMEOUT rises from 60s to 300s under sanitizers. An instrumented binary takes 2.8s to answer --gtest_list_tests against 0.04s uninstrumented, and discovery runs several binaries at once behind a -j build. One target is excluded via a NO_SANITIZE flag, server_developer_role_test. Reaching google::protobuf::TextFormat::Printer's destructor aborts on an absl assertion inside system protobuf 33.4 whenever the consumer translation unit is built without NDEBUG, which a Debug sanitizer build is. This reproduces with no Cockatrice code at all, just "TextFormat::Printer p; p.PrintToString(msg, &out);" built with -fsanitize=address, so it is upstream rather than ours. It is a flag and not a fourth label because "skip this in the sanitizer leg" has to compose with a test's group rather than compete for the single label slot, and because a flag excludes by not registering the cases at all, which cannot rot the way a label silently can. The target still builds and still runs on the legs without sanitizers. * [CI] Run the suite once under ASan and UBSan Adds --sanitize to .ci/compile.sh, mirroring --test, and turns it on for one entry of the existing Linux Docker matrix: Ubuntu 26.04, which already had a test-enabled leg. That reuses the image, the ccache layer and the existing step shape, so the CI diff is a matrix key plus the argument plumbing. Debian 13 and Fedora 44 keep covering the uninstrumented suite, so nothing is lost by instrumenting one leg. Sanitizers stay off the release and package legs, which build with --release for packaging: instrumenting those would change the shipped binaries' optimization profile and build time for no benefit. macOS and Windows stay uninstrumented too, since ASan is unreliable against the Qt build there and MSVC has no equivalent without /fsanitize=address. --sanitize does not imply --test, so an instrumented build can be produced locally without paying for ctest. When both are given, ctest runs with detect_leaks=0 because Qt and the sanitizer runtimes both leak at exit by design and LeakSanitizer's default buries real findings under hundreds of process-teardown reports, and with UBSan halt_on_error=0 so one run collects many findings instead of stopping at the first. ASan keeps abort_on_error=1, since a bad access leaves the process unusable and continuing only produces a second, more confusing report from the wreckage. * [Build] Fix sanitizer flag leak into release package and make UBSan fail CI - compile.sh always passes -DENABLE_SANITIZERS explicitly so the release-package reconfigure in the same build dir cannot inherit ON from the sanitized debug step (which shipped libasan/libubsan DEB deps). - Sanitizers.cmake resets COCKATRICE_SANITIZERS to OFF when the option is off (CACHE INTERNAL otherwise sticks forever, keeping NO_SANITIZE skips and the 300s discovery timeout). - UBSan now fails the job: -fno-sanitize-recover=undefined at compile time plus UBSAN_OPTIONS halt_on_error=1 (recover mode exits 0, so the old halt_on_error=0 could never fail). --------- Co-authored-by: Lukas Brübach --------- Co-authored-by: Lukas Brübach --- .ci/compile.sh | 33 +- .github/workflows/desktop-build.yml | 12 +- CMakeLists.txt | 6 + cmake/FindQtRuntime.cmake | 4 +- cmake/Sanitizers.cmake | 109 ++++ libcockatrice_rng/CMakeLists.txt | 3 + oracle/CMakeLists.txt | 4 + servatrice/CMakeLists.txt | 5 + tests/CMakeLists.txt | 496 +++++++++++------- tests/card_zone_algorithms/CMakeLists.txt | 17 +- tests/carddatabase/CMakeLists.txt | 73 +-- tests/deck_list_model/CMakeLists.txt | 34 +- tests/deck_list_zones/CMakeLists.txt | 11 +- tests/dummy_test.cpp | 20 - tests/loading_from_clipboard/CMakeLists.txt | 19 +- tests/movecard_tests/CMakeLists.txt | 23 +- tests/oracle/CMakeLists.txt | 67 ++- tests/picture_loader_benchmark/CMakeLists.txt | 22 +- tests/settings/CMakeLists.txt | 34 +- tests/theme_migration/CMakeLists.txt | 21 +- 20 files changed, 611 insertions(+), 402 deletions(-) create mode 100644 cmake/Sanitizers.cmake delete mode 100644 tests/dummy_test.cpp diff --git a/.ci/compile.sh b/.ci/compile.sh index 823a31898..de4fac004 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -8,6 +8,7 @@ # --suffix renames package with this suffix, requires arg # --server compiles servatrice # --test runs tests +# --sanitize builds with AddressSanitizer and UndefinedBehaviorSanitizer, gcc/clang only # --debug or --release sets the build type ie CMAKE_BUILD_TYPE # --ccache [] uses ccache and shows stats, optionally provide size # --evict-ccache runs ccache eviction based on given age after build @@ -57,6 +58,10 @@ while [[ $# != 0 ]]; do MAKE_TEST=1 shift ;; + '--sanitize') + MAKE_SANITIZE=1 + shift + ;; '--debug') BUILDTYPE="Debug" shift @@ -147,6 +152,14 @@ fi if [[ $MAKE_TEST ]]; then flags+=("-DTEST=1") fi +if [[ $MAKE_SANITIZE ]]; then + flags+=("-DENABLE_SANITIZERS=ON") +else + # The release-package step re-runs cmake in the same build dir, so an + # ENABLE_SANITIZERS=ON left over from the debug step would otherwise leak + # into the shipped binaries (libasan/libubsan Depends in the DEB). + flags+=("-DENABLE_SANITIZERS=OFF") +fi if [[ $USE_CCACHE ]]; then flags+=("-DUSE_CCACHE=1") # PCH-aware caching is required or ccache refuses to cache any TU that @@ -306,7 +319,25 @@ fi if [[ $MAKE_TEST ]]; then echo "::group::Run tests" - ctest -C "$BUILDTYPE" --output-on-failure + if [[ $MAKE_SANITIZE ]]; then + # detect_leaks=0: Qt and the sanitizers' own runtimes both leak at exit by design, and + # LeakSanitizer's default is aggressive enough to bury our real findings under hundreds + # of process-teardown reports. Malloc/free misuse and use-after-free are what ASan is for + # and they stay fully on. + # abort_on_error=1: a bad access leaves the process in an unusable state, so continuing + # only produces a second, more confusing report from the wreckage. + # UBSAN halt_on_error=1: without it UBSan prints "runtime error: ..." but the + # process exits 0 (recover mode), so gtest/ctest report a pass and the job + # stays green. Combined with -fno-sanitize-recover=undefined at compile time, + # the first finding aborts the test process with a non-zero exit. + export ASAN_OPTIONS="detect_leaks=0:abort_on_error=1:print_stacktrace=1:symbolize=1" + export UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:symbolize=1" + fi + # Every TEST() is its own CTest entry, so the suite is process-spawn bound rather than CPU + # bound and -j recovers nearly all of the per-process overhead. The test binaries are safe to + # run concurrently: the only one that claims a fixed resource is single_instance_manager_test, + # which already scopes its local socket name to its own pid so parallel copies cannot collide. + ctest -C "$BUILDTYPE" --output-on-failure -j "${CTEST_JOBS:-4}" echo "::endgroup::" fi diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index 17c42b07d..57c2993f6 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -144,6 +144,11 @@ jobs: package: DEB + # The one leg that runs the suite under AddressSanitizer and + # UndefinedBehaviorSanitizer. Debian 13 and Fedora 44 keep covering the + # uninstrumented suite, so nothing is lost by instrumenting this one. + sanitize: yes + name: ${{ matrix.distro }} ${{ matrix.version }} needs: configure runs-on: ubuntu-latest # https://github.com/actions/runner-images @@ -174,6 +179,8 @@ jobs: - name: "Build debug and test" if: matrix.test != 'skip' shell: bash + env: + sanitize: '${{ matrix.sanitize }}' run: | source .ci/docker.sh args=() @@ -181,7 +188,10 @@ jobs: args+=(--ccache "$CCACHE_SIZE") args+=(--cmake-generator "$CMAKE_GENERATOR") - RUN --server --debug --test "${args[@]}" + extra=() + [[ $sanitize == yes ]] && extra+=(--sanitize) + + RUN --server --debug --test "${extra[@]}" "${args[@]}" - name: "Build release package" id: build diff --git a/CMakeLists.txt b/CMakeLists.txt index f51c636eb..ef8866e3a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,6 +22,8 @@ option(WITH_ORACLE "Build Cockatrice card database tool (Oracle)" ON) option(WITH_SERVER "Build Cockatrice server (Servatrice)" OFF) # Compile tests option(TEST "Build tests" OFF) +# Build with AddressSanitizer and UndefinedBehaviorSanitizer +option(ENABLE_SANITIZERS "Build with AddressSanitizer and UndefinedBehaviorSanitizer" OFF) # Use vcpkg regardless of OS option(USE_VCPKG "Use vcpkg regardless of OS" OFF) @@ -211,6 +213,10 @@ else() set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -pipe") endif() +# Always included, because it defines cockatrice_disable_sanitizers_for() for the vendored +# trees. It only touches the global flags when ENABLE_SANITIZERS is on. +include(Sanitizers) + # GNU systems need to define the Mersenne exponent for the RNG to compile w/o warning if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") add_definitions("-DSFMT_MEXP=19937") diff --git a/cmake/FindQtRuntime.cmake b/cmake/FindQtRuntime.cmake index ad52352de..2d74aca4d 100644 --- a/cmake/FindQtRuntime.cmake +++ b/cmake/FindQtRuntime.cmake @@ -40,7 +40,9 @@ endif() if(TEST) # Union of Qt modules required across all test targets (independent of application targets). # When adding a new test that needs additional Qt modules, add them here rather than in the test's CMakeLists.txt. - set(_TEST_NEEDED Concurrent Network Svg Widgets) + # Test is QtTest: it brings QAbstractItemModelTester and QSignalSpy, which are how the Qt model + # and signal contracts get verified without standing up a GUI. + set(_TEST_NEEDED Concurrent Network Svg Widgets Test) endif() set(REQUIRED_QT_COMPONENTS ${REQUIRED_QT_COMPONENTS} ${_SERVATRICE_NEEDED} ${_COCKATRICE_NEEDED} ${_ORACLE_NEEDED} diff --git a/cmake/Sanitizers.cmake b/cmake/Sanitizers.cmake new file mode 100644 index 000000000..2e5d794de --- /dev/null +++ b/cmake/Sanitizers.cmake @@ -0,0 +1,109 @@ +# AddressSanitizer + UndefinedBehaviorSanitizer opt-in, plus per-source opt-out +# Input: ENABLE_SANITIZERS +# Defines: cockatrice_disable_sanitizers_for +# +# Included once from the root CMakeLists.txt, immediately after the per-compiler flag +# block, so add_compile_options() reaches every library and executable defined below. + +include(CheckCXXSourceCompiles) + +# Opt a set of vendored sources out of instrumentation. +# +# cockatrice_disable_sanitizers_for( ...) +# +# Sanitizer findings inside vendored code are not actionable for us, and letting them +# through buries our own bugs in noise until everyone learns to skip the job. +# +# The sources must be named relative to the directory that compiles them. CMake resolves a +# source-file property against the directory scope the target was declared in, so calling +# this from the root with absolute paths silently does nothing. That is why the call sites +# live next to the add_library/add_executable that pull the vendored code in. +# +# SKIP_PRECOMPILE_HEADERS is not optional. The precompiled header is built with the +# sanitizer flags, so it defines __SANITIZE_ADDRESS__; a translation unit compiled with +# -fno-sanitize=all does not, and GCC then refuses the PCH with +# "not used because __SANITIZE_ADDRESS__ not defined", which is a fatal -Werror=invalid-pch +# in the servatrice and oracle builds. +function(cockatrice_disable_sanitizers_for) + if(NOT ENABLE_SANITIZERS) + return() + endif() + set_source_files_properties(${ARGN} PROPERTIES COMPILE_OPTIONS "-fno-sanitize=all" SKIP_PRECOMPILE_HEADERS ON) +endfunction() + +if(ENABLE_SANITIZERS) + # Probe a flag the way it will actually be used: on the compile step AND the link step. + # check_cxx_compiler_flag() only probes the compile step, which makes every sanitizer + # flag look unsupported because linking an ASan object without -fsanitize=address leaves + # __asan_init undefined. That is a silent misconfiguration rather than an error, so the + # probe has to link. + function(cockatrice_probe_flag FLAG VARNAME) + set(CMAKE_REQUIRED_FLAGS "${FLAG}") + set(CMAKE_REQUIRED_LINK_OPTIONS "${FLAG}") + check_cxx_source_compiles("int main() { return 0; }" ${VARNAME}) + unset(CMAKE_REQUIRED_FLAGS) + unset(CMAKE_REQUIRED_LINK_OPTIONS) + endfunction() + + set(SANITIZER_FLAGS + -fsanitize=address,undefined + -fno-omit-frame-pointer + # UBSan defaults to recover mode: it prints "runtime error: ..." but the + # process exits 0, so gtest/ctest report a pass and CI stays green. + # -fno-sanitize-recover makes the first finding abort instead. This is + # probed like the other cosmetic flags, but CI also sets + # UBSAN_OPTIONS=halt_on_error=1 as the runtime-side guarantee. + -fno-sanitize-recover=undefined + # Qt builds QObject subclasses into raw storage and writes the vptr afterwards, which + # the vptr check reports on essentially every QObject instantiation. The check has no + # value for us because nothing outside moc-generated code inherits QObject, so it is + # off outright rather than suppressed piecemeal. + -fno-sanitize=vptr + ) + + # MSVC has no ASan/UBSan equivalent, and clang-cl would need /fsanitize=address plus a + # matching runtime link, which is a separate piece of work. + if(MSVC) + message(FATAL_ERROR "ENABLE_SANITIZERS is not supported with MSVC") + endif() + + # Probe rather than assume, so an older toolchain gets the sanitizers it can support + # instead of failing the build on a flag it does not recognize. + set(SANITIZER_SUPPORTED_FLAGS) + foreach(FLAG ${SANITIZER_FLAGS}) + string(MAKE_C_IDENTIFIER "COCKATRICE_HAS${FLAG}" _FLAG_VAR) + cockatrice_probe_flag("${FLAG}" ${_FLAG_VAR}) + if(${_FLAG_VAR}) + list(APPEND SANITIZER_SUPPORTED_FLAGS ${FLAG}) + else() + message(STATUS "Sanitizers: ${FLAG} unsupported by ${CMAKE_CXX_COMPILER_ID}, skipping") + endif() + endforeach() + + # The whole point of the option is the sanitizers, so an unsupported pair is fatal even + # though the cosmetic flags above are allowed to degrade. + if(NOT "-fsanitize=address,undefined" IN_LIST SANITIZER_SUPPORTED_FLAGS) + message(FATAL_ERROR "ENABLE_SANITIZERS requires -fsanitize=address,undefined, which " + "${CMAKE_CXX_COMPILER_ID} does not accept" + ) + endif() + + add_compile_options(${SANITIZER_SUPPORTED_FLAGS}) + add_link_options(-fsanitize=address,undefined) + + # Recorded so the test tree can label its entries and a plain `make` at the same time + # cannot quietly disagree with what CI ran. + set(COCKATRICE_SANITIZERS + ON + CACHE INTERNAL "AddressSanitizer and UBSan are enabled" + ) +else() + # ENABLE_SANITIZERS is OFF, but COCKATRICE_SANITIZERS is CACHE INTERNAL, so a + # reused build dir (e.g. CI's release-package step reconfiguring the same dir + # after the sanitized debug step) would otherwise keep skipping NO_SANITIZE + # tests and keep the 300s discovery timeout. Reset it explicitly. + set(COCKATRICE_SANITIZERS + OFF + CACHE INTERNAL "AddressSanitizer and UBSan are enabled" FORCE + ) +endif() diff --git a/libcockatrice_rng/CMakeLists.txt b/libcockatrice_rng/CMakeLists.txt index b988c9d2f..16beffcfe 100644 --- a/libcockatrice_rng/CMakeLists.txt +++ b/libcockatrice_rng/CMakeLists.txt @@ -11,6 +11,9 @@ add_library( libcockatrice/rng/sfmt/SFMT.c ) +# SFMT is vendored and its own headers trip UBSan's alignment and vptr checks. +cockatrice_disable_sanitizers_for(libcockatrice/rng/sfmt/SFMT.c) + target_include_directories(libcockatrice_rng PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(libcockatrice_rng PUBLIC ${QT_CORE_MODULE}) diff --git a/oracle/CMakeLists.txt b/oracle/CMakeLists.txt index 0003b77ac..9b6863553 100644 --- a/oracle/CMakeLists.txt +++ b/oracle/CMakeLists.txt @@ -79,6 +79,8 @@ if(ZLIB_FOUND) include_directories(${ZLIB_INCLUDE_DIRS}) add_definitions("-DHAS_ZLIB") list(APPEND oracle_SOURCES src/zip/unzip.cpp src/zip/zipglobal.cpp) + # minizip and zlib are vendored. + cockatrice_disable_sanitizers_for(src/zip/unzip.cpp src/zip/zipglobal.cpp) else() message(STATUS "Oracle: zlib not found; ZIP support disabled") endif() @@ -89,6 +91,8 @@ if(LIBLZMA_FOUND) include_directories(${LIBLZMA_INCLUDE_DIRS}) add_definitions("-DHAS_LZMA") list(APPEND oracle_SOURCES src/lzma/decompress.cpp) + # The LZMA SDK is vendored. + cockatrice_disable_sanitizers_for(src/lzma/decompress.cpp) else() message(STATUS "Oracle: LibLZMA not found; xz support disabled") endif() diff --git a/servatrice/CMakeLists.txt b/servatrice/CMakeLists.txt index 21f71a908..b176d8009 100644 --- a/servatrice/CMakeLists.txt +++ b/servatrice/CMakeLists.txt @@ -27,6 +27,11 @@ set(servatrice_SOURCES set(servatrice_RESOURCES servatrice.qrc) +# Qxt SMTP is vendored. +cockatrice_disable_sanitizers_for( + src/smtp/qxthmac.cpp src/smtp/qxtmailattachment.cpp src/smtp/qxtmailmessage.cpp src/smtp/qxtsmtp.cpp +) + if(WIN32) set(servatrice_SOURCES ${servatrice_SOURCES} servatrice.rc) endif(WIN32) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2c288c6ce..c48e6c957 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -2,109 +2,10 @@ # If a new test needs additional Qt modules, add them there — not in individual test CMakeLists.txt files. enable_testing() -add_test(NAME dummy_test COMMAND dummy_test) -add_test(NAME expression_test COMMAND expression_test) -add_test(NAME clamped_arithmetic_test COMMAND clamped_arithmetic_test) -add_test(NAME test_age_formatting COMMAND test_age_formatting) -add_test(NAME password_hash_test COMMAND password_hash_test) -add_test(NAME playmat_resolver_test COMMAND playmat_resolver_test) -add_test(NAME server_card_counter_test COMMAND server_card_counter_test) -add_test(NAME server_counter_test COMMAND server_counter_test) -add_test(NAME server_rate_limiter_test COMMAND server_rate_limiter_test) -add_test(NAME server_developer_role_test COMMAND server_developer_role_test) -add_test(NAME server_game_join_test COMMAND server_game_join_test) -add_test(NAME warning_categories_test COMMAND warning_categories_test) -# The translation loader test compiles a .qm fixture, which needs lrelease from Qt Linguist. -if(TARGET Qt6::lrelease) - add_test(NAME translation_loader_test COMMAND translation_loader_test) -endif() -add_test(NAME report_categories_test COMMAND report_categories_test) -add_test(NAME lag_monitor_test COMMAND lag_monitor_test) -add_test(NAME latency_tracker_test COMMAND latency_tracker_test) -add_test(NAME metrics_registry_test COMMAND metrics_registry_test) -add_test(NAME card_update_progress_test COMMAND card_update_progress_test) -add_test(NAME loader_local_matching_test COMMAND loader_local_matching_test) - -add_test(NAME deck_hash_performance_test COMMAND deck_hash_performance_test) -set_tests_properties(deck_hash_performance_test PROPERTIES TIMEOUT 15) +include(GoogleTest) # Find GTest -add_executable(dummy_test dummy_test.cpp) -add_executable(expression_test expression_test.cpp) -add_executable(clamped_arithmetic_test clamped_arithmetic_test.cpp) -add_executable(test_age_formatting test_age_formatting.cpp) -add_executable(password_hash_test password_hash_test.cpp) -add_executable(playmat_resolver_test playmat_resolver_test.cpp) -add_executable(deck_hash_performance_test deck_hash_performance_test.cpp) -add_executable(server_card_counter_test server_card_counter_test.cpp) -add_executable(server_counter_test server_counter_test.cpp) -add_executable(server_rate_limiter_test server_rate_limiter_test.cpp) -add_executable(server_developer_role_test server_developer_role_test.cpp) -add_executable(server_game_join_test server_game_join_test.cpp) -add_executable(warning_categories_test warning_categories_test.cpp) -if(TARGET Qt6::lrelease) - add_executable(translation_loader_test translation_loader_test.cpp) - - # QTranslator only accepts real .qm files, so the translation loader test gets one compiled from its - # own .ts fixture rather than a stub. - set(TRANSLATION_LOADER_TEST_QM "${CMAKE_CURRENT_BINARY_DIR}/translation_loader_test_de.qm") - add_custom_command( - OUTPUT "${TRANSLATION_LOADER_TEST_QM}" - COMMAND Qt6::lrelease "${CMAKE_CURRENT_SOURCE_DIR}/translation_loader_test.ts" -qm "${TRANSLATION_LOADER_TEST_QM}" - DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/translation_loader_test.ts" - COMMENT "Compiling translation_loader_test.ts" - ) - add_custom_target(translation_loader_test_qm DEPENDS "${TRANSLATION_LOADER_TEST_QM}") - add_dependencies(translation_loader_test translation_loader_test_qm) - target_compile_definitions(translation_loader_test PRIVATE TRANSLATION_LOADER_TEST_DIR="${CMAKE_CURRENT_BINARY_DIR}") - - # A multi-config generator (MSBuild, Xcode) puts the executable in a per-config subdirectory while - # the .qm stays in the current binary directory, so copy the fixture next to the binary: that is the - # layout ABuildTreeFindsTheTranslationsNextToTheExecutable is about, and the only one it finds on - # those generators. - add_custom_command( - TARGET translation_loader_test - POST_BUILD - COMMAND ${CMAKE_COMMAND} -E copy_if_different "${TRANSLATION_LOADER_TEST_QM}" - "$/" - VERBATIM - ) -endif() -add_executable(report_categories_test report_categories_test.cpp) -add_executable(lag_monitor_test ${CMAKE_SOURCE_DIR}/cockatrice/src/client/lag_monitor.cpp lag_monitor_test.cpp) -target_include_directories(lag_monitor_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) -add_executable(latency_tracker_test latency_tracker_test.cpp) -add_executable(metrics_registry_test ../servatrice/src/metrics_registry.cpp metrics_registry_test.cpp) -add_executable( - card_update_progress_test ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_database_update/card_update_progress.cpp - card_update_progress_test.cpp -) -target_include_directories(card_update_progress_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) -add_executable( - loader_local_matching_test - ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp - ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/cache_settings.cpp - ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/card_counter_settings.cpp - ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/shortcuts_settings.cpp - ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/client/release_channel.cpp - ${VERSION_STRING_CPP} - loader_local_matching_test.cpp -) -target_include_directories(loader_local_matching_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) - -# Guard must match the condition for src/single_instance_manager.cpp in cockatrice/CMakeLists.txt. -if(WITH_CLIENT) - add_executable( - single_instance_manager_test ${CMAKE_SOURCE_DIR}/cockatrice/src/single_instance_manager.cpp - single_instance_manager_test.cpp - ) - target_include_directories(single_instance_manager_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) - target_include_directories(single_instance_manager_test PRIVATE ${CMAKE_SOURCE_DIR}/libcockatrice_utility) - - add_test(NAME single_instance_manager_test COMMAND single_instance_manager_test) -endif() - find_package(GTest) if(NOT GTEST_FOUND) @@ -129,93 +30,330 @@ if(NOT GTEST_FOUND) target_include_directories(gtest INTERFACE "$") set(GTEST_INCLUDE_DIRS "${CMAKE_BINARY_DIR}/gtest-src/include") - set(GTEST_BOTH_LIBRARIES gtest) - add_dependencies(dummy_test gtest) - add_dependencies(expression_test gtest) - add_dependencies(clamped_arithmetic_test gtest) - add_dependencies(test_age_formatting gtest) - add_dependencies(password_hash_test gtest) - add_dependencies(playmat_resolver_test gtest) - add_dependencies(deck_hash_performance_test gtest) - add_dependencies(server_card_counter_test gtest) - add_dependencies(server_counter_test gtest) - add_dependencies(server_rate_limiter_test gtest) - add_dependencies(server_developer_role_test gtest) - add_dependencies(server_game_join_test gtest) - add_dependencies(warning_categories_test gtest) - if(TARGET translation_loader_test) - add_dependencies(translation_loader_test gtest) - endif() - add_dependencies(report_categories_test gtest) - add_dependencies(lag_monitor_test gtest) - add_dependencies(latency_tracker_test gtest) - add_dependencies(metrics_registry_test gtest) - add_dependencies(card_update_progress_test gtest) - add_dependencies(loader_local_matching_test gtest) - add_dependencies(single_instance_manager_test gtest) + # gtest_main is not optional here. FindGTest sets GTEST_BOTH_LIBRARIES to both halves, so this + # has to match it or the two paths disagree about what provides main(). A test source that + # defines its own main() gets away with the omission; one that does not fails at link with + # `undefined reference to main` pointing at crt1.o, which names neither gtest nor this line. + set(GTEST_BOTH_LIBRARIES gtest gtest_main) endif() +# Directory-scoped on purpose: the test subdirectories below add targets that include the +# fetched gtest headers and do not repeat this path themselves. include_directories(${GTEST_INCLUDE_DIRS}) -target_link_libraries(dummy_test Threads::Threads ${GTEST_BOTH_LIBRARIES}) -target_link_libraries(expression_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) -target_link_libraries( - clamped_arithmetic_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -target_link_libraries( - test_age_formatting libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -target_link_libraries( - password_hash_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -target_link_libraries( - playmat_resolver_test libcockatrice_deck_list libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} - ${TEST_QT_MODULES} -) -target_link_libraries( - deck_hash_performance_test libcockatrice_deck_list libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} - ${TEST_QT_MODULES} -) -target_link_libraries( - server_card_counter_test libcockatrice_network Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -target_link_libraries( - server_counter_test libcockatrice_network Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -target_link_libraries( - server_rate_limiter_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -target_link_libraries( - server_developer_role_test libcockatrice_network libcockatrice_rng Threads::Threads ${GTEST_BOTH_LIBRARIES} - ${TEST_QT_MODULES} -) -target_link_libraries( - server_game_join_test libcockatrice_network_server_remote libcockatrice_rng Threads::Threads ${GTEST_BOTH_LIBRARIES} - ${TEST_QT_MODULES} -) -target_link_libraries( - warning_categories_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -if(TARGET translation_loader_test) + +# --- Test registration ---------------------------------------------------------- +# +# Every test target needs the same four steps: build the executable, depend on gtest when it +# was fetched from source, link the shared test libraries, and hand the cases to CTest. The +# `if(NOT GTEST_FOUND)` branch is the easy one to forget — omit it and the link fails with +# undefined gtest symbols — so it lives here once instead of being repeated per test. +# +# This also replaces `add_test(NAME x COMMAND x)`. Naming the binary as a single CTest entry +# hides which case failed and makes `ctest -R Suite.Case` impossible; discovery exposes each +# TEST() as its own CTest entry instead. +# +# Labels: exactly one per test, because gtest_discover_tests() cannot carry more than one. +# GoogleTestAddTests.cmake accumulates the generated script with `foreach(arg ${ARGN})`, so a +# label list serialises as bare `LABELS ci server` in the CTestTestfile, and CTest reads only +# the first value of a property and silently discards the rest. A label therefore classifies a +# test into exactly one group, and the three groups cover every selection worth making: +# +# ci the default hermetic set, so `ctest -LE bench` runs everything but benchmarks +# server servatrice-facing tests, so `ctest -L server` works +# bench the slow suites, kept out of the default run +# +# A second label is a configure-time FATAL_ERROR rather than a line CTest quietly reads as one. +# Anything orthogonal to a test's kind belongs in a helper flag beside the label, so it +# composes with the group instead of competing for the single slot. NO_SANITIZE is such a flag. +# +# TIMEOUT is bounded per test because CTest's implicit default is 1500s: one hung socket wait +# would otherwise stall CI for 25 minutes. +# +# EXTRA_LIBS is appended after the shared libraries so a caller adding a static archive (zlib, +# lzma) keeps it last, which is where the linker needs it. + +function(cockatrice_add_test) + cmake_parse_arguments( + PARSE_ARGV 0 ARG "MANUAL;NO_SANITIZE" "NAME" "SOURCES;LIBS;LABELS;DEFINITIONS;INCLUDE_DIRS;EXTRA_LIBS" + ) + + if(NOT ARG_NAME OR NOT ARG_SOURCES) + message(FATAL_ERROR "cockatrice_add_test requires at least NAME and SOURCES") + endif() + + add_executable(${ARG_NAME} ${ARG_SOURCES}) + + if(ARG_INCLUDE_DIRS) + target_include_directories(${ARG_NAME} PRIVATE ${ARG_INCLUDE_DIRS}) + endif() + + if(ARG_DEFINITIONS) + target_compile_definitions(${ARG_NAME} PRIVATE ${ARG_DEFINITIONS}) + endif() + target_link_libraries( - translation_loader_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} + ${ARG_NAME} ${ARG_LIBS} Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} ${ARG_EXTRA_LIBS} + ) + + # Only needed when gtest was downloaded and add_subdirectory'd above. An imported gtest + # target already carries its own dependency, so this must stay conditional. + if(NOT GTEST_FOUND) + add_dependencies(${ARG_NAME} gtest gtest_main) + endif() + + # MANUAL targets are built but deliberately kept out of ctest: benchmarks that need a card + # database, hit the network, or measure RAM. Leaving them unregistered keeps CI hermetic + # and fast. They still link gtest so they can use TEST() without a second code path. + if(ARG_MANUAL) + return() + endif() + + # NO_SANITIZE marks a target the sanitizer leg cannot run. Excluded by not handing its cases + # to CTest at all, rather than by a label, so the exclusion cannot rot into a line CTest + # quietly reads as something else. The target still builds, and still runs on the other legs. + if(ARG_NO_SANITIZE AND COCKATRICE_SANITIZERS) + return() + endif() + + # One label only, per the note above. Rejecting a second loudly beats emitting a + # CTestTestfile line that CTest will quietly read as one label. + list(LENGTH ARG_LABELS _label_count) + if(_label_count GREATER 1) + message(FATAL_ERROR "${ARG_NAME}: pass at most one label, got '${ARG_LABELS}'") + endif() + if(NOT ARG_LABELS) + set(ARG_LABELS ci) + endif() + + # GoogleTestAddTests defaults DISCOVERY_TIMEOUT to 5s, which is ample for a plain debug + # binary (--gtest_list_tests costs 0.04s) but not for an instrumented one (~2.8s cold, and + # discovery runs several binaries concurrently behind a -j build). Left at the default, a + # sanitizer build fails at the *link* step with a bare "Process terminated due to timeout" + # and no hint that discovery, not the test, is what is slow. + if(COCKATRICE_SANITIZERS) + set(_discovery_timeout 300) + else() + set(_discovery_timeout 60) + endif() + + gtest_discover_tests( + ${ARG_NAME} DISCOVERY_MODE POST_BUILD DISCOVERY_TIMEOUT ${_discovery_timeout} + PROPERTIES LABELS "${ARG_LABELS}" + TIMEOUT 60 + ) +endfunction() + +# Same wiring for targets that are built but deliberately kept out of ctest: benchmarks that +# need a card database, hit the network, or measure RAM. A separate name rather than a MANUAL +# flag spelled out at each call site, so "this is not in the CI suite" is visible in the CMake +# file. Note the unquoted expansions: an unset multi-value keyword expands to zero arguments, +# which is what lets this forward a caller's keywords verbatim without tripping the FATAL_ERROR +# guard above on an empty list. +function(cockatrice_add_manual_test) + cmake_parse_arguments(PARSE_ARGV 0 ARG "" "NAME" "SOURCES;LIBS;DEFINITIONS;INCLUDE_DIRS;EXTRA_LIBS") + + cockatrice_add_test( + NAME + ${ARG_NAME} + SOURCES + ${ARG_SOURCES} + LIBS + ${ARG_LIBS} + DEFINITIONS + ${ARG_DEFINITIONS} + INCLUDE_DIRS + ${ARG_INCLUDE_DIRS} + EXTRA_LIBS + ${ARG_EXTRA_LIBS} + MANUAL + ) +endfunction() + +# --- Unit tests ------------------------------------------------------------------ + +cockatrice_add_test(NAME expression_test SOURCES expression_test.cpp LIBS libcockatrice_utility) +cockatrice_add_test(NAME clamped_arithmetic_test SOURCES clamped_arithmetic_test.cpp LIBS libcockatrice_utility) +cockatrice_add_test(NAME test_age_formatting SOURCES test_age_formatting.cpp LIBS libcockatrice_utility) +cockatrice_add_test(NAME password_hash_test SOURCES password_hash_test.cpp LIBS libcockatrice_utility) +cockatrice_add_test(NAME report_categories_test SOURCES report_categories_test.cpp LIBS libcockatrice_utility) +cockatrice_add_test(NAME warning_categories_test SOURCES warning_categories_test.cpp LIBS libcockatrice_utility) +cockatrice_add_test( + NAME + playmat_resolver_test + SOURCES + playmat_resolver_test.cpp + LIBS + libcockatrice_deck_list + libcockatrice_utility +) +cockatrice_add_test( + NAME + server_rate_limiter_test + SOURCES + server_rate_limiter_test.cpp + LIBS + libcockatrice_utility + LABELS + server +) +cockatrice_add_test( + NAME + server_card_counter_test + SOURCES + server_card_counter_test.cpp + LIBS + libcockatrice_network + LABELS + server +) +cockatrice_add_test( + NAME + server_counter_test + SOURCES + server_counter_test.cpp + LIBS + libcockatrice_network + LABELS + server +) +cockatrice_add_test( + NAME + server_developer_role_test + NO_SANITIZE + SOURCES + server_developer_role_test.cpp + LIBS + libcockatrice_network + libcockatrice_rng + LABELS + server +) +cockatrice_add_test( + NAME + server_game_join_test + SOURCES + server_game_join_test.cpp + LIBS + libcockatrice_network_server_remote + libcockatrice_rng + LABELS + server +) +cockatrice_add_test( + NAME + latency_tracker_test + SOURCES + latency_tracker_test.cpp + LIBS + libcockatrice_network + LABELS + server +) +cockatrice_add_test( + NAME + lag_monitor_test + SOURCES + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/lag_monitor.cpp + lag_monitor_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src +) +cockatrice_add_test( + NAME + metrics_registry_test + SOURCES + ${CMAKE_SOURCE_DIR}/servatrice/src/metrics_registry.cpp + metrics_registry_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/servatrice/src + LABELS + server +) +cockatrice_add_test( + NAME + card_update_progress_test + SOURCES + ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_database_update/card_update_progress.cpp + card_update_progress_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src +) +cockatrice_add_test( + NAME + loader_local_matching_test + SOURCES + ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/cache_settings.cpp + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/card_counter_settings.cpp + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/shortcuts_settings.cpp + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/client/release_channel.cpp + ${VERSION_STRING_CPP} + loader_local_matching_test.cpp + LIBS + libcockatrice_settings + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src +) +cockatrice_add_test( + NAME + deck_hash_performance_test + SOURCES + deck_hash_performance_test.cpp + LIBS + libcockatrice_deck_list + libcockatrice_utility + LABELS + bench +) + +# The translation loader test compiles a .qm fixture, which needs lrelease from Qt Linguist. +if(TARGET Qt6::lrelease) + set(TRANSLATION_LOADER_TEST_QM "${CMAKE_CURRENT_BINARY_DIR}/translation_loader_test_de.qm") + add_custom_command( + OUTPUT "${TRANSLATION_LOADER_TEST_QM}" + COMMAND Qt6::lrelease "${CMAKE_CURRENT_SOURCE_DIR}/translation_loader_test.ts" -qm "${TRANSLATION_LOADER_TEST_QM}" + DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/translation_loader_test.ts" + COMMENT "Compiling translation_loader_test.ts" + ) + add_custom_target(translation_loader_test_qm DEPENDS "${TRANSLATION_LOADER_TEST_QM}") + + cockatrice_add_test( + NAME + translation_loader_test + SOURCES + translation_loader_test.cpp + LIBS + libcockatrice_utility + DEFINITIONS + TRANSLATION_LOADER_TEST_DIR="${CMAKE_CURRENT_BINARY_DIR}" + ) + add_dependencies(translation_loader_test translation_loader_test_qm) + + # A multi-config generator (MSBuild, Xcode) puts the executable in a per-config subdirectory while + # the .qm stays in the current binary directory, so copy the fixture next to the binary: that is the + # layout ABuildTreeFindsTheTranslationsNextToTheExecutable is about, and the only one it finds on + # those generators. + add_custom_command( + TARGET translation_loader_test + POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy_if_different "${TRANSLATION_LOADER_TEST_QM}" + "$/" + VERBATIM ) endif() -target_link_libraries( - report_categories_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -target_link_libraries(lag_monitor_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) -target_link_libraries( - latency_tracker_test libcockatrice_network Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -target_include_directories(metrics_registry_test PRIVATE ${CMAKE_SOURCE_DIR}/servatrice/src) -target_link_libraries(metrics_registry_test ${TEST_QT_MODULES} Threads::Threads ${GTEST_BOTH_LIBRARIES}) -target_link_libraries(card_update_progress_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) -target_link_libraries( - loader_local_matching_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) +# Guard must match the condition for src/single_instance_manager.cpp in cockatrice/CMakeLists.txt. if(WITH_CLIENT) - target_link_libraries(single_instance_manager_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) + cockatrice_add_test( + NAME + single_instance_manager_test + SOURCES + ${CMAKE_SOURCE_DIR}/cockatrice/src/single_instance_manager.cpp + single_instance_manager_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src + ${CMAKE_SOURCE_DIR}/libcockatrice_utility + ) endif() add_subdirectory(card_zone_algorithms) diff --git a/tests/card_zone_algorithms/CMakeLists.txt b/tests/card_zone_algorithms/CMakeLists.txt index e03d8f77d..945f8a0ed 100644 --- a/tests/card_zone_algorithms/CMakeLists.txt +++ b/tests/card_zone_algorithms/CMakeLists.txt @@ -1,15 +1,4 @@ -add_executable(card_zone_algorithms_test card_zone_algorithms_test.cpp) - -target_include_directories(card_zone_algorithms_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src/game/zones) - -target_link_libraries( - card_zone_algorithms_test - PRIVATE Threads::Threads - PRIVATE ${GTEST_BOTH_LIBRARIES} +cockatrice_add_test( + NAME card_zone_algorithms_test SOURCES card_zone_algorithms_test.cpp INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src/game/zones ) - -add_test(NAME card_zone_algorithms_test COMMAND card_zone_algorithms_test) - -if(NOT GTEST_FOUND) - add_dependencies(card_zone_algorithms_test gtest) -endif() diff --git a/tests/carddatabase/CMakeLists.txt b/tests/carddatabase/CMakeLists.txt index 811cebc1d..22171a9e8 100644 --- a/tests/carddatabase/CMakeLists.txt +++ b/tests/carddatabase/CMakeLists.txt @@ -9,61 +9,46 @@ add_definitions("-DCARDDB_DATADIR=\"${CMAKE_CURRENT_SOURCE_DIR}/data/\"") # ------------------------ # Card Database Test # ------------------------ -add_executable(carddatabase_test ${MOCKS_SOURCES} ${VERSION_STRING_CPP} carddatabase_test.cpp mocks.cpp) - -target_link_libraries( +cockatrice_add_test( + NAME carddatabase_test - PRIVATE libcockatrice_card - PRIVATE Threads::Threads - PRIVATE ${GTEST_BOTH_LIBRARIES} - PRIVATE ${TEST_QT_MODULES} + SOURCES + ${VERSION_STRING_CPP} + carddatabase_test.cpp + mocks.cpp + LIBS + libcockatrice_card ) -add_test(NAME carddatabase_test COMMAND carddatabase_test) - -# ------------------------ -# Load Benchmark (manual, not run in CI) -# ------------------------ -add_executable(load_benchmark ${MOCKS_SOURCES} ${VERSION_STRING_CPP} load_benchmark.cpp mocks.cpp) - -target_link_libraries( - load_benchmark - PRIVATE libcockatrice_card - PRIVATE libcockatrice_models - PRIVATE Threads::Threads - PRIVATE ${GTEST_BOTH_LIBRARIES} - PRIVATE ${TEST_QT_MODULES} -) - -if(NOT GTEST_FOUND) - add_dependencies(load_benchmark gtest) -endif() - # ------------------------ # Filter String Test # (guard must match the condition for libcockatrice_filters in the root CMakeLists.txt) # ------------------------ if(WITH_ORACLE OR WITH_CLIENT) - add_executable(filter_string_test ${MOCKS_SOURCES} ${VERSION_STRING_CPP} filter_string_test.cpp mocks.cpp) - - target_link_libraries( + cockatrice_add_test( + NAME filter_string_test - PRIVATE libcockatrice_filters - PRIVATE Threads::Threads - PRIVATE ${GTEST_BOTH_LIBRARIES} - PRIVATE ${TEST_QT_MODULES} + SOURCES + ${VERSION_STRING_CPP} + filter_string_test.cpp + mocks.cpp + LIBS + libcockatrice_filters ) - - add_test(NAME filter_string_test COMMAND filter_string_test) - - if(NOT GTEST_FOUND) - add_dependencies(filter_string_test gtest) - endif() endif() # ------------------------ -# Dependencies on gtest +# Load Benchmark (manual, not run in CI) # ------------------------ -if(NOT GTEST_FOUND) - add_dependencies(carddatabase_test gtest) -endif() +cockatrice_add_manual_test( + NAME + load_benchmark + SOURCES + ${VERSION_STRING_CPP} + load_benchmark.cpp + mocks.cpp + LIBS + libcockatrice_card + libcockatrice_models + MANUAL +) diff --git a/tests/deck_list_model/CMakeLists.txt b/tests/deck_list_model/CMakeLists.txt index e3096c559..cd7d693f3 100644 --- a/tests/deck_list_model/CMakeLists.txt +++ b/tests/deck_list_model/CMakeLists.txt @@ -1,33 +1,23 @@ -add_executable(deck_list_model_custom_zones_test ${VERSION_STRING_CPP} deck_list_model_custom_zones_test.cpp) - -if(NOT GTEST_FOUND) - add_dependencies(deck_list_model_custom_zones_test gtest) -endif() - -target_link_libraries( +cockatrice_add_test( + NAME deck_list_model_custom_zones_test + SOURCES + ${VERSION_STRING_CPP} + deck_list_model_custom_zones_test.cpp + LIBS libcockatrice_models libcockatrice_card libcockatrice_deck_list - Threads::Threads - ${GTEST_BOTH_LIBRARIES} - ${TEST_QT_MODULES} ) -add_test(NAME deck_list_model_custom_zones_test COMMAND deck_list_model_custom_zones_test) -add_executable(deck_list_model_zone_integration_test ${VERSION_STRING_CPP} deck_list_model_zone_integration_test.cpp) - -if(NOT GTEST_FOUND) - add_dependencies(deck_list_model_zone_integration_test gtest) -endif() - -target_link_libraries( +cockatrice_add_test( + NAME deck_list_model_zone_integration_test + SOURCES + ${VERSION_STRING_CPP} + deck_list_model_zone_integration_test.cpp + LIBS libcockatrice_models libcockatrice_card libcockatrice_deck_list - Threads::Threads - ${GTEST_BOTH_LIBRARIES} - ${TEST_QT_MODULES} ) -add_test(NAME deck_list_model_zone_integration_test COMMAND deck_list_model_zone_integration_test) diff --git a/tests/deck_list_zones/CMakeLists.txt b/tests/deck_list_zones/CMakeLists.txt index 0710be94d..7af8fd187 100644 --- a/tests/deck_list_zones/CMakeLists.txt +++ b/tests/deck_list_zones/CMakeLists.txt @@ -1,10 +1 @@ -add_executable(deck_list_zones_test deck_list_zones_test.cpp) - -if(NOT GTEST_FOUND) - add_dependencies(deck_list_zones_test gtest) -endif() - -target_link_libraries( - deck_list_zones_test libcockatrice_deck_list Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -add_test(NAME deck_list_zones_test COMMAND deck_list_zones_test) +cockatrice_add_test(NAME deck_list_zones_test SOURCES deck_list_zones_test.cpp LIBS libcockatrice_deck_list) diff --git a/tests/dummy_test.cpp b/tests/dummy_test.cpp deleted file mode 100644 index 8fd9c78dd..000000000 --- a/tests/dummy_test.cpp +++ /dev/null @@ -1,20 +0,0 @@ -#include "gtest/gtest.h" -#include - -namespace -{ -class FooTest : public ::testing::Test -{ -}; - -TEST(DummyTest, Works) -{ - ASSERT_EQ(1, 1) << "One is not equal to one"; -} -} // namespace - -int main(int argc, char **argv) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} \ No newline at end of file diff --git a/tests/loading_from_clipboard/CMakeLists.txt b/tests/loading_from_clipboard/CMakeLists.txt index 719d62f45..88711e457 100644 --- a/tests/loading_from_clipboard/CMakeLists.txt +++ b/tests/loading_from_clipboard/CMakeLists.txt @@ -1,12 +1,13 @@ add_definitions("-DCARDDB_DATADIR=\"${CMAKE_CURRENT_SOURCE_DIR}/data/\"") -add_executable(loading_from_clipboard_test ${VERSION_STRING_CPP} clipboard_testing.cpp loading_from_clipboard_test.cpp) -if(NOT GTEST_FOUND) - add_dependencies(loading_from_clipboard_test gtest) -endif() - -target_link_libraries( - loading_from_clipboard_test libcockatrice_deck_list libcockatrice_card Threads::Threads ${GTEST_BOTH_LIBRARIES} - ${TEST_QT_MODULES} +cockatrice_add_test( + NAME + loading_from_clipboard_test + SOURCES + ${VERSION_STRING_CPP} + clipboard_testing.cpp + loading_from_clipboard_test.cpp + LIBS + libcockatrice_deck_list + libcockatrice_card ) -add_test(NAME loading_from_clipboard_test COMMAND loading_from_clipboard_test) diff --git a/tests/movecard_tests/CMakeLists.txt b/tests/movecard_tests/CMakeLists.txt index 769047148..68d8fc84c 100755 --- a/tests/movecard_tests/CMakeLists.txt +++ b/tests/movecard_tests/CMakeLists.txt @@ -1,16 +1,11 @@ -add_executable(reverse_card_move_test reverse_card_move_test.cpp) - -if(NOT GTEST_FOUND) - add_dependencies(reverse_card_move_test gtest) -endif() - -target_link_libraries( +cockatrice_add_test( + NAME reverse_card_move_test - PRIVATE libcockatrice_network_server_remote - PRIVATE libcockatrice_rng - PRIVATE Threads::Threads - PRIVATE ${GTEST_BOTH_LIBRARIES} - PRIVATE ${TEST_QT_MODULES} + SOURCES + reverse_card_move_test.cpp + LIBS + libcockatrice_network_server_remote + libcockatrice_rng + LABELS + server ) - -add_test(NAME reverse_card_move_test COMMAND reverse_card_move_test) diff --git a/tests/oracle/CMakeLists.txt b/tests/oracle/CMakeLists.txt index 9bc5ee5be..c896c7960 100644 --- a/tests/oracle/CMakeLists.txt +++ b/tests/oracle/CMakeLists.txt @@ -1,37 +1,31 @@ -add_executable(parse_cipt_test ../../oracle/src/parsehelpers.cpp parse_cipt_test.cpp) - -if(NOT GTEST_FOUND) - add_dependencies(parse_cipt_test gtest) -endif() - -target_link_libraries(parse_cipt_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) - -add_test(NAME parse_cipt_test COMMAND parse_cipt_test) +cockatrice_add_test(NAME parse_cipt_test SOURCES ../../oracle/src/parsehelpers.cpp parse_cipt_test.cpp) # Oracle importer unit tests -add_executable( - oracle_importer_test ${VERSION_STRING_CPP} ../../oracle/src/oracleimporter.cpp ../../oracle/src/parsehelpers.cpp - ../../oracle/src/raw_json_scanner.cpp oracle_importer_test.cpp +cockatrice_add_test( + NAME + oracle_importer_test + SOURCES + ${VERSION_STRING_CPP} + ../../oracle/src/oracleimporter.cpp + ../../oracle/src/parsehelpers.cpp + ../../oracle/src/raw_json_scanner.cpp + oracle_importer_test.cpp + LIBS + libcockatrice_card + libcockatrice_interfaces ) -if(NOT GTEST_FOUND) - add_dependencies(oracle_importer_test gtest) -endif() - -target_link_libraries( - oracle_importer_test libcockatrice_card libcockatrice_interfaces Threads::Threads ${GTEST_BOTH_LIBRARIES} - ${TEST_QT_MODULES} -) - -add_test(NAME oracle_importer_test COMMAND oracle_importer_test) - # Oracle importer benchmark tests (manual, not run in CI, incl. RAM benchmark) # Optional compression libs, mirrored from oracle/CMakeLists.txt, so the benchmark # can download and decompress whatever AllPrintings format the default URL selects. +# add_definitions() is directory-scoped and order-independent, so HAS_ZLIB/HAS_LZMA also +# reach the two unit tests above. That is harmless: those macros gate code only in +# oracle/src/pages.cpp and oracle/src/pagetemplates.cpp, which no test target compiles. find_package(ZLIB) if(ZLIB_FOUND) add_definitions("-DHAS_ZLIB") set(_ORACLE_BENCH_EXTRA_SOURCES ../../oracle/src/zip/unzip.cpp ../../oracle/src/zip/zipglobal.cpp) + cockatrice_disable_sanitizers_for(${_ORACLE_BENCH_EXTRA_SOURCES}) set(_ORACLE_BENCH_EXTRA_LIBRARIES ${ZLIB_LIBRARIES}) include_directories(${ZLIB_INCLUDE_DIRS}) else() @@ -42,28 +36,27 @@ find_package(LibLZMA) if(LIBLZMA_FOUND) add_definitions("-DHAS_LZMA") list(APPEND _ORACLE_BENCH_EXTRA_SOURCES ../../oracle/src/lzma/decompress.cpp) + cockatrice_disable_sanitizers_for(../../oracle/src/lzma/decompress.cpp) list(APPEND _ORACLE_BENCH_EXTRA_LIBRARIES ${LIBLZMA_LIBRARIES}) include_directories(${LIBLZMA_INCLUDE_DIRS}) else() message(STATUS "Oracle tests: LibLZMA not found; xz download benchmark disabled") endif() -add_executable( - oracle_importer_benchmark_test - ${VERSION_STRING_CPP} ../../oracle/src/oracleimporter.cpp ../../oracle/src/parsehelpers.cpp - ../../oracle/src/raw_json_scanner.cpp oracle_importer_benchmark_test.cpp ${_ORACLE_BENCH_EXTRA_SOURCES} -) - -if(NOT GTEST_FOUND) - add_dependencies(oracle_importer_benchmark_test gtest) -endif() - -target_link_libraries( +cockatrice_add_manual_test( + NAME oracle_importer_benchmark_test + SOURCES + ${VERSION_STRING_CPP} + ../../oracle/src/oracleimporter.cpp + ../../oracle/src/parsehelpers.cpp + ../../oracle/src/raw_json_scanner.cpp + oracle_importer_benchmark_test.cpp + ${_ORACLE_BENCH_EXTRA_SOURCES} + LIBS libcockatrice_card libcockatrice_interfaces - Threads::Threads - ${GTEST_BOTH_LIBRARIES} - ${TEST_QT_MODULES} + EXTRA_LIBS ${_ORACLE_BENCH_EXTRA_LIBRARIES} + MANUAL ) diff --git a/tests/picture_loader_benchmark/CMakeLists.txt b/tests/picture_loader_benchmark/CMakeLists.txt index a3b75fe88..0684f58f4 100644 --- a/tests/picture_loader_benchmark/CMakeLists.txt +++ b/tests/picture_loader_benchmark/CMakeLists.txt @@ -1,8 +1,10 @@ -# Manual benchmark hitting the real card image hosts. Not registered with -# add_test(): it requires a cards.xml, touches the network for minutes at a -# time, and needs network access. Build it explicitly and run by hand. -add_executable( +# Manual benchmark hitting the real card image hosts. Not registered with CTest: it requires a +# cards.xml, touches the network for minutes at a time, and needs network access. Build it +# explicitly and run by hand. +cockatrice_add_manual_test( + NAME picture_loader_benchmark_test + SOURCES ${VERSION_STRING_CPP} ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp ../../cockatrice/src/interface/card_picture_loader/card_picture_to_load.cpp @@ -11,16 +13,12 @@ add_executable( ../../cockatrice/src/client/settings/cache_settings.h picture_loader_benchmark.cpp settings_cache_mock.cpp -) - -target_include_directories(picture_loader_benchmark_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) - -target_link_libraries( - picture_loader_benchmark_test + LIBS libcockatrice_card libcockatrice_settings libcockatrice_interfaces libcockatrice_utility - Threads::Threads - ${TEST_QT_MODULES} + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src + MANUAL ) diff --git a/tests/settings/CMakeLists.txt b/tests/settings/CMakeLists.txt index 7ee32a645..98bed1f4b 100644 --- a/tests/settings/CMakeLists.txt +++ b/tests/settings/CMakeLists.txt @@ -1,26 +1,14 @@ -add_executable(settings_manager_test settings_manager_test.cpp) -add_executable(settings_defaults_test settings_defaults_test.cpp) -add_executable(settings_migration_test settings_migration_test.cpp) +cockatrice_add_test(NAME settings_manager_test SOURCES settings_manager_test.cpp LIBS libcockatrice_settings) -target_link_libraries( - settings_manager_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} +cockatrice_add_test( + NAME + settings_defaults_test + SOURCES + settings_defaults_test.cpp + LIBS + libcockatrice_settings + DEFINITIONS + SETTINGS_UNIT_TEST ) -target_link_libraries( - settings_defaults_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) -target_compile_definitions(settings_defaults_test PRIVATE SETTINGS_UNIT_TEST) - -target_link_libraries( - settings_migration_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} -) - -add_test(NAME settings_manager_test COMMAND settings_manager_test) -add_test(NAME settings_defaults_test COMMAND settings_defaults_test) -add_test(NAME settings_migration_test COMMAND settings_migration_test) - -if(NOT GTEST_FOUND) - add_dependencies(settings_manager_test gtest) - add_dependencies(settings_defaults_test gtest) - add_dependencies(settings_migration_test gtest) -endif() +cockatrice_add_test(NAME settings_migration_test SOURCES settings_migration_test.cpp LIBS libcockatrice_settings) diff --git a/tests/theme_migration/CMakeLists.txt b/tests/theme_migration/CMakeLists.txt index ac202210e..93493ee5f 100644 --- a/tests/theme_migration/CMakeLists.txt +++ b/tests/theme_migration/CMakeLists.txt @@ -1,5 +1,7 @@ -add_executable( +cockatrice_add_test( + NAME theme_migration_test + SOURCES ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/theme_manager.cpp ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/theme_config.cpp ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/pixel_map_generator.cpp @@ -9,22 +11,11 @@ add_executable( ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/client/release_channel.cpp ${VERSION_STRING_CPP} theme_migration_test.cpp -) -target_include_directories(theme_migration_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) - -target_link_libraries( - theme_migration_test + LIBS libcockatrice_settings libcockatrice_network libcockatrice_network_server_remote libcockatrice_protocol - Threads::Threads - ${GTEST_BOTH_LIBRARIES} - ${TEST_QT_MODULES} + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src ) - -add_test(NAME theme_migration_test COMMAND theme_migration_test) - -if(NOT GTEST_FOUND) - add_dependencies(theme_migration_test gtest) -endif() From bdda3dedfaf58065f167bbc26a62b8685f2cb427 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:05:28 +0200 Subject: [PATCH 02/15] [CI] Publish a gcovr line-coverage report from the test run (#7404) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Tests] Register each test case with CTest instead of each test binary Registering a test cost four edits across a dozen CMakeLists files: add_test, add_executable, a conditional add_dependencies on gtest, and target_link_libraries. The conditional branch was the trap -- it is only needed when GTest was fetched and add_subdirectory'd, and omitting it yields undefined gtest symbols at link time -- so it was easy to leave out and impossible to notice until the link failed. Fold all four steps into cockatrice_add_test() and migrate every test directory onto it, with a cockatrice_add_manual_test() wrapper for the three benchmarks that are built but deliberately kept out of ctest. The calls now read as a declaration of what a test is rather than a transcript of how to register one. While rewriting the registrations, replace add_test(NAME x COMMAND x) with gtest_discover_tests(). Naming the binary as a single CTest entry meant a CI failure reported only which binary failed, and ctest -R Suite.Case could not select an individual case. Discovery raises the suite from 35 opaque entries to 469 real ones: ctest -R ThemeMigrationTest.IsIdempotent now runs exactly that case, and ctest -L server / -LE bench work off labels applied at registration. Give every test a 60s TIMEOUT. CTest's implicit default is 1500s, so one hung socket wait would stall CI for 25 minutes, and single_instance_manager_test already does real QLocalSocket waits. Dropping per-case granularity costs wall-clock time, since the suite becomes process-spawn bound rather than CPU bound: serial ctest went from 3.8s to 30.8s. Run it with -j in CI to recover it -- 7.6s at -j4, and 3.5s at -j8, both at or below the old serial time. The binaries are parallel-safe: the only test claiming a fixed resource scopes its local socket name to its own pid, so concurrent copies cannot collide. Also delete dummy_test, which asserted 1 == 1 and only proved the harness was wired up -- something the other 468 cases already establish. No test is added, removed, or changed by this commit; the 469 cases are the same ones the 35 binaries contained. * [Tests] Make QtTest available to test targets QtTest was not among the modules test targets could link, so the Qt model and signal contracts had no way to be verified from a test: QAbstractItemModelTester and QSignalSpy both live there. Add Test to _TEST_NEEDED in cmake/FindQtRuntime.cmake. That is the single place the union of test-only Qt modules is declared, so this is the only file that needs to change to make the module reachable from every test target. Nothing uses it yet. This lands the capability ahead of the model-layer tests that need it, so those can be added as tests alone rather than as test plus build-system plumbing. * [Tests] Correct the note on the oracle test compression defines add_definitions() populates the directory's COMPILE_DEFINITIONS property, which is consumed at generate time, so it reaches every target in the directory no matter where it is called relative to add_executable(). The previous note claimed the flags stayed confined to the benchmark because the find_package block came after it; oracle_importer_test, created above that block, carries both defines too. Verified harmless rather than a live bug: HAS_ZLIB and HAS_LZMA gate code only in oracle/src/pages.cpp and oracle/src/pagetemplates.cpp, and no test target compiles either. The ordering and the leaked defines are both pre-existing, so this only corrects the explanation. * [Tests] Carry one CTest label per test instead of several gtest_discover_tests() can hand CTest exactly one label. GoogleTestAddTests.cmake accumulates its generated script with `foreach(arg ${ARGN})`, so a label list reaches the CTestTestfile as bare `LABELS ci server`, and CTest reads only the first value of a property and drops the rest without a word. The `ci` label that is appended to every registered test was therefore absent from every test that also asked for `server` or `bench`: `ctest -L ci` reported 400 of 469 cases, and `ctest --print-labels` never listed a second label anywhere in the build. Reduce each test to exactly one of `ci`, `server`, or `bench`, which is all the selections need. The three now partition the suite exactly, 400 + 64 + 5 = 469, and `ctest -LE bench` still means everything but benchmarks. A second label is now a configure-time FATAL_ERROR rather than a line that quietly means something else. Anything orthogonal to a test's kind belongs in a helper flag beside the label, so it composes with the group instead of competing for the single slot. Also raise DISCOVERY_TIMEOUT off GoogleTestAddTests' 5s default. A plain debug binary answers --gtest_list_tests in 0.04s, but that budget is thin once several binaries discover concurrently behind a -j build, and being over it fails at the *link* step with a bare "Process terminated due to timeout" that points nowhere near the cause. * [Tests] Link gtest_main on the fetched-gtest path The downloaded-gtest branch set GTEST_BOTH_LIBRARIES to `gtest` alone, while FindGTest sets it to both halves. Every test target links that list rather than picking an entry point, so on this path nothing provides main() and any test source that does not define its own fails at link with `undefined reference to main` pointing at crt1.o, naming neither gtest nor the line responsible. That stayed latent while every test carried its own four-line main(). A test that omits it now fails, and the error points at the C runtime rather than at gtest, which is a long way from the cause. Restoring both halves makes the two paths agree and drops the boilerplate requirement on new test sources. The 18 sources that still define main() keep linking: gtest_main.cc defines nothing but main, so the linker leaves its object in the archive unused rather than reporting a duplicate. * [Build] Add an ENABLE_SANITIZERS option for ASan and UBSan Adds -DENABLE_SANITIZERS, off by default, building with AddressSanitizer and UndefinedBehaviorSanitizer. The bulk of the work is in the three ways the obvious implementation silently does the wrong thing. Probe the flags on the link step, not just the compile step. check_cxx_compiler_flag() only compiles, so every sanitizer flag comes back unsupported, because linking an ASan object without -fsanitize=address leaves __asan_init undefined. The result is not a build failure but a build that appears instrumented and is not, which is worse. Use check_cxx_source_compiles() with CMAKE_REQUIRED_LINK_OPTIONS instead, and treat an unsupported sanitizer pair as fatal since the cosmetic flags are allowed to degrade. Opt vendored sources out per source, through a cockatrice_disable_sanitizers_for() helper rather than a list of set_source_files_properties() calls at the root. The helper has to exist because CMake resolves a source-file property against the directory scope its target was declared in: setting the property from the root with absolute paths is a no-op, silently, which is why the call sites sit next to the add_library and add_executable that pull the vendored code in. SFMT, minizip, the LZMA SDK and Qxt SMTP are excluded. oracle/src/qt-json is in the history but not in the tree, removed in #7214. Also mark those sources SKIP_PRECOMPILE_HEADERS. The PCH is built with the sanitizer flags, so it defines __SANITIZE_ADDRESS__; a translation unit compiled with -fno-sanitize=all does not and GCC then refuses the PCH, which is a fatal -Werror=invalid-pch in the servatrice and oracle builds. -fno-sanitize=vptr is not optional. Qt constructs QObject subclasses into raw storage and writes the vptr afterwards, so the vptr check reports on essentially every QObject instantiation, and it has no value here because nothing outside moc-generated code inherits QObject. Verified on the resulting build: our own translation units carry ASan and UBSan calls, SFMT.o carries neither, and the full suite of 464 cases passes clean under ASAN_OPTIONS=detect_leaks=0:abort_on_error=1 with UBSAN_OPTIONS=halt_on_error=0. The test suite needs two adjustments to make this option usable at all, both of which land here because they are consequences of COCKATRICE_SANITIZERS existing rather than independent changes. DISCOVERY_TIMEOUT rises from 60s to 300s under sanitizers. An instrumented binary takes 2.8s to answer --gtest_list_tests against 0.04s uninstrumented, and discovery runs several binaries at once behind a -j build. One target is excluded via a NO_SANITIZE flag, server_developer_role_test. Reaching google::protobuf::TextFormat::Printer's destructor aborts on an absl assertion inside system protobuf 33.4 whenever the consumer translation unit is built without NDEBUG, which a Debug sanitizer build is. This reproduces with no Cockatrice code at all, just "TextFormat::Printer p; p.PrintToString(msg, &out);" built with -fsanitize=address, so it is upstream rather than ours. It is a flag and not a fourth label because "skip this in the sanitizer leg" has to compose with a test's group rather than compete for the single label slot, and because a flag excludes by not registering the cases at all, which cannot rot the way a label silently can. The target still builds and still runs on the legs without sanitizers. * [CI] Run the suite once under ASan and UBSan Adds --sanitize to .ci/compile.sh, mirroring --test, and turns it on for one entry of the existing Linux Docker matrix: Ubuntu 26.04, which already had a test-enabled leg. That reuses the image, the ccache layer and the existing step shape, so the CI diff is a matrix key plus the argument plumbing. Debian 13 and Fedora 44 keep covering the uninstrumented suite, so nothing is lost by instrumenting one leg. Sanitizers stay off the release and package legs, which build with --release for packaging: instrumenting those would change the shipped binaries' optimization profile and build time for no benefit. macOS and Windows stay uninstrumented too, since ASan is unreliable against the Qt build there and MSVC has no equivalent without /fsanitize=address. --sanitize does not imply --test, so an instrumented build can be produced locally without paying for ctest. When both are given, ctest runs with detect_leaks=0 because Qt and the sanitizer runtimes both leak at exit by design and LeakSanitizer's default buries real findings under hundreds of process-teardown reports, and with UBSan halt_on_error=0 so one run collects many findings instead of stopping at the first. ASan keeps abort_on_error=1, since a bad access leaves the process unusable and continuing only produces a second, more confusing report from the wreckage. * [CI] Publish a gcovr line-coverage report from the test run Sanitizers tell us what a run did wrong; they say nothing about what it never reached. This adds the other half: an instrumented build whose report says which of our own lines no test executes. Add an ENABLE_COVERAGE option that compiles with --coverage -O0 -g, mirroring the ENABLE_SANITIZERS opt-in including the per-source carve-out that keeps vendored trees out of the denominator. -O0 is deliberate: at higher optimisation levels GCC merges and relocates lines, so the report would describe code that no longer exists in that form. Only GCC is supported, since Clang's intermediate format needs llvm-cov; asking for coverage under Clang is a hard error rather than a build that silently produces unreadable .gcno files. Keep the filters in a committed gcovr.cfg rather than in the invocation. The tests themselves are excluded because they are compiled with the same instrumentation and are only ever executed by the run doing the measuring, so leaving them in would report a healthier project than we have. The process entry points and the client's window chrome go too: each needs a display, an interactive terminal or a live database before it does anything a headless test could observe, so they would otherwise sit at 0% forever and bury real movement. Give coverage its own matrix leg rather than a second flag on the sanitizer leg. The two answer different questions, and combining them slows both down without telling us more. The new variant key gives the leg its own Docker image and ccache entry so the two legs cannot race on the same tag. The leg publishes an HTML report with per-file pages, LCOV, Cobertura XML and a JSON summary. gcovr is a report-time dependency rather than a build dependency, so a missing one warns and skips instead of failing an otherwise green build. * [CI] Resolve the coverage source root before the build dir cd The previous commit computed SRC_ROOT after `cd "$BUILD_DIR"`, under a comment claiming it ran before. `$0` is relative to the directory the script was invoked from, so from inside the build dir `dirname "$0"` no longer resolves and the subshell's `cd` fails. Under `set -e` that is fatal rather than an empty variable, which took down every leg in the matrix, coverage or not, including the ones this change does not otherwise touch. Move it above the cd and guard it on MAKE_COVERAGE, since the coverage report is the only consumer. Resolving it unconditionally meant a coverage-only detail was paid for by every build in the fleet, and gave every one of them a way to fail. --------- Co-authored-by: Lukas Brübach --- .ci/Ubuntu26.04/Dockerfile | 1 + .ci/compile.sh | 47 +++++++++++++++++- .ci/docker.sh | 10 +++- .github/workflows/desktop-build.yml | 44 +++++++++++++++-- CMakeLists.txt | 8 ++- cmake/Coverage.cmake | 75 +++++++++++++++++++++++++++++ gcovr.cfg | 63 ++++++++++++++++++++++++ libcockatrice_rng/CMakeLists.txt | 1 + oracle/CMakeLists.txt | 2 + servatrice/CMakeLists.txt | 3 ++ tests/oracle/CMakeLists.txt | 2 + 11 files changed, 247 insertions(+), 9 deletions(-) create mode 100644 cmake/Coverage.cmake create mode 100644 gcovr.cfg diff --git a/.ci/Ubuntu26.04/Dockerfile b/.ci/Ubuntu26.04/Dockerfile index 96dd10763..3166b7eb4 100644 --- a/.ci/Ubuntu26.04/Dockerfile +++ b/.ci/Ubuntu26.04/Dockerfile @@ -9,6 +9,7 @@ RUN apt-get update && \ cmake \ file \ g++ \ + gcovr \ git \ libgl-dev \ liblzma-dev \ diff --git a/.ci/compile.sh b/.ci/compile.sh index de4fac004..606b9d201 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -9,14 +9,16 @@ # --server compiles servatrice # --test runs tests # --sanitize builds with AddressSanitizer and UndefinedBehaviorSanitizer, gcc/clang only +# --coverage instruments the test run for line coverage and prints a gcovr summary, gcc only + # --debug or --release sets the build type ie CMAKE_BUILD_TYPE # --ccache [] uses ccache and shows stats, optionally provide size # --evict-ccache runs ccache eviction based on given age after build # --dir sets the name of the build dir, default is "build" # --cmake-generator sets CMAKE_GENERATOR as used by cmake # --target-macos-version sets the min os version - only used for macOS builds -# uses env: BUILDTYPE MAKE_INSTALL MAKE_PACKAGE PACKAGE_TYPE PACKAGE_SUFFIX MAKE_SERVER MAKE_NO_CLIENT MAKE_TEST USE_CCACHE CCACHE_SIZE CCACHE_EVICTION_AGE BUILD_DIR CMAKE_GENERATOR TARGET_MACOS_VERSION -# (correspond to args: --debug/--release --install --package --suffix --server --test --ccache --dir ) +# uses env: BUILDTYPE MAKE_INSTALL MAKE_PACKAGE PACKAGE_TYPE PACKAGE_SUFFIX MAKE_SERVER MAKE_NO_CLIENT MAKE_TEST USE_CCACHE CCACHE_SIZE CCACHE_EVICTION_AGE BUILD_DIR CMAKE_GENERATOR TARGET_MACOS_VERSION MAKE_SANITIZE MAKE_COVERAGE +# (correspond to args: --debug/--release --install --package --suffix --server --test --sanitize --coverage --ccache --dir ) # exitcode: 1 for failure, 3 for invalid arguments # Read arguments @@ -62,6 +64,10 @@ while [[ $# != 0 ]]; do MAKE_SANITIZE=1 shift ;; + '--coverage') + MAKE_COVERAGE=1 + shift + ;; '--debug') BUILDTYPE="Debug" shift @@ -136,6 +142,15 @@ if [[ ! $BUILD_DIR ]]; then BUILD_DIR="build" fi mkdir -p "$BUILD_DIR" + +if [[ $MAKE_COVERAGE ]]; then + # Resolved while the working directory is still the source root. $0 is relative to the + # directory the script was invoked from, so after the cd below it no longer resolves, and + # under `set -e` the failed subshell would abort the build rather than just yielding an empty + # variable. Only the coverage report needs this, so only coverage pays for it. + SRC_ROOT="$(cd "$(dirname "$0")/.." && pwd)" +fi + cd "$BUILD_DIR" # Set minimum CMake Version @@ -160,6 +175,9 @@ else # into the shipped binaries (libasan/libubsan Depends in the DEB). flags+=("-DENABLE_SANITIZERS=OFF") fi +if [[ $MAKE_COVERAGE ]]; then + flags+=("-DENABLE_COVERAGE=ON") +fi if [[ $USE_CCACHE ]]; then flags+=("-DUSE_CCACHE=1") # PCH-aware caching is required or ccache refuses to cache any TU that @@ -341,6 +359,31 @@ if [[ $MAKE_TEST ]]; then echo "::endgroup::" fi +if [[ $MAKE_COVERAGE ]]; then + # gcovr is not a build dependency and is not available everywhere, so a missing one is reported + # and skipped rather than failing an otherwise green build. + echo "::group::Coverage report" + GCOVR="$(command -v gcovr || true)" + if [[ -z $GCOVR ]]; then + echo "gcovr not found; install it to produce a coverage report" + else + "$GCOVR" --version + # This script configures with `cmake ..`, so the build dir it was started from is the object + # directory and the source root is one level up. Both are passed explicitly: gcovr otherwise + # walks the current directory and trips over in-tree build directories left by other tooling, + # whose stale .gcno files cannot be resolved. --config is named too, so the filters apply no + # matter what directory gcovr happens to start from. + GCOVR_ARGS=("." --root "$SRC_ROOT" --object-directory "." --config "$SRC_ROOT/gcovr.cfg") + "$GCOVR" "${GCOVR_ARGS[@]}" --txt --decisions + "$GCOVR" "${GCOVR_ARGS[@]}" --html-details coverage-details.html --html-title "Cockatrice line coverage" + "$GCOVR" "${GCOVR_ARGS[@]}" --lcov -o coverage.lcov + "$GCOVR" "${GCOVR_ARGS[@]}" --cobertura-pretty -o coverage.xml + "$GCOVR" "${GCOVR_ARGS[@]}" --json-summary-pretty -o coverage-summary.json + echo "Coverage artifacts written to coverage-details.html, coverage.lcov, coverage.xml and coverage-summary.json" + fi + echo "::endgroup::" +fi + if [[ $MAKE_INSTALL ]]; then echo "::group::Install" cmake --build . --target install --config "$BUILDTYPE" diff --git a/.ci/docker.sh b/.ci/docker.sh index 46112daaa..412ad6a1d 100644 --- a/.ci/docker.sh +++ b/.ci/docker.sh @@ -13,11 +13,17 @@ # --set-cache sets the location to cache the image or for ccache # # requires: docker -# uses env: NAME CACHE BUILD GET SAVE INTERACTIVE +# uses env: NAME IMAGE_SUFFIX CACHE BUILD GET SAVE INTERACTIVE # (correspond to args: --set-cache --build --get --save --interactive) # sets env: RUN CCACHE_DIR IMAGE_NAME RUN_ARGS RUN_OPTS BUILD_SCRIPT # exitcode: 1 for failure, 2 for missing dockerfile, 3 for invalid arguments # +# IMAGE_SUFFIX is optional and only appended to the image name, never to NAME, so that a matrix +# leg can have its own image and image cache without needing its own directory under .ci. Two legs +# building and tagging the same image concurrently would race on `docker build --tag`. +# It is deliberately not called SUFFIX: compile.sh already uses SUFFIX for output file names, and +# a job-level SUFFIX would be silently overridden by those per-step env blocks. +# # exported RUN function will run the BUILD_SCRIPT inside of the docker container. # note that the docker container will not inherit any environment variables! # @@ -81,7 +87,7 @@ if ! [[ $NAME ]]; then return 3 fi -export IMAGE_NAME="${project_name,,}_${NAME,,}" # lower case +export IMAGE_NAME="${project_name,,}_${NAME,,}${IMAGE_SUFFIX:+_${IMAGE_SUFFIX,,}}" # lower case docker_dir=".ci/$NAME" if ! [[ -r $docker_dir/Dockerfile ]]; then diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index 57c2993f6..098aabc4f 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -149,17 +149,31 @@ jobs: # uninstrumented suite, so nothing is lost by instrumenting this one. sanitize: yes - name: ${{ matrix.distro }} ${{ matrix.version }} + - distro: Ubuntu + version: 26.04 + variant: cover # separate image and ccache, same distro + + # The one leg that runs instrumented for line coverage and publishes the report as + # a workflow artifact. It is a separate leg rather than a second flag on the + # sanitizer leg because the two instruments answer different questions: the + # sanitizer leg reports what the run did wrong, the coverage leg reports what the + # run never touched at all. Combining them would also slow both down. Packaging is + # skipped because the leg above already publishes the DEB for this distro. + coverage: yes + package: skip + + name: ${{ matrix.distro }} ${{ matrix.version }}${{ matrix.variant && format(' {0}', matrix.variant) || '' }} needs: configure runs-on: ubuntu-latest # https://github.com/actions/runner-images continue-on-error: ${{ matrix.allow-failure == 'yes' }} timeout-minutes: 70 env: - CACHE: ${{ github.workspace }}/.cache/${{ matrix.distro }}${{ matrix.version }} # directory for caching docker image and ccache + CACHE: ${{ github.workspace }}/.cache/${{ matrix.distro }}${{ matrix.version }}${{ matrix.variant }} # directory for caching docker image and ccache CCACHE_EVICTION_AGE: 7d CCACHE_SIZE: 600M # space of all repo is 10Gi: https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#usage-limits-and-eviction-policy CMAKE_GENERATOR: 'Ninja' NAME: ${{ matrix.distro }}${{ matrix.version }} + IMAGE_SUFFIX: ${{ matrix.variant }} steps: - name: "Checkout" @@ -169,7 +183,7 @@ jobs: id: restore_ccache uses: actions/cache/restore@v6 with: - key: ccache-${{ matrix.distro }}${{ matrix.version }} + key: ccache-${{ matrix.distro }}${{ matrix.version }}${{ matrix.variant }} path: ${{ env.CACHE }} - name: "Build ${{ matrix.distro }} ${{ matrix.version }} Docker image" @@ -181,6 +195,7 @@ jobs: shell: bash env: sanitize: '${{ matrix.sanitize }}' + coverage: '${{ matrix.coverage }}' run: | source .ci/docker.sh args=() @@ -190,9 +205,32 @@ jobs: extra=() [[ $sanitize == yes ]] && extra+=(--sanitize) + [[ $coverage == yes ]] && extra+=(--coverage) RUN --server --debug --test "${extra[@]}" "${args[@]}" + # RUN bind-mounts the workspace into the container, so the report the build wrote into + # build/ is already on the host and no second container is needed to collect it. + - name: "Collect coverage report" + if: matrix.coverage == 'yes' + shell: bash + run: | + mkdir -p coverage + # --html-details writes one page per source file plus a stylesheet and script, and the + # index links to all of them by relative name. Copying the index alone would publish a + # report where every link 404s, so the whole family has to travel together. + cp build/coverage-details* coverage/ + cp build/coverage.lcov build/coverage.xml build/coverage-summary.json coverage/ + ls -la coverage/ + + - name: "Upload coverage artifact" + if: matrix.coverage == 'yes' + uses: actions/upload-artifact@v4 + with: + name: coverage-report + path: coverage/ + if-no-files-found: warn + - name: "Build release package" id: build if: matrix.package != 'skip' diff --git a/CMakeLists.txt b/CMakeLists.txt index ef8866e3a..b272d4ad8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -24,6 +24,8 @@ option(WITH_SERVER "Build Cockatrice server (Servatrice)" OFF) option(TEST "Build tests" OFF) # Build with AddressSanitizer and UndefinedBehaviorSanitizer option(ENABLE_SANITIZERS "Build with AddressSanitizer and UndefinedBehaviorSanitizer" OFF) +# Build instrumented for GCC line coverage, reported with gcovr +option(ENABLE_COVERAGE "Build instrumented for GCC line coverage" OFF) # Use vcpkg regardless of OS option(USE_VCPKG "Use vcpkg regardless of OS" OFF) @@ -213,9 +215,11 @@ else() set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -pipe") endif() -# Always included, because it defines cockatrice_disable_sanitizers_for() for the vendored -# trees. It only touches the global flags when ENABLE_SANITIZERS is on. +# Always included, because they define cockatrice_disable_sanitizers_for() and +# cockatrice_disable_coverage_for() for the vendored trees. They only touch the global flags +# when ENABLE_SANITIZERS or ENABLE_COVERAGE is on. include(Sanitizers) +include(Coverage) # GNU systems need to define the Mersenne exponent for the RNG to compile w/o warning if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") diff --git a/cmake/Coverage.cmake b/cmake/Coverage.cmake new file mode 100644 index 000000000..0345a2d36 --- /dev/null +++ b/cmake/Coverage.cmake @@ -0,0 +1,75 @@ +# Line-coverage instrumentation opt-in, plus per-source opt-out +# Input: ENABLE_COVERAGE +# Defines: cockatrice_disable_coverage_for +# +# Included once from the root CMakeLists.txt, immediately after the per-compiler flag +# block, so add_compile_options() reaches every library and executable defined below. +# +# Note the name deliberately shadows CMake's own built-in Modules/Coverage.cmake. It resolves +# to this file only because the root CMakeLists.txt puts cmake/ at the front of +# CMAKE_MODULE_PATH; if that ever moves, include(Coverage) would silently pick up the built-in +# module, which only defines a `COVERAGE` custom target and touches no global flags, so +# instrumentation would quietly stop happening rather than erroring. + +include(CheckCXXSourceCompiles) + +# Opt a set of vendored sources out of instrumentation. +# +# cockatrice_disable_coverage_for( ...) +# +# Third-party code drags the total down with lines no test can ever reach, and a total that +# cannot move is not a useful signal. This is the same carve-out the sanitizer build uses. +# +# The sources must be named relative to the directory that compiles them. CMake resolves a +# source-file property against the directory scope the target was declared in, so calling +# this from the root with absolute paths silently does nothing. That is why the call sites +# live next to the add_library/add_executable that pull the vendored code in. +# +# SKIP_PRECOMPILE_HEADERS mirrors the sanitizer helper. The precompiled header carries the +# instrumentation flags, and mixing an instrumented PCH with an uninstrumented translation +# unit makes GCC reject it. +function(cockatrice_disable_coverage_for) + if(NOT ENABLE_COVERAGE) + return() + endif() + # There is no -fno-coverage: --coverage is shorthand for -fprofile-arcs -ftest-coverage, so both + # halves have to be negated. CC rejects "-fno-coverage" outright, which is how the vendored C + # files below hit it first. + set_source_files_properties( + ${ARGN} PROPERTIES COMPILE_OPTIONS "-fno-profile-arcs;-fno-test-coverage" SKIP_PRECOMPILE_HEADERS ON + ) +endfunction() + +if(ENABLE_COVERAGE) + # Line coverage needs the compiler's own toolchain, so only GCC is supported. Clang's + # equivalent is llvm-cov, which reads a different intermediate format and would need a + # separate report path; rejecting it is better than emitting .gcno files that gcovr cannot + # read. + if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + message(FATAL_ERROR "ENABLE_COVERAGE requires GCC, but the compiler is ${CMAKE_CXX_COMPILER_ID}. " + "Clang coverage needs llvm-cov and is not wired up." + ) + endif() + + # Probe the way the flag will actually be used: compile AND link. A .gcno file comes from the + # compile step, but the runtime that writes .gcda files comes from the link step, so probing + # only the compile step would pass on a toolchain that cannot actually produce coverage. + set(_cockatrice_saved_required_flags "${CMAKE_REQUIRED_FLAGS}") + set(_cockatrice_saved_required_link_options "${CMAKE_REQUIRED_LINK_OPTIONS}") + set(CMAKE_REQUIRED_FLAGS "-O0 -g") + set(CMAKE_REQUIRED_LINK_OPTIONS -fprofile-arcs -ftest-coverage) + check_cxx_source_compiles("int main() { return 0; }" COCKATRICE_HAVE_COVERAGE_FLAGS) + set(CMAKE_REQUIRED_FLAGS "${_cockatrice_saved_required_flags}") + set(CMAKE_REQUIRED_LINK_OPTIONS "${_cockatrice_saved_required_link_options}") + + if(NOT COCKATRICE_HAVE_COVERAGE_FLAGS) + message(FATAL_ERROR "ENABLE_COVERAGE is on, but this toolchain rejected the coverage flags.") + endif() + + # -O0 keeps the line mapping honest: at higher optimisation levels the compiler merges and + # relocates lines, and the report describes code that no longer exists in that form. + add_compile_options(--coverage -O0 -g) + add_link_options(--coverage) + + message(STATUS "Coverage instrumentation enabled (--coverage). Report with gcovr over the build dir.") +endif() diff --git a/gcovr.cfg b/gcovr.cfg new file mode 100644 index 000000000..ac893b6a4 --- /dev/null +++ b/gcovr.cfg @@ -0,0 +1,63 @@ +# gcovr configuration for Cockatrice line coverage. +# +# Used by .ci/compile.sh --coverage and by hand: +# +# cmake -S . -B build/cover -DCMAKE_BUILD_TYPE=Debug -DWITH_SERVER=1 -DTEST=1 -DENABLE_COVERAGE=ON +# cmake --build build/cover -j 24 +# ctest --test-dir build/cover -LE bench -j 24 +# gcovr build/cover --root . --object-directory build/cover --txt --decisions +# +# The point of most of this file is subtraction. A raw report is dominated by code no test can +# reach: vendored third-party trees, build directories left over from earlier experiments, and +# the client's main() and window classes, which have no headless test. Those all dilute the one +# number worth watching, so they are filtered out here rather than in every invocation. + +# root is deliberately not set here. This file is normally invoked from the build directory +# (compile.sh runs there, because it configures with `cmake ..`), so a `root` of `.` would +# resolve to the build tree and every filter below would exclude everything. compile.sh passes +# --root explicitly instead. + +# Our own code only. This keeps generated Qt MOC output, protobuf output and anything CMake +# built under the source tree out of the report. +filter = ^cockatrice/ +filter = ^libcockatrice_[^/]+/ +filter = ^servatrice/ +filter = ^oracle/ + +# The tests are the vehicle, not the subject. They are compiled with the same instrumentation, +# so leaving them in would add their near-perfect coverage to the numerator and quietly report a +# healthier project than we have: test code is only ever executed by the run that is measuring +# it. Helpers under tests/ are excluded on the same grounds. +exclude = ^tests/ + +# Vendored third-party code is not ours to cover and is never going to move the number. +exclude = ^vcpkg/ +exclude = ^doc/doxygen/theme/ +exclude = ^libcockatrice_rng/libcockatrice/rng/sfmt/ +exclude = ^libcockatrice_utility/libcockatrice/utility/peglib\.h$ +exclude = ^oracle/src/lzma/ +exclude = ^oracle/src/qt-json/ +exclude = ^oracle/src/zip/ +exclude = ^servatrice/src/smtp/ + +# Qt's own moc/pch/uic output is generated into the build tree, not the source tree, so the +# filters above already keep it out. These cover CLion's directories, which sit in-tree and are +# picked up by the object-directory walk. +exclude = .*cmake-build-.* +exclude = .*\.cache/ + +# Process entry points and the client's window chrome. Each of these needs a display, an +# interactive terminal, or a live database before it does anything a test could observe, so no +# headless run will ever move them. Left in, they contribute thousands of lines at 0% and drag +# the total down by a fixed amount that hides movement in the code we can actually test. The +# window header is excluded with its .cpp because gcovr attributes its inline template code +# there, which is where most of its line count lives. +exclude = ^cockatrice/src/main\.cpp$ +exclude = ^cockatrice/src/interface/window_main\. +exclude = ^oracle/src/main\.cpp$ +exclude = ^servatrice/src/main\.cpp$ + +exclude-throw-branches = yes +exclude-function-lines = yes + +print-summary = yes diff --git a/libcockatrice_rng/CMakeLists.txt b/libcockatrice_rng/CMakeLists.txt index 16beffcfe..6128e3f6e 100644 --- a/libcockatrice_rng/CMakeLists.txt +++ b/libcockatrice_rng/CMakeLists.txt @@ -13,6 +13,7 @@ add_library( # SFMT is vendored and its own headers trip UBSan's alignment and vptr checks. cockatrice_disable_sanitizers_for(libcockatrice/rng/sfmt/SFMT.c) +cockatrice_disable_coverage_for(libcockatrice/rng/sfmt/SFMT.c) target_include_directories(libcockatrice_rng PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) diff --git a/oracle/CMakeLists.txt b/oracle/CMakeLists.txt index 9b6863553..0df345812 100644 --- a/oracle/CMakeLists.txt +++ b/oracle/CMakeLists.txt @@ -81,6 +81,7 @@ if(ZLIB_FOUND) list(APPEND oracle_SOURCES src/zip/unzip.cpp src/zip/zipglobal.cpp) # minizip and zlib are vendored. cockatrice_disable_sanitizers_for(src/zip/unzip.cpp src/zip/zipglobal.cpp) + cockatrice_disable_coverage_for(src/zip/unzip.cpp src/zip/zipglobal.cpp) else() message(STATUS "Oracle: zlib not found; ZIP support disabled") endif() @@ -93,6 +94,7 @@ if(LIBLZMA_FOUND) list(APPEND oracle_SOURCES src/lzma/decompress.cpp) # The LZMA SDK is vendored. cockatrice_disable_sanitizers_for(src/lzma/decompress.cpp) + cockatrice_disable_coverage_for(src/lzma/decompress.cpp) else() message(STATUS "Oracle: LibLZMA not found; xz support disabled") endif() diff --git a/servatrice/CMakeLists.txt b/servatrice/CMakeLists.txt index b176d8009..b45608d86 100644 --- a/servatrice/CMakeLists.txt +++ b/servatrice/CMakeLists.txt @@ -31,6 +31,9 @@ set(servatrice_RESOURCES servatrice.qrc) cockatrice_disable_sanitizers_for( src/smtp/qxthmac.cpp src/smtp/qxtmailattachment.cpp src/smtp/qxtmailmessage.cpp src/smtp/qxtsmtp.cpp ) +cockatrice_disable_coverage_for( + src/smtp/qxthmac.cpp src/smtp/qxtmailattachment.cpp src/smtp/qxtmailmessage.cpp src/smtp/qxtsmtp.cpp +) if(WIN32) set(servatrice_SOURCES ${servatrice_SOURCES} servatrice.rc) diff --git a/tests/oracle/CMakeLists.txt b/tests/oracle/CMakeLists.txt index c896c7960..6958673f7 100644 --- a/tests/oracle/CMakeLists.txt +++ b/tests/oracle/CMakeLists.txt @@ -26,6 +26,7 @@ if(ZLIB_FOUND) add_definitions("-DHAS_ZLIB") set(_ORACLE_BENCH_EXTRA_SOURCES ../../oracle/src/zip/unzip.cpp ../../oracle/src/zip/zipglobal.cpp) cockatrice_disable_sanitizers_for(${_ORACLE_BENCH_EXTRA_SOURCES}) + cockatrice_disable_coverage_for(${_ORACLE_BENCH_EXTRA_SOURCES}) set(_ORACLE_BENCH_EXTRA_LIBRARIES ${ZLIB_LIBRARIES}) include_directories(${ZLIB_INCLUDE_DIRS}) else() @@ -37,6 +38,7 @@ if(LIBLZMA_FOUND) add_definitions("-DHAS_LZMA") list(APPEND _ORACLE_BENCH_EXTRA_SOURCES ../../oracle/src/lzma/decompress.cpp) cockatrice_disable_sanitizers_for(../../oracle/src/lzma/decompress.cpp) + cockatrice_disable_coverage_for(../../oracle/src/lzma/decompress.cpp) list(APPEND _ORACLE_BENCH_EXTRA_LIBRARIES ${LIBLZMA_LIBRARIES}) include_directories(${LIBLZMA_INCLUDE_DIRS}) else() From 315cbae345894b805a7f5a4ae8916c01feeb12a0 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:07:30 +0200 Subject: [PATCH 03/15] [Build] Extend PCH to libraries and grow IWYU-driven header sets (#7456) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The executables precompile Qt since #7235, but the libcockatrice_* static libraries still reparse the full Qt Core stack in every TU. Measured over the tree after the IWYU pass: 380 MB of header parse in the libraries, 92% of it reachable from qtcore_pch.h. - Apply qtcore_pch.h to the 14 Qt Core static library targets (skips libcockatrice_rng: its SFMT.c is C and cannot use a C++ precompiled header). - Grow qtcore_pch.h with 25 QtCore headers that IWYU shows are re-parsed across many TUs and were not already reachable from the existing list (JSON/QUuid cluster, QSettings, item models, threading, XmlStream, QTemporaryDir, ...). - Grow qtwidgets_pch.h with 30 QtGui/QtWidgets headers (dialog cluster, item views, QImageReader, ...). Only stable Qt headers go into the PCH, so it rarely invalidates. Test executables stay PCH-less: 1-3 TUs per target does not amortize the extra precompile. Co-authored-by: Lukas Brübach --- cmake/pch/qtcore_pch.h | 31 +++++++++++++++++++ cmake/pch/qtwidgets_pch.h | 29 +++++++++++++++++ libcockatrice_card/CMakeLists.txt | 2 ++ libcockatrice_deck_list/CMakeLists.txt | 2 ++ libcockatrice_filters/CMakeLists.txt | 2 ++ libcockatrice_interfaces/CMakeLists.txt | 2 ++ .../models/database/CMakeLists.txt | 2 ++ .../models/deck_list/CMakeLists.txt | 2 ++ .../network/client/abstract/CMakeLists.txt | 2 ++ .../network/client/local/CMakeLists.txt | 2 ++ .../network/client/remote/CMakeLists.txt | 2 ++ .../network/server/local/CMakeLists.txt | 2 ++ .../network/server/remote/CMakeLists.txt | 2 ++ libcockatrice_protocol/CMakeLists.txt | 2 ++ libcockatrice_settings/CMakeLists.txt | 2 ++ libcockatrice_utility/CMakeLists.txt | 2 ++ 16 files changed, 88 insertions(+) diff --git a/cmake/pch/qtcore_pch.h b/cmake/pch/qtcore_pch.h index cc3dd12ee..176f2939c 100644 --- a/cmake/pch/qtcore_pch.h +++ b/cmake/pch/qtcore_pch.h @@ -3,22 +3,53 @@ * * Safe for every target that links Qt Core, including the headless * Servatrice binary. Keep this header free of any widget/gui types. + * + * Candidates are chosen from IWYU include statistics: a header earns a + * spot when it is not already reachable from the rest of this list and + * enough translation units re-parse it. Qt headers are stable across + * builds, so the precompiled header rarely invalidates. */ #include #include +#include +#include +#include #include #include +#include +#include #include +#include #include +#include +#include +#include +#include +#include #include #include #include +#include #include +#include +#include #include +#include +#include #include +#include +#include +#include +#include #include +#include #include #include +#include +#include #include +#include #include +#include +#include diff --git a/cmake/pch/qtwidgets_pch.h b/cmake/pch/qtwidgets_pch.h index 2c63f450e..030bec69c 100644 --- a/cmake/pch/qtwidgets_pch.h +++ b/cmake/pch/qtwidgets_pch.h @@ -4,27 +4,56 @@ * Includes the Qt Core precompiled header plus the heavy Gui, Widgets and * Network layers that virtually every client translation unit re-parses. * Do not use on Servatrice (headless, QT_DONT_USE_QTGUI). + * + * Candidates are chosen from IWYU include statistics: a header earns a + * spot when it is not already reachable from the rest of this list and + * enough translation units re-parse it. Qt headers are stable across + * builds, so the precompiled header rarely invalidates. */ #include "qtcore_pch.h" #include #include +#include +#include +#include +#include +#include +#include +#include #include #include #include +#include #include +#include +#include +#include +#include #include +#include +#include #include #include +#include #include #include +#include #include #include #include +#include #include #include +#include +#include +#include #include +#include +#include #include +#include #include +#include #include diff --git a/libcockatrice_card/CMakeLists.txt b/libcockatrice_card/CMakeLists.txt index 15388ecfc..b3583522c 100644 --- a/libcockatrice_card/CMakeLists.txt +++ b/libcockatrice_card/CMakeLists.txt @@ -60,3 +60,5 @@ target_link_libraries( PUBLIC libcockatrice_utility PUBLIC ${QT_CORE_MODULE} ) + +target_precompile_headers(libcockatrice_card PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_deck_list/CMakeLists.txt b/libcockatrice_deck_list/CMakeLists.txt index 0c487466c..e61a5702c 100644 --- a/libcockatrice_deck_list/CMakeLists.txt +++ b/libcockatrice_deck_list/CMakeLists.txt @@ -41,3 +41,5 @@ target_link_libraries( libcockatrice_deck_list PUBLIC libcockatrice_interfaces libcockatrice_protocol libcockatrice_utility ${QT_CORE_MODULE} ) + +target_precompile_headers(libcockatrice_deck_list PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_filters/CMakeLists.txt b/libcockatrice_filters/CMakeLists.txt index 905533df8..2d45b8e5b 100644 --- a/libcockatrice_filters/CMakeLists.txt +++ b/libcockatrice_filters/CMakeLists.txt @@ -18,3 +18,5 @@ add_dependencies(libcockatrice_filters libcockatrice_card) target_include_directories(libcockatrice_filters PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(libcockatrice_filters PUBLIC libcockatrice_card ${QT_CORE_MODULE}) + +target_precompile_headers(libcockatrice_filters PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_interfaces/CMakeLists.txt b/libcockatrice_interfaces/CMakeLists.txt index f53c41807..3b17569b1 100644 --- a/libcockatrice_interfaces/CMakeLists.txt +++ b/libcockatrice_interfaces/CMakeLists.txt @@ -31,3 +31,5 @@ add_library(libcockatrice_interfaces STATIC ${MOC_SOURCES}) target_include_directories(libcockatrice_interfaces PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(libcockatrice_interfaces PUBLIC libcockatrice_utility ${QT_CORE_MODULE}) + +target_precompile_headers(libcockatrice_interfaces PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_models/libcockatrice/models/database/CMakeLists.txt b/libcockatrice_models/libcockatrice/models/database/CMakeLists.txt index 840f14b08..a73e36dbb 100644 --- a/libcockatrice_models/libcockatrice/models/database/CMakeLists.txt +++ b/libcockatrice_models/libcockatrice/models/database/CMakeLists.txt @@ -29,3 +29,5 @@ add_library( target_include_directories(libcockatrice_models_database PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(libcockatrice_models_database PUBLIC libcockatrice_card libcockatrice_filters ${QT_CORE_MODULE}) + +target_precompile_headers(libcockatrice_models_database PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_models/libcockatrice/models/deck_list/CMakeLists.txt b/libcockatrice_models/libcockatrice/models/deck_list/CMakeLists.txt index a6ab2a204..324bed102 100644 --- a/libcockatrice_models/libcockatrice/models/deck_list/CMakeLists.txt +++ b/libcockatrice_models/libcockatrice/models/deck_list/CMakeLists.txt @@ -16,3 +16,5 @@ target_include_directories(libcockatrice_models_deck_list PUBLIC ${CMAKE_CURRENT target_link_libraries( libcockatrice_models_deck_list PUBLIC libcockatrice_card libcockatrice_deck_list ${QT_CORE_MODULE} ) + +target_precompile_headers(libcockatrice_models_deck_list PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt index 6fba8d629..53ad524b4 100644 --- a/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt @@ -19,3 +19,5 @@ target_link_libraries( PUBLIC ${COCKATRICE_QT_VERSION_NAME}::Network ${COCKATRICE_QT_VERSION_NAME}::WebSockets libcockatrice_protocol libcockatrice_network_server_remote ) + +target_precompile_headers(libcockatrice_network_client_abstract PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/client/local/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/client/local/CMakeLists.txt index 2ac12e1fe..08c12b7f8 100644 --- a/libcockatrice_network/libcockatrice/network/client/local/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/client/local/CMakeLists.txt @@ -19,3 +19,5 @@ target_link_libraries( PUBLIC ${COCKATRICE_QT_VERSION_NAME}::Network ${COCKATRICE_QT_VERSION_NAME}::WebSockets libcockatrice_network_client_abstract ) + +target_precompile_headers(libcockatrice_network_client_local PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/client/remote/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/client/remote/CMakeLists.txt index cb68d0c37..c32ca0aa8 100644 --- a/libcockatrice_network/libcockatrice/network/client/remote/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/client/remote/CMakeLists.txt @@ -19,3 +19,5 @@ target_link_libraries( PUBLIC ${COCKATRICE_QT_VERSION_NAME}::Network ${COCKATRICE_QT_VERSION_NAME}::WebSockets libcockatrice_network_client_abstract libcockatrice_interfaces libcockatrice_utility libcockatrice_protocol ) + +target_precompile_headers(libcockatrice_network_client_remote PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/server/local/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/server/local/CMakeLists.txt index 494ae1294..020753d7f 100644 --- a/libcockatrice_network/libcockatrice/network/server/local/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/server/local/CMakeLists.txt @@ -15,3 +15,5 @@ add_dependencies(libcockatrice_network_server_local libcockatrice_protocol) target_include_directories(libcockatrice_network_server_local PUBLIC .) target_link_libraries(libcockatrice_network_server_local PUBLIC ${COCKATRICE_QT_MODULES} libcockatrice_protocol) + +target_precompile_headers(libcockatrice_network_server_local PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt index e11a962d1..5840f80c0 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/server/remote/CMakeLists.txt @@ -62,3 +62,5 @@ target_link_libraries( libcockatrice_network_server_remote PUBLIC libcockatrice_protocol libcockatrice_utility libcockatrice_rng libcockatrice_deck_list ${COCKATRICE_QT_MODULES} ) + +target_precompile_headers(libcockatrice_network_server_remote PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_protocol/CMakeLists.txt b/libcockatrice_protocol/CMakeLists.txt index 7dc0a0360..55184045a 100644 --- a/libcockatrice_protocol/CMakeLists.txt +++ b/libcockatrice_protocol/CMakeLists.txt @@ -24,3 +24,5 @@ target_include_directories( libcockatrice_protocol PUBLIC ${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR} # points to the generated headers ) + +target_precompile_headers(libcockatrice_protocol PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_settings/CMakeLists.txt b/libcockatrice_settings/CMakeLists.txt index 80a465b6d..5a2fa42a9 100644 --- a/libcockatrice_settings/CMakeLists.txt +++ b/libcockatrice_settings/CMakeLists.txt @@ -71,3 +71,5 @@ target_include_directories( ) target_link_libraries(libcockatrice_settings PUBLIC libcockatrice_card libcockatrice_utility ${QT_CORE_MODULE}) + +target_precompile_headers(libcockatrice_settings PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_utility/CMakeLists.txt b/libcockatrice_utility/CMakeLists.txt index d986214d7..5e6fa986d 100644 --- a/libcockatrice_utility/CMakeLists.txt +++ b/libcockatrice_utility/CMakeLists.txt @@ -49,3 +49,5 @@ target_link_libraries(libcockatrice_utility PUBLIC libcockatrice_rng OpenSSL::Cr set(ORACLE_LIBS) include_directories(${${COCKATRICE_QT_VERSION_NAME}Core_INCLUDE_DIRS}) + +target_precompile_headers(libcockatrice_utility PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") From 4a010bd17f619a73b47e71f89d66d101ded2d4b7 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:28:45 +0200 Subject: [PATCH 04/15] [Report] Add report reopen action for closed reports (#7442) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - New Command_ReportReopen moderator command (extension 1206) with server handler that reopens resolved/dismissed reports, resets resolution state and notified, and records the reopening note in the audit log - Reopen... button in the report queue action bar, gated to resolved and dismissed reports, with optional note prompt and tooltip - Success confirmations for Assign, Resolve/Dismiss and Reopen now survive the list refresh via a pending status message instead of being clobbered - Dispatch/auth coverage in server_moderator_role_test Co-authored-by: Lukas Brübach --- .../src/interface/widgets/tabs/tab_report.cpp | 57 ++++++- .../src/interface/widgets/tabs/tab_report.h | 4 + .../libcockatrice/protocol/pb/CMakeLists.txt | 1 + .../protocol/pb/command_report_reopen.proto | 10 ++ .../protocol/pb/moderator_commands.proto | 1 + servatrice/src/serversocketinterface.cpp | 39 +++++ servatrice/src/serversocketinterface.h | 2 + tests/CMakeLists.txt | 12 ++ tests/server_moderator_role_test.cpp | 156 ++++++++++++++++++ 9 files changed, 280 insertions(+), 2 deletions(-) create mode 100644 libcockatrice_protocol/libcockatrice/protocol/pb/command_report_reopen.proto create mode 100644 tests/server_moderator_role_test.cpp diff --git a/cockatrice/src/interface/widgets/tabs/tab_report.cpp b/cockatrice/src/interface/widgets/tabs/tab_report.cpp index 5f158e9b1..5fe656f44 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_report.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_report.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -233,12 +234,14 @@ TabReport::TabReport(TabSupervisor *_tabSupervisor, AbstractClient *_client) : T resolveButton = new QPushButton; resolveWithNoteButton = new QPushButton; dismissButton = new QPushButton; + reopenButton = new QPushButton; viewReplayButton = new QPushButton; joinGameButton = new QPushButton; connect(assignButton, &QPushButton::clicked, this, &TabReport::assignReport); connect(resolveButton, &QPushButton::clicked, this, [this]() { resolveReport(false, false); }); connect(resolveWithNoteButton, &QPushButton::clicked, this, [this]() { resolveReport(false, true); }); connect(dismissButton, &QPushButton::clicked, this, [this]() { resolveReport(true, true); }); + connect(reopenButton, &QPushButton::clicked, this, &TabReport::reopenReport); connect(viewReplayButton, &QPushButton::clicked, this, &TabReport::viewReplay); connect(joinGameButton, &QPushButton::clicked, this, &TabReport::joinGame); @@ -249,6 +252,7 @@ TabReport::TabReport(TabSupervisor *_tabSupervisor, AbstractClient *_client) : T actionBar->addWidget(resolveButton); actionBar->addWidget(resolveWithNoteButton); actionBar->addWidget(dismissButton); + actionBar->addWidget(reopenButton); actionBar->addSpacing(20); actionBar->addWidget(viewReplayButton); actionBar->addWidget(joinGameButton); @@ -315,6 +319,8 @@ void TabReport::retranslateUi() resolveButton->setText(tr("Resolve")); resolveWithNoteButton->setText(tr("Resolve with note...")); dismissButton->setText(tr("Dismiss...")); + reopenButton->setText(tr("Reopen...")); + reopenButton->setToolTip(tr("Available for resolved or dismissed reports.")); viewReplayButton->setText(tr("View Replay")); joinGameButton->setText(tr("Join Game")); } @@ -346,6 +352,7 @@ void TabReport::reportListResponse(const Response &response) refreshButton->setEnabled(true); if (response.response_code() != Response::RespOk) { + pendingStatusMessage.clear(); statusLabel->setText(tr("Failed to load reports.")); return; } @@ -359,6 +366,11 @@ void TabReport::reportListResponse(const Response &response) applyFilters(); updateStats(); + if (!pendingStatusMessage.isEmpty()) { + statusLabel->setText(pendingStatusMessage); + pendingStatusMessage.clear(); + } + if (selectedReportIdBeforeRefresh >= 0) { bool found = false; for (int row = 0; row < table->rowCount(); ++row) { @@ -579,6 +591,7 @@ void TabReport::updateActionStates() resolveButton->setEnabled(status == "open" || status == "assigned"); resolveWithNoteButton->setEnabled(status == "open" || status == "assigned"); dismissButton->setEnabled(status == "open" || status == "assigned"); + reopenButton->setEnabled(status == "resolved" || status == "dismissed"); const bool hasGameId = report.game_id() > 0; const bool hasReplay = hasGameId && report.has_replay_id() && report.replay_id() > 0; @@ -592,6 +605,7 @@ void TabReport::setActionsEnabled(bool enabled) resolveButton->setEnabled(enabled); resolveWithNoteButton->setEnabled(enabled); dismissButton->setEnabled(enabled); + reopenButton->setEnabled(enabled); viewReplayButton->setEnabled(false); joinGameButton->setEnabled(false); commentButton->setEnabled(enabled); @@ -645,7 +659,7 @@ void TabReport::assignReport() void TabReport::assignResponse(const Response &response) { if (response.response_code() == Response::RespOk) { - statusLabel->setText(tr("Assigned.")); + pendingStatusMessage = tr("Assigned."); refreshList(); } else { statusLabel->setText(tr("Assignment failed.")); @@ -689,7 +703,46 @@ void TabReport::resolveReport(bool dismissed, bool promptNote) void TabReport::resolveResponse(const Response &response) { if (response.response_code() == Response::RespOk) { - statusLabel->setText(tr("Done.")); + pendingStatusMessage = tr("Done."); + refreshList(); + } else { + statusLabel->setText(tr("Action failed.")); + updateActionStates(); + } +} + +void TabReport::reopenReport() +{ + const int reportId = selectedReportId(); + if (reportId < 0) { + return; + } + + bool ok; + QString note = QInputDialog::getText(this, tr("Reopen Report"), tr("Reopening note (optional):"), QLineEdit::Normal, + QString(), &ok); + if (!ok) { + return; + } + + setActionsEnabled(false); + statusLabel->setText(tr("Reopening...")); + + Command_ReportReopen cmd; + cmd.set_report_id(reportId); + if (!note.isEmpty()) { + cmd.set_reopen_note(note.toStdString()); + } + + PendingCommand *pend = client->prepareModeratorCommand(cmd); + connect(pend, &PendingCommand::finished, this, &TabReport::reopenResponse); + client->sendCommand(pend); +} + +void TabReport::reopenResponse(const Response &response) +{ + if (response.response_code() == Response::RespOk) { + pendingStatusMessage = tr("Reopened."); refreshList(); } else { statusLabel->setText(tr("Action failed.")); diff --git a/cockatrice/src/interface/widgets/tabs/tab_report.h b/cockatrice/src/interface/widgets/tabs/tab_report.h index 645f949de..6ea8199ce 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_report.h +++ b/cockatrice/src/interface/widgets/tabs/tab_report.h @@ -56,6 +56,8 @@ private slots: void reportDetailsResponse(const Response &response); void requestStats(); void statsResponse(const Response &response); + void reopenReport(); + void reopenResponse(const Response &response); private: int selectedReportId() const; @@ -87,6 +89,7 @@ private: QPushButton *resolveButton; QPushButton *resolveWithNoteButton; QPushButton *dismissButton; + QPushButton *reopenButton; QPushButton *viewReplayButton; QPushButton *joinGameButton; QLabel *statusLabel; @@ -108,6 +111,7 @@ private: QLabel *userContextNotesLabel; QLabel *userContextRecentReportsLabel; QString lastRequestedUser; + QString pendingStatusMessage; QGroupBox *statsGroup; QLabel *statsTotalLabel; QLabel *statsTrendLabel; diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/CMakeLists.txt b/libcockatrice_protocol/libcockatrice/protocol/pb/CMakeLists.txt index bc4814d5e..4e4f0e190 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/CMakeLists.txt +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/CMakeLists.txt @@ -55,6 +55,7 @@ set(PROTO_FILES command_report_details.proto command_report_list.proto command_report_my_list.proto + command_report_reopen.proto command_report_resolve.proto command_report_stats.proto command_report_user_info.proto diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/command_report_reopen.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/command_report_reopen.proto new file mode 100644 index 000000000..1f9c707e8 --- /dev/null +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/command_report_reopen.proto @@ -0,0 +1,10 @@ +syntax = "proto2"; +import "moderator_commands.proto"; + +message Command_ReportReopen { + extend ModeratorCommand { + optional Command_ReportReopen ext = 1206; + } + required int32 report_id = 1; + optional string reopen_note = 2; +} diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/moderator_commands.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/moderator_commands.proto index 4f1e80c27..5298d3dad 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/moderator_commands.proto +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/moderator_commands.proto @@ -26,6 +26,7 @@ message ModeratorCommand { REPLAY_DOWNLOAD_BY_GAME_ID = 1203; REPORT_USER_INFO = 1204; REPORT_STATS = 1205; + REPORT_REOPEN = 1206; } extensions 100 to max; } diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index d46a9b0ee..8fa070ce7 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -110,6 +110,7 @@ #include #include #include +#include #include #include #include @@ -434,6 +435,8 @@ Response::ResponseCode AbstractServerSocketInterface::processExtendedModeratorCo return cmdReportAssign(cmd.GetExtension(Command_ReportAssign::ext), rc); case ModeratorCommand::REPORT_RESOLVE: return cmdReportResolve(cmd.GetExtension(Command_ReportResolve::ext), rc); + case ModeratorCommand::REPORT_REOPEN: + return cmdReportReopen(cmd.GetExtension(Command_ReportReopen::ext), rc); case ModeratorCommand::VIEWLOG_HISTORY: return cmdGetLogHistory(cmd.GetExtension(Command_ViewLogHistory::ext), rc, true); case ModeratorCommand::GRANT_REPLAY_ACCESS: @@ -2791,6 +2794,42 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportMyList(const Comm return Response::RespOk; } +Response::ResponseCode AbstractServerSocketInterface::cmdReportReopen(const Command_ReportReopen &cmd, + ResponseContainer & /*rc*/) +{ + if (!sqlInterface->checkSql()) { + return Response::RespInternalError; + } + + int reportId = cmd.report_id(); + QString note = textFromStdString(cmd.reopen_note()); + + QSqlQuery *query = sqlInterface->prepareQuery("UPDATE {prefix}_reports " + "SET status = 'open', resolution_note = NULL, " + "resolution_time = NULL, resolved_by = NULL, assigned_to = NULL, " + "notified = 0 " + "WHERE id = :id AND status IN ('resolved', 'dismissed')"); + + query->bindValue(":id", reportId); + + if (!sqlInterface->execSqlQuery(query)) { + return Response::RespInternalError; + } + + if (query->numRowsAffected() == 0) { + return Response::RespInvalidData; + } + + QString auditDetails = QString("Report #%1 reopened").arg(reportId); + if (!note.isEmpty()) { + auditDetails += QString(". Reason: %1").arg(note.left(200)); + } + sqlInterface->addAuditRecord(QString::number(reportId), this->getAddress(), + QString::fromStdString(userInfo->clientid()), "REPORT_REOPENED", auditDetails, true); + + return Response::RespOk; +} + Response::ResponseCode AbstractServerSocketInterface::cmdReportDetails(const Command_ReportDetails &cmd, ResponseContainer &rc) { diff --git a/servatrice/src/serversocketinterface.h b/servatrice/src/serversocketinterface.h index 27708ed27..7de742c66 100644 --- a/servatrice/src/serversocketinterface.h +++ b/servatrice/src/serversocketinterface.h @@ -96,6 +96,7 @@ class Command_ReportAssign; class Command_ReportDetails; class Command_ReportList; class Command_ReportMyList; +class Command_ReportReopen; class Command_ReportResolve; class Command_ReportStats; class Command_ReportUserInfo; @@ -181,6 +182,7 @@ private: Response::ResponseCode cmdUpdateServerMessage(const Command_UpdateServerMessage &cmd, ResponseContainer &rc); Response::ResponseCode cmdReportAssign(const Command_ReportAssign &cmd, ResponseContainer &); Response::ResponseCode cmdReportResolve(const Command_ReportResolve &cmd, ResponseContainer &); + Response::ResponseCode cmdReportReopen(const Command_ReportReopen &cmd, ResponseContainer &); Response::ResponseCode cmdReportUserInfo(const Command_ReportUserInfo &cmd, ResponseContainer &rc); Response::ResponseCode cmdReportStats(const Command_ReportStats &cmd, ResponseContainer &rc); Response::ResponseCode cmdRegisterAccount(const Command_Register &cmd, ResponseContainer &rc); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index c48e6c957..22dbedb98 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -228,6 +228,18 @@ cockatrice_add_test( LABELS server ) +cockatrice_add_test( + NAME + server_moderator_role_test + NO_SANITIZE + SOURCES + server_moderator_role_test.cpp + LIBS + libcockatrice_network + libcockatrice_rng + LABELS + server +) cockatrice_add_test( NAME server_game_join_test diff --git a/tests/server_moderator_role_test.cpp b/tests/server_moderator_role_test.cpp new file mode 100644 index 000000000..2517a2d86 --- /dev/null +++ b/tests/server_moderator_role_test.cpp @@ -0,0 +1,156 @@ +/** @file server_moderator_role_test.cpp + * @brief Tests for the moderator staff role authorization and dispatch. + * @ingroup Tests + */ + +#include "libcockatrice/protocol/pb/response.pb.h" +#include "libcockatrice/protocol/pb/server_message.pb.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +// The server_remote library references the global RNG, which is normally +// defined by the servatrice/client executable main(). Provide a stub so the +// unit test can link against it. +RNG_Abstract *rng = nullptr; + +namespace +{ + +class TestModeratorHandler : public Server_ProtocolHandler +{ +public: + explicit TestModeratorHandler(Server *_server) : Server_ProtocolHandler(_server, nullptr) + { + } + + QString getAddress() const override + { + return {}; + } + QString getConnectionType() const override + { + return {}; + } + + // Buffer the last response code sent to the client so tests can assert on + // the outcome of processCommandContainer(). + Response::ResponseCode lastResponseCode = Response::RespNothing; + int dispatchCount = 0; + +protected: + void transmitProtocolItem(const ServerMessage &item) override + { + if (item.message_type() == ServerMessage::RESPONSE) { + lastResponseCode = item.response().response_code(); + } + } + + Response::ResponseCode + processExtendedModeratorCommand(int cmdType, const ModeratorCommand &, ResponseContainer &) override + { + ++dispatchCount; + // Fail closed for anything not explicitly handled. + if (cmdType != ModeratorCommand::REPORT_REOPEN) { + return Response::RespFunctionNotAllowed; + } + return Response::RespOk; + } +}; + +class ModeratorRoleTest : public ::testing::Test +{ +protected: + Server server; + TestModeratorHandler handler{&server}; + + void setUserLevel(uint32_t level) + { + ServerInfo_User user; + user.set_user_level(level); + handler.setUserInfo(user); + } +}; + +TEST_F(ModeratorRoleTest, RejectsWhenNotLoggedIn) +{ + CommandContainer cont; + cont.add_moderator_command(); + handler.processCommandContainer(cont); + EXPECT_EQ(handler.lastResponseCode, Response::RespLoginNeeded); + EXPECT_EQ(handler.dispatchCount, 0); +} + +TEST_F(ModeratorRoleTest, RejectsPlainUser) +{ + setUserLevel(ServerInfo_User::IsUser | ServerInfo_User::IsRegistered); + + CommandContainer cont; + cont.add_moderator_command(); + handler.processCommandContainer(cont); + EXPECT_EQ(handler.lastResponseCode, Response::RespLoginNeeded); + EXPECT_EQ(handler.dispatchCount, 0); +} + +TEST_F(ModeratorRoleTest, DispatchesToModeratorCommandForModerator) +{ + setUserLevel(ServerInfo_User::IsModerator); + + CommandContainer cont; + ModeratorCommand *cmd = cont.add_moderator_command(); + cmd->MutableExtension(Command_ReportReopen::ext); + handler.processCommandContainer(cont); + EXPECT_EQ(handler.lastResponseCode, Response::RespOk); + EXPECT_EQ(handler.dispatchCount, 1); +} + +TEST_F(ModeratorRoleTest, DispatchesToModeratorCommandForAdmin) +{ + setUserLevel(ServerInfo_User::IsModerator | ServerInfo_User::IsAdmin); + + CommandContainer cont; + ModeratorCommand *cmd = cont.add_moderator_command(); + cmd->MutableExtension(Command_ReportReopen::ext); + handler.processCommandContainer(cont); + EXPECT_EQ(handler.lastResponseCode, Response::RespOk); + EXPECT_EQ(handler.dispatchCount, 1); +} + +TEST_F(ModeratorRoleTest, RejectsDeveloperThatIsNotModerator) +{ + setUserLevel(ServerInfo_User::IsDeveloper); + + CommandContainer cont; + cont.add_moderator_command(); + handler.processCommandContainer(cont); + EXPECT_EQ(handler.lastResponseCode, Response::RespLoginNeeded); + EXPECT_EQ(handler.dispatchCount, 0); +} + +TEST_F(ModeratorRoleTest, FailClosedForUnknownModeratorCommand) +{ + setUserLevel(ServerInfo_User::IsModerator); + + CommandContainer cont; + cont.add_moderator_command(); // no extension set -> getPbExtension() returns -1 + handler.processCommandContainer(cont); + EXPECT_EQ(handler.lastResponseCode, Response::RespFunctionNotAllowed); + EXPECT_EQ(handler.dispatchCount, 1); +} + +} // namespace + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} From 74a6acde917830c100a89ee831e4d2557673b32d Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:29:09 +0200 Subject: [PATCH 05/15] [Report] Surface resolution audit info in report details (#7443) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add resolved_by_name to ServerInfo_Report and join resolved_by users in the report list, my-list and details queries - Render resolution note and resolved-at/by in the report detail pane instead of duplicating the description into the chat log context Co-authored-by: Lukas Brübach --- .../widgets/utility/report_utils.cpp | 21 ++++++- .../protocol/pb/serverinfo_report.proto | 1 + servatrice/src/serversocketinterface.cpp | 63 +++++++++++-------- 3 files changed, 58 insertions(+), 27 deletions(-) diff --git a/cockatrice/src/interface/widgets/utility/report_utils.cpp b/cockatrice/src/interface/widgets/utility/report_utils.cpp index a581c9837..d7fc083c6 100644 --- a/cockatrice/src/interface/widgets/utility/report_utils.cpp +++ b/cockatrice/src/interface/widgets/utility/report_utils.cpp @@ -100,10 +100,27 @@ void renderReportDetails(QTextEdit *chatLogEdit, const QString &moderatorPrefix, const QString &nonModeratorPrefix) { + QString details; + if (report.has_resolution_note() && !report.resolution_note().empty()) { + details = QObject::tr("Resolution note: %1").arg(QString::fromStdString(report.resolution_note())); + } + if (report.has_resolution_time()) { + QString resolvedBy = QObject::tr("unknown"); + if (report.has_resolved_by_name() && !report.resolved_by_name().empty()) { + resolvedBy = QString::fromStdString(report.resolved_by_name()); + } + if (!details.isEmpty()) { + details += "\n"; + } + details += QObject::tr("Resolved at %1 by %2").arg(formatReportTime(report.resolution_time()), resolvedBy); + } + if (report.has_chat_log() && !report.chat_log().empty()) { - chatLogEdit->setPlainText(QString::fromStdString(report.chat_log())); + const QString chatLog = QString::fromStdString(report.chat_log()); + chatLogEdit->setPlainText( + details.isEmpty() ? chatLog : details + "\n\n" + QObject::tr("=== Chat Log ===") + "\n" + chatLog); } else { - chatLogEdit->clear(); + chatLogEdit->setPlainText(details); } commentsEdit->clear(); diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto index a145fd855..e67b1f361 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto @@ -33,4 +33,5 @@ message ServerInfo_Report { optional string chat_log = 14; optional string resolution_note = 15; + optional string resolved_by_name = 16; } diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 8fa070ce7..177e945c2 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -2015,11 +2015,12 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportList(const Comman QString queryStr = "SELECT r.id, r.reporter_name, r.reported_user_name, r.game_id, " "r.category, r.description, r.created_at, r.status, " - "r.resolution_note, u.name AS assigned_mod_name, r.room_id, " + "r.resolution_note, u.name AS assigned_mod_name, ru.name AS resolved_by_name, r.room_id, " "(SELECT id FROM {prefix}_replays WHERE id_game = r.game_id ORDER BY id DESC LIMIT 1) " "AS replay_id " "FROM {prefix}_reports r " - "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id "; + "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id " + "LEFT JOIN {prefix}_users ru ON r.resolved_by = ru.id "; if (unresolvedOnly) { queryStr += "WHERE r.status = 'open' OR r.status = 'assigned' "; @@ -2060,13 +2061,16 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportList(const Comman if (!query->value(9).isNull()) { info->set_assigned_mod_name(query->value(9).toString().toStdString()); } - if (!query->value(10).isNull()) { - info->set_room_id(query->value(10).toInt()); + info->set_resolved_by_name(query->value(10).toString().toStdString()); } if (!query->value(11).isNull()) { - info->set_replay_id(query->value(11).toInt()); + info->set_room_id(query->value(11).toInt()); + } + + if (!query->value(12).isNull()) { + info->set_replay_id(query->value(12).toInt()); } } @@ -2740,11 +2744,12 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportMyList(const Comm QString queryStr = "SELECT r.id, r.reporter_name, r.reported_user_name, r.game_id, " "r.category, r.description, r.created_at, r.status, " - "r.resolution_note, u.name AS assigned_mod_name, r.room_id, " + "r.resolution_note, u.name AS assigned_mod_name, ru.name AS resolved_by_name, r.room_id, " "(SELECT id FROM {prefix}_replays WHERE id_game = r.game_id ORDER BY id DESC LIMIT 1) " "AS replay_id " "FROM {prefix}_reports r " "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id " + "LEFT JOIN {prefix}_users ru ON r.resolved_by = ru.id " "WHERE r.reporter_id = :reporter_id " "ORDER BY r.created_at DESC LIMIT 200"; @@ -2780,13 +2785,16 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportMyList(const Comm if (!query->value(9).isNull()) { info->set_assigned_mod_name(query->value(9).toString().toStdString()); } - if (!query->value(10).isNull()) { - info->set_room_id(query->value(10).toInt()); + info->set_resolved_by_name(query->value(10).toString().toStdString()); } if (!query->value(11).isNull()) { - info->set_replay_id(query->value(11).toInt()); + info->set_room_id(query->value(11).toInt()); + } + + if (!query->value(12).isNull()) { + info->set_replay_id(query->value(12).toInt()); } } @@ -2845,17 +2853,19 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportDetails(const Com // Columns: 0=id, 1=reporter_id, 2=reporter_name, 3=reported_user_name, 4=game_id, // 5=category, 6=description, 7=created_at, 8=status, - // 9=resolution_note, 10=assigned_mod_name, 11=chat_log, - // 12=room_id, 13=resolution_time, 14=replay_id - QString queryStr = "SELECT r.id, r.reporter_id, r.reporter_name, r.reported_user_name, r.game_id, " - "r.category, r.description, r.created_at, r.status, " - "r.resolution_note, u.name AS assigned_mod_name, r.chat_log, r.room_id, " - "r.resolution_time, " - "(SELECT id FROM {prefix}_replays WHERE id_game = r.game_id ORDER BY id DESC LIMIT 1) " - "AS replay_id " - "FROM {prefix}_reports r " - "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id " - "WHERE r.id = :id"; + // 9=resolution_note, 10=assigned_mod_name, 11=resolved_by_name, + // 12=chat_log, 13=room_id, 14=resolution_time, 15=replay_id + QString queryStr = + "SELECT r.id, r.reporter_id, r.reporter_name, r.reported_user_name, r.game_id, " + "r.category, r.description, r.created_at, r.status, " + "r.resolution_note, u.name AS assigned_mod_name, ru.name AS resolved_by_name, r.chat_log, r.room_id, " + "r.resolution_time, " + "(SELECT id FROM {prefix}_replays WHERE id_game = r.game_id ORDER BY id DESC LIMIT 1) " + "AS replay_id " + "FROM {prefix}_reports r " + "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id " + "LEFT JOIN {prefix}_users ru ON r.resolved_by = ru.id " + "WHERE r.id = :id"; QSqlQuery *query = sqlInterface->prepareQuery(queryStr); query->bindValue(":id", reportId); @@ -2894,21 +2904,24 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportDetails(const Com if (!query->value(10).isNull()) { info->set_assigned_mod_name(query->value(10).toString().toStdString()); } - if (!query->value(11).isNull()) { - info->set_chat_log(query->value(11).toString().toStdString()); + info->set_resolved_by_name(query->value(11).toString().toStdString()); } if (!query->value(12).isNull()) { - info->set_room_id(query->value(12).toInt()); + info->set_chat_log(query->value(12).toString().toStdString()); } if (!query->value(13).isNull()) { - info->set_resolution_time(query->value(13).toDateTime().toSecsSinceEpoch()); + info->set_room_id(query->value(13).toInt()); } if (!query->value(14).isNull()) { - info->set_replay_id(query->value(14).toInt()); + info->set_resolution_time(query->value(14).toDateTime().toSecsSinceEpoch()); + } + + if (!query->value(15).isNull()) { + info->set_replay_id(query->value(15).toInt()); } QSqlQuery *commentQuery = From 38acddeedefe2fdbdd172e7d7a5c637465904074 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:33:47 +0200 Subject: [PATCH 06/15] [Report] Add context menu to report queue rows (#7444) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Report] Add report reopen action for closed reports - New Command_ReportReopen moderator command (extension 1206) with server handler that reopens resolved/dismissed reports, resets resolution state and notified, and records the reopening note in the audit log - Reopen... button in the report queue action bar, gated to resolved and dismissed reports, with optional note prompt and tooltip - Success confirmations for Assign, Resolve/Dismiss and Reopen now survive the list refresh via a pending status message instead of being clobbered - Dispatch/auth coverage in server_moderator_role_test * [Report] Surface resolution audit info in report details - Add resolved_by_name to ServerInfo_Report and join resolved_by users in the report list, my-list and details queries - Render resolution note and resolved-at/by in the report detail pane instead of duplicating the description into the chat log context * [Report] Add context menu to report queue rows - Open the standard UserContextMenu on right-click/Menu key for the reported user, selecting the row first so the details panel matches the menu target - Carry the reported user's level in ServerInfo_Report so the menu shows the same actions as the user list, with an IsUser fallback for deleted or unregistered users - Wire the Private chat action through TabReport to TabSupervisor and retranslate the menu on live language switches * [Server] Fix reported_user_level column indices in report queries The SELECT lists resolved_by_name at index 10, shifting room_id to 11, replay_id to 12, reported_user_id to 13 and reported_user_admin to 14. The code read 12/13, so 'user exists' meant 'a replay exists' and the admin flags came from the user id (e.g. id 5 -> admin+judge; users without a replay reported as unregistered, hiding registered-only actions). Fixed in cmdReportList and cmdReportMyList, plus the column comment. --------- Co-authored-by: Lukas Brübach --- .../src/interface/widgets/tabs/tab_report.cpp | 37 +++++++++++++++++ .../src/interface/widgets/tabs/tab_report.h | 5 +++ .../interface/widgets/tabs/tab_supervisor.cpp | 1 + .../protocol/pb/serverinfo_report.proto | 2 + servatrice/src/serversocketinterface.cpp | 40 ++++++++++++++++--- 5 files changed, 80 insertions(+), 5 deletions(-) diff --git a/cockatrice/src/interface/widgets/tabs/tab_report.cpp b/cockatrice/src/interface/widgets/tabs/tab_report.cpp index 5fe656f44..3a31f9b52 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_report.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_report.cpp @@ -1,5 +1,6 @@ #include "tab_report.h" +#include "../server/user/user_context_menu.h" #include "../utility/report_utils.h" #include "abstract_client.h" #include "libcockatrice/protocol/pb/response.pb.h" @@ -33,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -121,6 +123,10 @@ TabReport::TabReport(TabSupervisor *_tabSupervisor, AbstractClient *_client) : T table->horizontalHeader()->setSectionResizeMode(COL_REPORTED, QHeaderView::Stretch); table->horizontalHeader()->setSectionResizeMode(COL_REPORTER, QHeaderView::ResizeToContents); connect(table, &QTableWidget::itemSelectionChanged, this, &TabReport::onSelectionChanged); + table->setContextMenuPolicy(Qt::CustomContextMenu); + connect(table, &QTableWidget::customContextMenuRequested, this, &TabReport::showTableContextMenu); + userContextMenu = new UserContextMenu(tabSupervisor, this); + connect(userContextMenu, &UserContextMenu::openMessageDialog, this, &TabReport::openMessageDialog); descGroup = new QGroupBox; descriptionEdit = new QTextEdit; @@ -276,6 +282,7 @@ TabReport::TabReport(TabSupervisor *_tabSupervisor, AbstractClient *_client) : T void TabReport::retranslateUi() { + userContextMenu->retranslateUi(); searchEdit->setPlaceholderText(tr("Search by username, category...")); statusFilter->clear(); statusFilter->addItem(tr("All Statuses"), ""); @@ -506,6 +513,36 @@ void TabReport::onSelectionChanged() updateActionStates(); } +void TabReport::showTableContextMenu(const QPoint &pos) +{ + QTableWidgetItem *clickedItem = table->itemAt(pos); + if (!clickedItem || clickedItem->row() < 0) { + return; + } + + const int row = clickedItem->row(); + const QTableWidgetItem *reportedItem = table->item(row, COL_REPORTED); + const QTableWidgetItem *idItem = table->item(row, COL_ID); + if (!reportedItem || !idItem || reportedItem->text().isEmpty()) { + return; + } + + int userLevel = ServerInfo_User::IsUser; + const int reportId = idItem->data(Qt::UserRole).toInt(); + for (const ServerInfo_Report &r : filteredReports) { + if (r.report_id() == reportId) { + if (r.reported_user_level() != 0) { + userLevel = r.reported_user_level(); + } + break; + } + } + + table->selectRow(row); + userContextMenu->showContextMenu(table->viewport()->mapToGlobal(pos), reportedItem->text(), + UserLevelFlags(userLevel)); +} + void TabReport::loadReportDetails(int reportId) { detailsRequestedReportId = reportId; diff --git a/cockatrice/src/interface/widgets/tabs/tab_report.h b/cockatrice/src/interface/widgets/tabs/tab_report.h index 6ea8199ce..ea686c239 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_report.h +++ b/cockatrice/src/interface/widgets/tabs/tab_report.h @@ -4,6 +4,7 @@ #include "tab.h" #include +#include #include #include #include @@ -22,6 +23,7 @@ class QTimer; class GameReplay; class Response; class TabSupervisor; +class UserContextMenu; class TabReport : public Tab { @@ -37,6 +39,7 @@ public: signals: void openReplay(GameReplay *replay); void requestJoinGame(int gameId, int roomId); + void openMessageDialog(const QString &userName, bool focus); private slots: void refreshList(); @@ -46,6 +49,7 @@ private slots: void resolveReport(bool dismissed, bool promptNote); void resolveResponse(const Response &response); void onSelectionChanged(); + void showTableContextMenu(const QPoint &pos); void viewReplay(); void viewReplayResponse(const Response &response); void joinGame(); @@ -94,6 +98,7 @@ private: QPushButton *joinGameButton; QLabel *statusLabel; QTimer *refreshTimer; + UserContextMenu *userContextMenu; QGroupBox *userContextGroup; QLabel *userContextName; diff --git a/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp b/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp index 70014ec47..2352d7d7b 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp @@ -919,6 +919,7 @@ void TabSupervisor::openTabReport() myAddTab(tabReport, aTabReport); connect(tabReport, &TabReport::openReplay, this, &TabSupervisor::openReplay); connect(tabReport, &TabReport::requestJoinGame, this, &TabSupervisor::joinReportGame); + connect(tabReport, &TabReport::openMessageDialog, this, &TabSupervisor::addMessageTab); connect(tabReport, &QObject::destroyed, this, [this] { tabReport = nullptr; aTabReport->setChecked(false); diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto index e67b1f361..3b624cc6d 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/serverinfo_report.proto @@ -34,4 +34,6 @@ message ServerInfo_Report { optional string resolution_note = 15; optional string resolved_by_name = 16; + + optional int32 reported_user_level = 17; } diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 177e945c2..5f5d1ced5 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -1986,6 +1986,29 @@ Response::ResponseCode AbstractServerSocketInterface::cmdBanFromServer(const Com return Response::RespOk; } +namespace +{ +int reportUserLevel(bool userExists, int adminFlags) +{ + int userLevel = ServerInfo_User::IsUser; + if (userExists) { + userLevel |= ServerInfo_User::IsRegistered; + if (adminFlags & 1) { + userLevel |= ServerInfo_User::IsAdmin | ServerInfo_User::IsModerator; + } else if (adminFlags & 2) { + userLevel |= ServerInfo_User::IsModerator; + } + if (adminFlags & 4) { + userLevel |= ServerInfo_User::IsJudge; + } + if (adminFlags & 8) { + userLevel |= ServerInfo_User::IsDeveloper; + } + } + return userLevel; +} +} // namespace + Response::ResponseCode AbstractServerSocketInterface::cmdReportList(const Command_ReportList &cmd, ResponseContainer &rc) { @@ -1999,8 +2022,9 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportList(const Comman // Columns: 0=id, 1=reporter_name, 2=reported_user_name, 3=game_id, // 4=category, 5=description, 6=created_at, 7=status, - // 8=resolution_note, 9=assigned_mod_name, - // 10=room_id, 11=replay_id + // 8=resolution_note, 9=assigned_mod_name, 10=resolved_by_name, + // 11=room_id, 12=replay_id, + // 13=reported_user_id, 14=reported_user_admin QString whereClause; if (unresolvedOnly) { whereClause = "WHERE r.status = 'open' OR r.status = 'assigned' "; @@ -2017,10 +2041,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportList(const Comman "r.category, r.description, r.created_at, r.status, " "r.resolution_note, u.name AS assigned_mod_name, ru.name AS resolved_by_name, r.room_id, " "(SELECT id FROM {prefix}_replays WHERE id_game = r.game_id ORDER BY id DESC LIMIT 1) " - "AS replay_id " + "AS replay_id, u2.id AS reported_user_id, u2.admin AS reported_user_admin " "FROM {prefix}_reports r " "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id " - "LEFT JOIN {prefix}_users ru ON r.resolved_by = ru.id "; + "LEFT JOIN {prefix}_users ru ON r.resolved_by = ru.id " + "LEFT JOIN {prefix}_users u2 ON u2.name = r.reported_user_name "; if (unresolvedOnly) { queryStr += "WHERE r.status = 'open' OR r.status = 'assigned' "; @@ -2072,6 +2097,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportList(const Comman if (!query->value(12).isNull()) { info->set_replay_id(query->value(12).toInt()); } + + info->set_reported_user_level(reportUserLevel(!query->value(13).isNull(), query->value(14).toInt())); } rc.setResponseExtension(re); @@ -2746,10 +2773,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportMyList(const Comm "r.category, r.description, r.created_at, r.status, " "r.resolution_note, u.name AS assigned_mod_name, ru.name AS resolved_by_name, r.room_id, " "(SELECT id FROM {prefix}_replays WHERE id_game = r.game_id ORDER BY id DESC LIMIT 1) " - "AS replay_id " + "AS replay_id, u2.id AS reported_user_id, u2.admin AS reported_user_admin " "FROM {prefix}_reports r " "LEFT JOIN {prefix}_users u ON r.assigned_to = u.id " "LEFT JOIN {prefix}_users ru ON r.resolved_by = ru.id " + "LEFT JOIN {prefix}_users u2 ON u2.name = r.reported_user_name " "WHERE r.reporter_id = :reporter_id " "ORDER BY r.created_at DESC LIMIT 200"; @@ -2796,6 +2824,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReportMyList(const Comm if (!query->value(12).isNull()) { info->set_replay_id(query->value(12).toInt()); } + + info->set_reported_user_level(reportUserLevel(!query->value(13).isNull(), query->value(14).toInt())); } rc.setResponseExtension(re); From 07eb9f0103b1ac8f353f337d376e8f606d73e87a Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:33:48 +0200 Subject: [PATCH 07/15] [Report] Prompt for a note on Resolve and drop the redundant option (#7445) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove the third close option (Resolve with note...) so closing a report is one of two clear actions - Make Resolve always prompt for the optional resolution note, matching Dismiss, and rename the button to Resolve... - Halve the resolveReport signature now that every caller prompts Co-authored-by: Lukas Brübach --- .../src/interface/widgets/tabs/tab_report.cpp | 29 +++++++------------ .../src/interface/widgets/tabs/tab_report.h | 3 +- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/cockatrice/src/interface/widgets/tabs/tab_report.cpp b/cockatrice/src/interface/widgets/tabs/tab_report.cpp index 3a31f9b52..9018e2477 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_report.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_report.cpp @@ -238,15 +238,13 @@ TabReport::TabReport(TabSupervisor *_tabSupervisor, AbstractClient *_client) : T assignButton = new QPushButton; resolveButton = new QPushButton; - resolveWithNoteButton = new QPushButton; dismissButton = new QPushButton; reopenButton = new QPushButton; viewReplayButton = new QPushButton; joinGameButton = new QPushButton; connect(assignButton, &QPushButton::clicked, this, &TabReport::assignReport); - connect(resolveButton, &QPushButton::clicked, this, [this]() { resolveReport(false, false); }); - connect(resolveWithNoteButton, &QPushButton::clicked, this, [this]() { resolveReport(false, true); }); - connect(dismissButton, &QPushButton::clicked, this, [this]() { resolveReport(true, true); }); + connect(resolveButton, &QPushButton::clicked, this, [this]() { resolveReport(false); }); + connect(dismissButton, &QPushButton::clicked, this, [this]() { resolveReport(true); }); connect(reopenButton, &QPushButton::clicked, this, &TabReport::reopenReport); connect(viewReplayButton, &QPushButton::clicked, this, &TabReport::viewReplay); connect(joinGameButton, &QPushButton::clicked, this, &TabReport::joinGame); @@ -256,7 +254,6 @@ TabReport::TabReport(TabSupervisor *_tabSupervisor, AbstractClient *_client) : T auto *actionBar = new QHBoxLayout; actionBar->addWidget(assignButton); actionBar->addWidget(resolveButton); - actionBar->addWidget(resolveWithNoteButton); actionBar->addWidget(dismissButton); actionBar->addWidget(reopenButton); actionBar->addSpacing(20); @@ -323,8 +320,7 @@ void TabReport::retranslateUi() statsGroup->setTitle(tr("Report Statistics")); assignButton->setText(tr("Assign to Me")); - resolveButton->setText(tr("Resolve")); - resolveWithNoteButton->setText(tr("Resolve with note...")); + resolveButton->setText(tr("Resolve...")); dismissButton->setText(tr("Dismiss...")); reopenButton->setText(tr("Reopen...")); reopenButton->setToolTip(tr("Available for resolved or dismissed reports.")); @@ -626,7 +622,6 @@ void TabReport::updateActionStates() const QString status = QString::fromStdString(report.status()); assignButton->setEnabled(status == "open"); resolveButton->setEnabled(status == "open" || status == "assigned"); - resolveWithNoteButton->setEnabled(status == "open" || status == "assigned"); dismissButton->setEnabled(status == "open" || status == "assigned"); reopenButton->setEnabled(status == "resolved" || status == "dismissed"); @@ -640,7 +635,6 @@ void TabReport::setActionsEnabled(bool enabled) { assignButton->setEnabled(enabled); resolveButton->setEnabled(enabled); - resolveWithNoteButton->setEnabled(enabled); dismissButton->setEnabled(enabled); reopenButton->setEnabled(enabled); viewReplayButton->setEnabled(false); @@ -704,22 +698,19 @@ void TabReport::assignResponse(const Response &response) } } -void TabReport::resolveReport(bool dismissed, bool promptNote) +void TabReport::resolveReport(bool dismissed) { const int reportId = selectedReportId(); if (reportId < 0) { return; } - QString note; - if (promptNote) { - bool ok; - note = QInputDialog::getText(this, dismissed ? tr("Dismiss Report") : tr("Resolve Report"), - dismissed ? tr("Optional note:") : tr("Resolution note (optional):"), - QLineEdit::Normal, QString(), &ok); - if (!ok) { - return; - } + bool ok; + QString note = QInputDialog::getText(this, dismissed ? tr("Dismiss Report") : tr("Resolve Report"), + dismissed ? tr("Optional note:") : tr("Resolution note (optional):"), + QLineEdit::Normal, QString(), &ok); + if (!ok) { + return; } setActionsEnabled(false); diff --git a/cockatrice/src/interface/widgets/tabs/tab_report.h b/cockatrice/src/interface/widgets/tabs/tab_report.h index ea686c239..32ebe1a3f 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_report.h +++ b/cockatrice/src/interface/widgets/tabs/tab_report.h @@ -46,7 +46,7 @@ private slots: void reportListResponse(const Response &response); void assignReport(); void assignResponse(const Response &response); - void resolveReport(bool dismissed, bool promptNote); + void resolveReport(bool dismissed); void resolveResponse(const Response &response); void onSelectionChanged(); void showTableContextMenu(const QPoint &pos); @@ -91,7 +91,6 @@ private: QPushButton *commentButton; QPushButton *assignButton; QPushButton *resolveButton; - QPushButton *resolveWithNoteButton; QPushButton *dismissButton; QPushButton *reopenButton; QPushButton *viewReplayButton; From f83a61979fb33f8a9ad3f7d19d0ee2c56aebfcba Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:34:49 +0200 Subject: [PATCH 08/15] [Installer] Uninstaller deletes only the installed payload, never the whole tree (#7472) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Lukas Brübach --- cmake/NSIS.template.in | 48 ++++++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/cmake/NSIS.template.in b/cmake/NSIS.template.in index 4c55c00b3..b6125238f 100644 --- a/cmake/NSIS.template.in +++ b/cmake/NSIS.template.in @@ -304,10 +304,11 @@ SetShellVarContext all ; _?=$INSTDIR keeps the old uninstaller running in place. An NSIS uninstaller ; normally copies itself to %TEMP% and runs from there so that it can delete its ; own directory, and the process we started exits at once. ExecWait then returns -; before the uninstall has run, and the temp copy's "RMDir /r $INSTDIR" executes -; while the section below is still writing the new payload. _?= disables the copy -; and makes ExecWait block until the uninstall finishes. It must be the last -; argument and must not be quoted, even when the path contains spaces. +; before the uninstall has run, and the temp copy keeps deleting files - from +; 3.1.0 and 3.1.1 on disk a blanket "RMDir /r $INSTDIR" - while the section +; below is still writing the new payload. _?= disables the copy and makes +; ExecWait block until the uninstall finishes. It must be the last argument +; and must not be quoted, even when the path contains spaces. ; ; --- 32-bit uninstall --- SetRegView 32 @@ -1107,9 +1108,14 @@ ${AndIf} ${FileExists} "$INSTDIR\portable.dat" ; upgrade portable mode !insertmacro CockLog "section: portable upgrade purge" !insertmacro CockLogListing "before-portable-purge" "*.dll" + ; plugins and translations are payload and are rewritten by the payload + ; below, so removing them here is safe. sounds and themes are payload + ; too - the default packs are rewritten below as well - but a portable + ; user's custom sounds and themes live in exactly those folders, so a + ; recursive purge here would delete user content along with the defaults. + ; Stale default files from an older release are overwritten or replaced + ; by the payload, never by a purge. RMDir /r "$INSTDIR\plugins" - RMDir /r "$INSTDIR\sounds" - RMDir /r "$INSTDIR\themes" RMDir /r "$INSTDIR\translations" Delete "$INSTDIR\uninstall.exe" Delete "$INSTDIR\cockatrice.exe" @@ -1263,16 +1269,32 @@ Section "un.Application" UnSecApplication SetShellVarContext all StrCpy $R1 0 - ; Record whether this was a portable install before the directory (and the - ; portable.dat marker inside it) is removed below. + ; Record whether this was a portable install before portable.dat is + ; removed below. IfFileExists "$INSTDIR\portable.dat" 0 +2 StrCpy $R1 1 - ; Remove the entire application directory so any file that is not part of - ; the installed payload (e.g. build-tree artifacts such as *.dir folders, - ; *_autogen and *.tlog files from a build) cannot survive between an - ; uninstall and a fresh reinstall. - RMDir /r "$INSTDIR" + ; Remove only what the installer wrote. The two lists below are generated + ; by CPack from the staged install tree: one Delete per installed file and + ; one non-recursive RMDir per installed directory, children first. + ; Anything the user put in $INSTDIR - decks, pictures, portable data + ; folders - is not on those lists and survives, and $INSTDIR itself is + ; only removed when nothing else is left in it. An earlier version of this + ; section ran a blanket "RMDir /r $INSTDIR" to keep build-tree artifacts + ; out of a fresh reinstall; that also deleted user decks whenever a user + ; had pointed their data folders into the install directory, so payload + ; purity may never be bought at that price again. +@CPACK_NSIS_DELETE_FILES@ +@CPACK_NSIS_DELETE_DIRECTORIES@ + + ; Written by the installer itself, so absent from the staged payload + ; above; qdebug.txt is app-generated log output next to the executables. + Delete "$INSTDIR\uninstall.exe" + Delete "$INSTDIR\portable.dat" + Delete "$INSTDIR\qdebug.txt" + + ; Succeeds only when no foreign files remain. + RMDir "$INSTDIR" RMDir /r "$SMPROGRAMS\Cockatrice" From 57a6a0d9cbb26d82f187f4dd076cce881fb21a1e Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:34:49 +0200 Subject: [PATCH 09/15] [Client] Warn when data paths point inside the program directory (#7473) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Lukas Brübach --- cockatrice/CMakeLists.txt | 1 + .../widgets/dialogs/path_safety_warning.cpp | 34 +++++++++++++ .../widgets/dialogs/path_safety_warning.h | 25 +++++++++ .../deck_editor_settings_page.cpp | 4 ++ .../settings_page/general_settings_page.cpp | 22 ++++++++ cockatrice/src/interface/window_main.cpp | 33 ++++++++++++ cockatrice/src/interface/window_main.h | 3 ++ .../libcockatrice/settings/paths_settings.cpp | 51 +++++++++++++++++++ .../libcockatrice/settings/paths_settings.h | 24 ++++++++- tests/settings/settings_defaults_test.cpp | 51 +++++++++++++++++++ 10 files changed, 247 insertions(+), 1 deletion(-) create mode 100644 cockatrice/src/interface/widgets/dialogs/path_safety_warning.cpp create mode 100644 cockatrice/src/interface/widgets/dialogs/path_safety_warning.h diff --git a/cockatrice/CMakeLists.txt b/cockatrice/CMakeLists.txt index 32b81cf61..63ce7d1af 100644 --- a/cockatrice/CMakeLists.txt +++ b/cockatrice/CMakeLists.txt @@ -59,6 +59,7 @@ set(cockatrice_SOURCES src/interface/widgets/dialogs/dlg_update.cpp src/interface/widgets/dialogs/dlg_view_log.cpp src/interface/widgets/dialogs/override_printing_warning.cpp + src/interface/widgets/dialogs/path_safety_warning.cpp src/interface/widgets/dialogs/tip_of_the_day.cpp src/interface/widgets/deck_share/deck_share_utils.cpp src/interface/widgets/deck_share/shared_deck_preview_widget.cpp diff --git a/cockatrice/src/interface/widgets/dialogs/path_safety_warning.cpp b/cockatrice/src/interface/widgets/dialogs/path_safety_warning.cpp new file mode 100644 index 000000000..d8eabd72d --- /dev/null +++ b/cockatrice/src/interface/widgets/dialogs/path_safety_warning.cpp @@ -0,0 +1,34 @@ +#include "path_safety_warning.h" + +#include "../../client/settings/cache_settings.h" + +#include +#include +#include +#include +#include + +class QWidget; + +bool PathSafetyWarning::confirmOutsideProgramDir(QWidget *parent, const QString &path) +{ + if (SettingsCache::instance().getIsPortableBuild()) { + return true; + } + + if (!PathsSettings::isInsideDir(path, QCoreApplication::applicationDirPath())) { + return true; + } + + QMessageBox box(QMessageBox::Warning, QObject::tr("Path inside the program directory"), + QObject::tr("'%1' is inside the Cockatrice program directory.\n\n" + "Files stored there can be deleted by an update or by uninstalling " + "Cockatrice, so this change was not applied. Choose a location outside " + "the program directory.") + .arg(path), + QMessageBox::Ok, parent); + box.setTextFormat(Qt::PlainText); + box.exec(); + + return false; +} diff --git a/cockatrice/src/interface/widgets/dialogs/path_safety_warning.h b/cockatrice/src/interface/widgets/dialogs/path_safety_warning.h new file mode 100644 index 000000000..058df3e4b --- /dev/null +++ b/cockatrice/src/interface/widgets/dialogs/path_safety_warning.h @@ -0,0 +1,25 @@ +#ifndef COCKATRICE_PATH_SAFETY_WARNING_H +#define COCKATRICE_PATH_SAFETY_WARNING_H + +class QString; +class QWidget; + +namespace PathSafetyWarning +{ + +/** + * @brief Warns when the user tries to configure a data path inside the program directory. + * + * Shows a message box explaining that files kept there can be deleted by an + * update or by uninstalling Cockatrice. Portable builds are always exempt: + * their data lives beside the executables by design. + * + * @param parent The parent widget + * @param path The path the user is trying to configure + * @return true when the path is outside the program directory and safe to apply + */ +bool confirmOutsideProgramDir(QWidget *parent, const QString &path); + +} // namespace PathSafetyWarning + +#endif // COCKATRICE_PATH_SAFETY_WARNING_H diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index 2020b2031..31178ba4c 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -2,6 +2,7 @@ #include "../../../client/settings/cache_settings.h" #include "../../pixel_map_generator.h" +#include "../dialogs/path_safety_warning.h" #include "update/card_spoiler/spoiler_background_updater.h" #include @@ -363,6 +364,9 @@ void DeckEditorSettingsPage::spoilerPathButtonClicked() if (lsPath.isEmpty()) { return; } + if (!PathSafetyWarning::confirmOutsideProgramDir(this, lsPath + "/spoiler.xml")) { + return; + } mpSpoilerSavePathLineEdit->setText(lsPath + "/spoiler.xml"); SettingsCache::instance().paths().setSpoilerDatabasePath(lsPath + "/spoiler.xml"); diff --git a/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp index 73a0f7be2..fa8938b32 100644 --- a/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp @@ -1,6 +1,7 @@ #include "general_settings_page.h" #include "../../../client/settings/cache_settings.h" +#include "../dialogs/path_safety_warning.h" #include "../interface/card_picture_loader/card_picture_loader.h" #include "../main.h" #include "../server/user/user_info_connection.h" @@ -378,6 +379,9 @@ void GeneralSettingsPage::deckPathButtonClicked() if (path.isEmpty()) { return; } + if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { + return; + } deckPathEdit->setText(path); SettingsCache::instance().paths().setDeckPath(path); @@ -389,6 +393,9 @@ void GeneralSettingsPage::filtersPathButtonClicked() if (path.isEmpty()) { return; } + if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { + return; + } filtersPathEdit->setText(path); SettingsCache::instance().paths().setFiltersPath(path); @@ -400,6 +407,9 @@ void GeneralSettingsPage::replaysPathButtonClicked() if (path.isEmpty()) { return; } + if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { + return; + } replaysPathEdit->setText(path); SettingsCache::instance().paths().setReplaysPath(path); @@ -411,6 +421,9 @@ void GeneralSettingsPage::picsPathButtonClicked() if (path.isEmpty()) { return; } + if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { + return; + } picsPathEdit->setText(path); SettingsCache::instance().paths().setPicsPath(path); @@ -422,6 +435,9 @@ void GeneralSettingsPage::cardDatabasePathButtonClicked() if (path.isEmpty()) { return; } + if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { + return; + } cardDatabasePathEdit->setText(path); SettingsCache::instance().paths().setCardDatabasePath(path); @@ -433,6 +449,9 @@ void GeneralSettingsPage::customCardDatabaseButtonClicked() if (path.isEmpty()) { return; } + if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { + return; + } customCardDatabasePathEdit->setText(path); SettingsCache::instance().paths().setCustomCardDatabasePath(path); @@ -444,6 +463,9 @@ void GeneralSettingsPage::tokenDatabasePathButtonClicked() if (path.isEmpty()) { return; } + if (!PathSafetyWarning::confirmOutsideProgramDir(this, path)) { + return; + } tokenDatabasePathEdit->setText(path); SettingsCache::instance().paths().setTokenDatabasePath(path); diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index 189585d84..4f363181e 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -58,7 +58,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -603,8 +605,39 @@ MainWindow::MainWindow(QWidget *parent) QTimer::singleShot(0, this, &MainWindow::applyStartupDestination); } +void MainWindow::warnAboutDataPathsInProgramDir() +{ + auto &paths = SettingsCache::instance().paths(); + if (SettingsCache::instance().getIsPortableBuild() || paths.getAppDirWarningAcknowledged()) { + return; + } + + const QStringList offenders = paths.pathsInsideDir(QCoreApplication::applicationDirPath()); + if (offenders.isEmpty()) { + return; + } + + QMessageBox box(QMessageBox::Warning, tr("Data paths inside the program directory"), + tr("These paths are configured inside the Cockatrice program directory:\n\n%1\n\n" + "Cockatrice keeps its own data outside the program directory by default. Files " + "stored inside it can be deleted by an update or by uninstalling Cockatrice. " + "Change them in the Settings dialog - most are on the General page - to keep " + "them safe.") + .arg(offenders.join(QLatin1Char('\n'))), + QMessageBox::Ok, this); + box.setTextFormat(Qt::PlainText); + auto *dontShowAgain = new QCheckBox(tr("Don't show this again")); + box.setCheckBox(dontShowAgain); + box.exec(); + if (dontShowAgain->isChecked()) { + paths.setAppDirWarningAcknowledged(true); + } +} + void MainWindow::startupConfigCheck() { + warnAboutDataPathsInProgramDir(); + const bool isCleanInstall = SettingsCache::instance().network().getClientVersion() == CLIENT_INFO_NOT_SET; // checkUnknownSets() is intentionally deferred from the card database load diff --git a/cockatrice/src/interface/window_main.h b/cockatrice/src/interface/window_main.h index bf68ed72a..6b1186d3c 100644 --- a/cockatrice/src/interface/window_main.h +++ b/cockatrice/src/interface/window_main.h @@ -149,6 +149,9 @@ private: void runFirstRunWizard(bool firstRun = false); + /** @brief Warns once when a non-portable install keeps its data paths inside the program directory. */ + void warnAboutDataPathsInProgramDir(); + inline QString getCardUpdaterBinaryName() { return "oracle"; diff --git a/libcockatrice_settings/libcockatrice/settings/paths_settings.cpp b/libcockatrice_settings/libcockatrice/settings/paths_settings.cpp index 5f5725395..41ccf6460 100644 --- a/libcockatrice_settings/libcockatrice/settings/paths_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/paths_settings.cpp @@ -2,6 +2,8 @@ #include "libcockatrice/settings/settings_manager.h" +#include +#include #include class QObject; @@ -121,3 +123,52 @@ void PathsSettings::setSpoilerDatabasePath(const QString &_spoilerDatabasePath) setValue(_spoilerDatabasePath, "spoilerDatabase"); emit cardDatabasePathChanged(); } + +QStringList PathsSettings::pathsInsideDir(const QString &baseDir) const +{ + QStringList offenders; + const auto check = [&offenders, &baseDir](const QString &path) { + if (!path.isEmpty() && isInsideDir(path, baseDir)) { + offenders.append(QDir::cleanPath(path)); + } + }; + + check(getDeckPath()); + check(getFiltersPath()); + check(getReplaysPath()); + check(getPicsPath()); + check(getCustomPicsPath()); + check(getThemesPath()); + check(getCardDatabasePath()); + check(getCustomCardDatabasePath()); + check(getTokenDatabasePath()); + check(getSpoilerCardDatabasePath()); + check(getRedirectCachePath()); + + offenders.removeDuplicates(); + return offenders; +} + +bool PathsSettings::isInsideDir(const QString &path, const QString &baseDir) +{ +#if defined(Q_OS_WIN) || defined(Q_OS_MACOS) + // Windows and default macOS volumes are case-insensitive filesystems. + constexpr Qt::CaseSensitivity caseSensitivity = Qt::CaseInsensitive; +#else + constexpr Qt::CaseSensitivity caseSensitivity = Qt::CaseSensitive; +#endif + const QString base = QDir::cleanPath(baseDir); + const QString cleaned = QDir::cleanPath(path); + const QString prefix = base.endsWith(QLatin1Char('/')) ? base : base + QLatin1Char('/'); + return cleaned == base || cleaned.startsWith(prefix, caseSensitivity); +} + +bool PathsSettings::getAppDirWarningAcknowledged() const +{ + return getValue("appDirWarningAcknowledged").toBool(); +} + +void PathsSettings::setAppDirWarningAcknowledged(bool _appDirWarningAcknowledged) +{ + setValue(_appDirWarningAcknowledged, "appDirWarningAcknowledged"); +} diff --git a/libcockatrice_settings/libcockatrice/settings/paths_settings.h b/libcockatrice_settings/libcockatrice/settings/paths_settings.h index 399337bbb..fa00428e3 100644 --- a/libcockatrice_settings/libcockatrice/settings/paths_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/paths_settings.h @@ -4,6 +4,7 @@ #include "settings_manager.h" #include +#include #include #include @@ -27,6 +28,24 @@ public: [[nodiscard]] QString getSpoilerCardDatabasePath() const override; [[nodiscard]] QString getRedirectCachePath() const override; + /** + * @brief Returns the configured paths that resolve inside baseDir. + * + * Comparison is lexical (QDir::cleanPath); symlinks are not resolved, so a + * path that only reaches baseDir through a symlink is not reported. + */ + [[nodiscard]] QStringList pathsInsideDir(const QString &baseDir) const; + + /** + * @brief Whether path resolves inside baseDir, as a single-path check. + * + * A path counts as inside when it equals baseDir or is nested below it; + * a sibling sharing the name prefix (baseDir + "2") does not. + */ + [[nodiscard]] static bool isInsideDir(const QString &path, const QString &baseDir); + + [[nodiscard]] bool getAppDirWarningAcknowledged() const; + void setDeckPath(const QString &_deckPath); void setFiltersPath(const QString &_filtersPath); void setReplaysPath(const QString &_replaysPath); @@ -37,14 +56,17 @@ public: void setCustomCardDatabasePath(const QString &_customCardDatabasePath); void setTokenDatabasePath(const QString &_tokenDatabasePath); void setSpoilerDatabasePath(const QString &_spoilerDatabasePath); + void setAppDirWarningAcknowledged(bool _appDirWarningAcknowledged); signals: void cardDatabasePathChanged(); void picsPathChanged(); void themeChanged(); -private: +public: explicit PathsSettings(const QString &settingPath, QObject *parent = nullptr); + +private: PathsSettings(const PathsSettings & /*other*/); }; diff --git a/tests/settings/settings_defaults_test.cpp b/tests/settings/settings_defaults_test.cpp index 65e41af76..2ba9f3fbb 100644 --- a/tests/settings/settings_defaults_test.cpp +++ b/tests/settings/settings_defaults_test.cpp @@ -1,6 +1,7 @@ #include "libcockatrice/card/card_localization.h" #include "gtest/gtest.h" +#include #include #include #include @@ -15,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -695,6 +697,55 @@ TEST_F(SettingsDefaultsTest, VisualDeckStorage_ShowUploadTime_SetAndGet) ASSERT_EQ(s.getVisualDeckStorageShowUploadTime(), false); } +TEST_F(SettingsDefaultsTest, Paths_AppDirWarningAcknowledged_Default) +{ + PathsSettings s(settingsPath, nullptr); + ASSERT_EQ(s.getAppDirWarningAcknowledged(), false); +} + +TEST_F(SettingsDefaultsTest, Paths_AppDirWarningAcknowledged_SetAndGet) +{ + PathsSettings s(settingsPath, nullptr); + s.setAppDirWarningAcknowledged(true); + ASSERT_EQ(s.getAppDirWarningAcknowledged(), true); +} + +TEST_F(SettingsDefaultsTest, Paths_PathsInsideDir_DetectsNestedAndIgnoresSiblings) +{ + PathsSettings s(settingsPath, nullptr); + const QString base = QDir::cleanPath(settingsPath); + + s.setDeckPath(base + "/decks/"); + s.setPicsPath(base + "/pics"); + s.setReplaysPath(base + "2"); // sibling sharing the name prefix, not nested + + const QStringList offenders = s.pathsInsideDir(base); + ASSERT_EQ(offenders.size(), 2); + ASSERT_TRUE(offenders.contains(base + "/decks")); + ASSERT_TRUE(offenders.contains(base + "/pics")); +} + +TEST_F(SettingsDefaultsTest, Paths_PathsInsideDir_EmptyWhenNothingMatches) +{ + PathsSettings s(settingsPath, nullptr); + s.setDeckPath("/somewhere/else/decks"); + ASSERT_TRUE(s.pathsInsideDir(QDir::cleanPath(settingsPath)).isEmpty()); +} + +TEST_F(SettingsDefaultsTest, Paths_PathsInsideDir_MatchesExactBaseAndNormalizesDots) +{ + PathsSettings s(settingsPath, nullptr); + const QString base = QDir::cleanPath(settingsPath); + + s.setCustomPicsPath(base); // exactly the base directory + s.setThemesPath(base + "/pics/../decks"); // needs cleanPath normalization + + const QStringList offenders = s.pathsInsideDir(base); + ASSERT_EQ(offenders.size(), 2); + ASSERT_TRUE(offenders.contains(base)); + ASSERT_TRUE(offenders.contains(base + "/decks")); +} + } // namespace int main(int argc, char **argv) From 5d06bb903d8016b495d4be2d0d149b3f965ccb77 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:35:10 +0200 Subject: [PATCH 10/15] [Chat] Fix card completion after an @ mention (#7469) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An open [[ card link now takes precedence over a space-preceded @ mention, so typing an @ before a card no longer shadows the card completer. Co-authored-by: Lukas Brübach --- .../interface/widgets/utility/line_edit_completer.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cockatrice/src/interface/widgets/utility/line_edit_completer.cpp b/cockatrice/src/interface/widgets/utility/line_edit_completer.cpp index 056887f1a..d692fb41b 100644 --- a/cockatrice/src/interface/widgets/utility/line_edit_completer.cpp +++ b/cockatrice/src/interface/widgets/utility/line_edit_completer.cpp @@ -130,10 +130,19 @@ void LineEditCompleter::keyPressEvent(QKeyEvent *event) CompleterInfo *active = nullptr; QString prefix; + // An open "[[" card link takes precedence over a mention: while the cursor is + // inside it, an earlier "@" must not shadow the card completer. + const int openCardPos = textValue.lastIndexOf("[[", cursorPos - 1); + const int closeCardPos = openCardPos == -1 ? -1 : textValue.indexOf("]]", openCardPos + 2); + const bool insideCardLink = openCardPos != -1 && (closeCardPos == -1 || closeCardPos >= cursorPos); + for (auto &info : completers) { bool triggered = false; switch (info.trigger) { case CompleterTrigger::Mention: { + if (insideCardLink) { + break; + } int triggerPos = textValue.lastIndexOf("@", cursorPos - 1); if (triggerPos != -1 && (triggerPos == 0 || textValue[triggerPos - 1].isSpace())) { triggered = true; From 4c5a5e3e55404f7c66eb9886c78c9d719e4aacbe Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:36:11 +0200 Subject: [PATCH 11/15] [Game] Invalidate view when hiding replaced card menu (#7453) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Game] Invalidate view when hiding replaced card menu Hide any visible card menu before replacing and invalidate viewport. * [Game] Fix viewport update compile error in setCardMenu QWidget::update has no QRectF overload, and sceneRect is in scene coordinates rather than viewport coordinates anyway, so invalidate the whole viewport instead. --------- Co-authored-by: Lukas Brübach --- cockatrice/src/interface/widgets/tabs/tab_game.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cockatrice/src/interface/widgets/tabs/tab_game.cpp b/cockatrice/src/interface/widgets/tabs/tab_game.cpp index 9a6862b2c..4cf5919ba 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_game.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_game.cpp @@ -1060,6 +1060,12 @@ void TabGame::setCardMenu(CardMenu *menu) return; } + if (aCardMenu->menu() && aCardMenu->menu()->isVisible()) { + aCardMenu->menu()->hide(); + if (gameView && gameView->viewport()) { + gameView->viewport()->update(); + } + } if (menu) { aCardMenu->setMenu(menu); } else { From 4feb31bd1a4c829294339b74dc6a5ef57631da2a Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:53:26 +0200 Subject: [PATCH 12/15] [Game] Show loaded deck messages to players joining a game (#7268) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Show loaded deck messages to players joining a game (#552) The "Player X has loaded a deck" message is only broadcast when a deck is selected, so players and spectators who join a game afterwards never see it. On game join, replay a deck-select event context privately to the joining participant for every player that already has a deck loaded, reusing the existing Event_PlayerPropertiesChanged + Context_DeckSelect mechanism so no protocol version bump is needed. - Server_AbstractPlayer: expose the sideboard-locked state - Server_Game: synthesize deck-select contexts in createGameJoinedEvent * [Server] Skip deck replay on rejoin and after start --------- Co-authored-by: Lukas Brübach --- .../remote/game/server_abstract_player.h | 4 ++ .../server/remote/game/server_game.cpp | 44 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h index eff5034b4..ae20955bf 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h @@ -56,6 +56,10 @@ public: { return deck; } + bool getSideboardLocked() const + { + return sideboardLocked; + } bool getReadyStart() const { return readyStart; diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp index 330d473b2..b1863c91e 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp @@ -61,6 +61,8 @@ #include #include #include +#include +#include #include #include #include @@ -825,6 +827,48 @@ void Server_Game::createGameJoinedEvent(Server_AbstractParticipant *joiningParti } rc.enqueuePostResponseItem(ServerMessage::GAME_EVENT_CONTAINER, prepareGameEvent(event2, -1)); + + // A newly joined player never saw the "Player X has loaded a deck" messages + // (they are only broadcast when a deck is selected), so replay a deck-select + // context for every player that already has a deck loaded. It is delivered + // privately to the joining participant, whose client renders it as a log + // entry in the same way as a fresh deck selection. Skipped on reconnect + // (resuming) — the player already saw these — and once the game has started, + // when the message is no longer useful and would only spam the log. + if (!resuming && !gameStarted) { + for (auto *participant : participants.values()) { + if (participant == joiningParticipant) { + continue; + } + auto *deckOwner = qobject_cast(participant); + if (deckOwner == nullptr) { + continue; + } + const DeckList *deckList = deckOwner->getDeckList(); + if (deckList == nullptr) { + continue; + } + + Event_PlayerPropertiesChanged event; + event.mutable_player_properties()->set_sideboard_locked(deckOwner->getSideboardLocked()); + event.mutable_player_properties()->set_deck_hash(deckList->getDeckHash().toStdString()); + + Context_DeckSelect deckSelect; + deckSelect.set_deck_hash(deckList->getDeckHash().toStdString()); + deckSelect.set_sideboard_size(deckList->getSideboardSize()); + if (getShareDecklistsOnLoad()) { + deckSelect.set_deck_list(deckList->writeToString_Native().toStdString()); + } + + GameEventContext context; + context.GetReflection() + ->MutableMessage(&context, deckSelect.GetDescriptor()->FindExtensionByName("ext")) + ->CopyFrom(deckSelect); + + rc.enqueuePostResponseItem(ServerMessage::GAME_EVENT_CONTAINER, + prepareGameEvent(event, deckOwner->getPlayerId(), &context)); + } + } } void Server_Game::sendGameEventContainer(GameEventContainer *cont, From 1a4d89ff8f28d28af3a4f49e1f4e1552e1caeaae Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:54:03 +0200 Subject: [PATCH 13/15] [Game] Don't alert game tab for spectator activity (#1848) (#7263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spectator join/leave events were triggering the game tab's attention indicator (exclamation mark and taskbar alert) for players who often leave the tab open while waiting for an opponent. This made spectator activity a false alarm. Only emit the user event when an actual player joins or leaves, keeping spectator joins/leaves quiet. Co-authored-by: Lukas Brübach --- cockatrice/src/game/game_event_handler.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cockatrice/src/game/game_event_handler.cpp b/cockatrice/src/game/game_event_handler.cpp index b86f661bb..7b08af47c 100644 --- a/cockatrice/src/game/game_event_handler.cpp +++ b/cockatrice/src/game/game_event_handler.cpp @@ -277,8 +277,6 @@ void GameEventHandler::eventSpectatorLeave(const Event_Leave &event, emit spectatorLeft(eventPlayerId); game->getPlayerManager()->removeSpectator(eventPlayerId); - - emitUserEvent(); } void GameEventHandler::eventGameStateChanged(const Event_GameStateChanged &event, @@ -465,9 +463,9 @@ void GameEventHandler::eventJoin(const Event_Join &event, int /*eventPlayerId*/, PlayerLogic *newPlayer = game->getPlayerManager()->addPlayer(playerId, playerInfo.user_info()); emit logJoinPlayer(newPlayer); emit playerJoined(playerInfo); - } - emitUserEvent(); + emitUserEvent(); + } } QString GameEventHandler::getLeaveReason(Event_Leave::LeaveReason reason) From 89687096423af74ac7729823142428960bb48d3e Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 20:59:32 +0200 Subject: [PATCH 14/15] [SMTP] Add reliable email delivery with retry and database-backed queue (#7240) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [SMTP] Add reliable email delivery with retry and database-backed queue Rework the SMTP client from fire-and-forget sending into a persistent queue with exponential backoff and per-message retry budget, backed by the existing database rows as the durable source of truth. - Keep pending emails in an in-memory queue with attempts/backoff tracking; enqueue without deleting database rows, and only remove the row once delivery is confirmed via the mailDelivered signal. - Add exponential backoff (doubling, capped at 30 min) between delivery attempts, bounded by new smtp/maxretries and smtp/retrydelay settings. - Track connection-layer failures (connect/auth/encrypt) with their own backoff; route failures to recordFailed/recordConnectionFailure so undeliverable mail eventually drops from the queue instead of retrying forever. - Treat rejected recipients/senders as permanent failures. - Rebuild the SMTP buffer from the authoritative queue before each send via a new QxtSmtp::reset() so reconnects never duplicate deliveries. - Start the idle timeout after authentication, not on TCP connect, so slow SSL/STARTTLS handshakes are not cut short. * [SMTP] Fix retry accounting, backoff reset, and token dedupe --------- Co-authored-by: Lukas Brübach --- servatrice/servatrice.ini.example | 10 + servatrice/src/servatrice.cpp | 104 ++++-- servatrice/src/servatrice.h | 8 + servatrice/src/smtp/qxtsmtp.cpp | 32 ++ servatrice/src/smtp/qxtsmtp.h | 2 + servatrice/src/smtpclient.cpp | 532 ++++++++++++++++++++++++------ servatrice/src/smtpclient.h | 100 +++++- 7 files changed, 655 insertions(+), 133 deletions(-) diff --git a/servatrice/servatrice.ini.example b/servatrice/servatrice.ini.example index c625705da..3ee52dd21 100644 --- a/servatrice/servatrice.ini.example +++ b/servatrice/servatrice.ini.example @@ -250,6 +250,16 @@ email=root@localhost ; Sender email name name="Cockatrice server" +; Maximum number of attempts to deliver a given e-mail before giving up. +; If the smtp server is unreachable or repeatedly rejects a message, the server +; will retry with an exponentially growing delay up to this many times, then drop +; the message from the queue and log the failure. Default: 5 +maxretries=5 + +; Initial retry delay in seconds between delivery attempts. Doubles with each +; failed attempt (capped at 30 minutes). Default: 60 +retrydelay=60 + ; Email subject subject="Cockatrice server account activation token" diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index 0329a31fd..9ec9ef5bc 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -69,6 +70,8 @@ #include #include +inline Q_LOGGING_CATEGORY(ServatriceLog, "servatrice"); + Servatrice_GameServer::Servatrice_GameServer(Servatrice *_server, int _numberPools, const QSqlDatabase &_sqlDatabase, @@ -335,6 +338,11 @@ bool Servatrice::initServer() servatriceDatabaseInterface->clearSessionTables(); } + // Connect the SMTP client's delivery-confirmation signals to the database cleanup handlers. + // Emails are removed from the queue tables only after delivery is confirmed (see statusUpdate()). + connect(smtpClient, &SmtpClient::mailDelivered, this, &Servatrice::onEmailDelivered); + connect(smtpClient, &SmtpClient::mailPermanentlyFailed, this, &Servatrice::onEmailPermanentlyFailed); + if (getRoomsMethodString() == "sql") { QSqlQuery *query = servatriceDatabaseInterface->prepareQuery( "select id, name, descr, permissionlevel, privlevel, auto_join, join_message, chat_history_size from " @@ -737,41 +745,36 @@ void Servatrice::statusUpdate() return; } - auto *queryDelete = - servatriceDatabaseInterface->prepareQuery("delete from {prefix}_activation_emails where name = :name"); - + // Rows are intentionally NOT deleted here: the durable source of truth for a pending + // activation e-mail is the database row, which is only removed once delivery has been + // confirmed by the SMTP client (see onEmailDelivered). This makes sending resilient to + // transient SMTP failures and server restarts. while (servDbSelQuery->next()) { const QString userName = servDbSelQuery->value(0).toString(); const auto emailAddress = EmailParser::getParsedEmailAddress(servDbSelQuery->value(1).toString()); const QString token = servDbSelQuery->value(2).toString(); - if (smtpClient->enqueueActivationTokenMail(userName, emailAddress, token)) { - queryDelete->bindValue(":name", userName); - servatriceDatabaseInterface->execSqlQuery(queryDelete); - } + smtpClient->enqueueActivationTokenMail(userName, emailAddress, token); } } if (getEnableForgotPassword()) { auto *forgotPwQuery = servatriceDatabaseInterface->prepareQuery( - "select a.name, b.email, b.token from {prefix}_forgot_password a left join {prefix}_users b on a.name " - "= b.name where a.emailed = 0"); + "select a.id, a.name, b.email, b.token from {prefix}_forgot_password a left join {prefix}_users b on " + "a.name = b.name where a.emailed = 0"); if (!servatriceDatabaseInterface->execSqlQuery(forgotPwQuery)) { return; } - QSqlQuery *queryDelete = servatriceDatabaseInterface->prepareQuery( - "update {prefix}_forgot_password set emailed = 1 where name = :name"); - + // Rows are intentionally left with emailed = 0 here; they are marked emailed only once + // delivery is confirmed by the SMTP client (see onEmailDelivered). while (forgotPwQuery->next()) { - const QString userName = forgotPwQuery->value(0).toString(); - const auto emailAddress = EmailParser::getParsedEmailAddress(forgotPwQuery->value(1).toString()); - const QString token = forgotPwQuery->value(2).toString(); + const qint64 rowId = forgotPwQuery->value(0).toLongLong(); + const QString userName = forgotPwQuery->value(1).toString(); + const auto emailAddress = EmailParser::getParsedEmailAddress(forgotPwQuery->value(2).toString()); + const QString token = forgotPwQuery->value(3).toString(); - if (smtpClient->enqueueForgotPasswordTokenMail(userName, emailAddress, token)) { - queryDelete->bindValue(":name", userName); - servatriceDatabaseInterface->execSqlQuery(queryDelete); - } + smtpClient->enqueueForgotPasswordTokenMail(userName, emailAddress, token, rowId); } } @@ -779,6 +782,69 @@ void Servatrice::statusUpdate() } } +void Servatrice::onEmailDelivered(const QString &userName, EmailType type, const QString &token, qint64 rowId) +{ + if (cleanupDatabaseForEmail(userName, type, token, rowId)) { + qCDebug(ServatriceLog) << "E-mail delivered to" << userName; + } else { + qCWarning(ServatriceLog) << "Failed to clean up database row for delivered e-mail to" << userName + << "- row may be re-enqueued on next status update"; + } +} + +void Servatrice::onEmailPermanentlyFailed(const QString &userName, + EmailType type, + FailureReason reason, + const QString &token, + qint64 rowId) +{ + switch (reason) { + case FailureReason::RetryExhausted: + qCWarning(ServatriceLog) << "E-mail to" << userName << "permanently failed after max retries"; + break; + case FailureReason::RecipientRejected: + qCWarning(ServatriceLog) << "E-mail to" << userName << "permanently failed: recipient rejected"; + break; + case FailureReason::SenderRejected: + qCWarning(ServatriceLog) << "E-mail to" << userName << "permanently failed: sender rejected"; + break; + } + + if (!cleanupDatabaseForEmail(userName, type, token, rowId)) { + qCWarning(ServatriceLog) << "Failed to clean up database row for permanently failed e-mail to" << userName + << "- row may be re-enqueued on next status update"; + } +} + +bool Servatrice::cleanupDatabaseForEmail(const QString &userName, EmailType type, const QString &token, qint64 rowId) +{ + Q_UNUSED(token); + switch (type) { + case EmailType::Activation: { + QSqlQuery *query = + servatriceDatabaseInterface->prepareQuery("delete from {prefix}_activation_emails where name = :name"); + query->bindValue(":name", userName); + return servatriceDatabaseInterface->execSqlQuery(query); + } + case EmailType::ForgotPassword: { + // Scope the update to the row that was actually sent: the table + // holds one row per request, and marking by name would also mark + // rows for newer tokens that were never delivered. + if (rowId >= 0) { + QSqlQuery *query = servatriceDatabaseInterface->prepareQuery( + "update {prefix}_forgot_password set emailed = 1 where id = :id"); + query->bindValue(":id", rowId); + return servatriceDatabaseInterface->execSqlQuery(query); + } + QSqlQuery *query = servatriceDatabaseInterface->prepareQuery( + "update {prefix}_forgot_password set emailed = 1 where name = :name"); + query->bindValue(":name", userName); + return servatriceDatabaseInterface->execSqlQuery(query); + } + } + return false; +} + SessionEvent *Servatrice::makeShutdownEvent() const { Event_ServerShutdown event; diff --git a/servatrice/src/servatrice.h b/servatrice/src/servatrice.h index 242d445ad..205a4f04d 100644 --- a/servatrice/src/servatrice.h +++ b/servatrice/src/servatrice.h @@ -21,6 +21,7 @@ #define SERVATRICE_H #include "metrics_registry.h" +#include "smtpclient.h" #include #include @@ -181,6 +182,7 @@ private: QMap serverRequiredFeatureList; QString officialWarnings; Servatrice_DatabaseInterface *servatriceDatabaseInterface; + bool cleanupDatabaseForEmail(const QString &userName, EmailType type, const QString &token, qint64 rowId); int serverId; int uptime; QMutex txBytesMutex, rxBytesMutex; @@ -231,6 +233,12 @@ public slots: void scheduleShutdown(const QString &reason, int minutes); void updateLoginMessage(); void setRequiredFeatures(const QString &featureList); + void onEmailDelivered(const QString &userName, EmailType type, const QString &token, qint64 rowId); + void onEmailPermanentlyFailed(const QString &userName, + EmailType type, + FailureReason reason, + const QString &token, + qint64 rowId); public: explicit Servatrice(QObject *parent = nullptr); diff --git a/servatrice/src/smtp/qxtsmtp.cpp b/servatrice/src/smtp/qxtsmtp.cpp index c81e0955c..a4da649f7 100644 --- a/servatrice/src/smtp/qxtsmtp.cpp +++ b/servatrice/src/smtp/qxtsmtp.cpp @@ -94,6 +94,38 @@ int QxtSmtp::pendingMessages() const return qxt_d().pending.count(); } +/*! + * \brief Drops all queued (not yet transmitted) messages without sending them. + * + * Any message still awaiting delivery is discarded. This is used by callers that + * keep their own authoritative queue and wish to rebuild the SMTP buffer from scratch + * (e.g. after a reconnect) without risking duplicates. + */ +void QxtSmtp::clearPending() +{ + qxt_d().pending.clear(); +} + +/*! + * \brief Clears the outgoing queue and resets the SMTP state machine. + * + * Unlike clearPending(), this also resets the internal state to Disconnected so + * that subsequent send() calls will not fire sendNext() prematurely on a + * not-yet-connected socket. Use this when rebuilding the queue from scratch + * before a fresh connectToHost() / connectToSecureHost() cycle. + */ +void QxtSmtp::reset() +{ + qxt_d().pending.clear(); + qxt_d().extensions.clear(); + qxt_d().buffer.clear(); + qxt_d().recipients.clear(); + qxt_d().rcptNumber = 0; + qxt_d().rcptAck = 0; + qxt_d().mailAck = false; + qxt_d().state = QxtSmtpPrivate::Disconnected; +} + QTcpSocket *QxtSmtp::socket() const { return qxt_d().socket; diff --git a/servatrice/src/smtp/qxtsmtp.h b/servatrice/src/smtp/qxtsmtp.h index 747e22df9..d264d1047 100644 --- a/servatrice/src/smtp/qxtsmtp.h +++ b/servatrice/src/smtp/qxtsmtp.h @@ -66,6 +66,8 @@ public: int send(const QxtMailMessage& message); int pendingMessages() const; + void clearPending(); + void reset(); QTcpSocket* socket() const; void connectToHost(const QString& hostName, quint16 port = 25); diff --git a/servatrice/src/smtpclient.cpp b/servatrice/src/smtpclient.cpp index a45588b8b..82824e496 100644 --- a/servatrice/src/smtpclient.cpp +++ b/servatrice/src/smtpclient.cpp @@ -7,17 +7,56 @@ #include #include #include +#include #include #include +#include #include #include #include #include +static const qint64 DEFAULT_MAX_RETRIES = 5; +static const qint64 DEFAULT_RETRY_DELAY = 60; +static const qint64 MAX_RETRY_DELAY = 1800; // 30 minutes +static const qint64 IDLE_TIMEOUT_MS = 60000; // 60 seconds +// Maximum bit-shift exponent used in the exponential back-off formula. +// With a base delay of 60 s, shift 10 gives 60 * 1024 ≈ 17 h before the +// MAX_RETRY_DELAY cap is applied, preventing unbounded intermediate values. +static const int MAX_BACKOFF_SHIFT = 10; + +inline Q_LOGGING_CATEGORY(SmtpClientLog, "smtp_client") + +static QString pendingKey(const QString &userName, EmailType type, const QString &token) +{ + const char *typeName = (type == EmailType::Activation) ? "activation" : "forgotpassword"; + // The token is part of the key: a regenerated token (forgot-password issues a + // fresh one per request) must not be suppressed by a stale pending entry. + return userName + '/' + typeName + '/' + token; +} + +// True only for permanent 5xx replies. Anything else (4xx greylisting/rate +// limiting, or an unparsable reply) must be retried, never treated as final: +// destroying the durable queue row on a transient failure loses the mail. +static bool isPermanentSmtpReply(const QByteArray &msg) +{ + for (char c : msg) { + if (c == ' ' || c == '\t' || c == '\r' || c == '\n' || c == '-') { + continue; + } + return c == '5'; + } + return false; +} + SmtpClient::SmtpClient(QObject *parent) : QObject(parent) { smtp = new QxtSmtp(this); + idleTimer = new QTimer(this); + idleTimer->setSingleShot(true); + connect(idleTimer, SIGNAL(timeout()), this, SLOT(idleTimeout())); + connect(smtp, SIGNAL(authenticated()), this, SLOT(authenticated())); connect(smtp, SIGNAL(authenticationFailed(const QByteArray &)), this, SLOT(authenticationFailed(const QByteArray &))); @@ -40,104 +79,151 @@ SmtpClient::~SmtpClient() { if (smtp) { delete smtp; - smtp = 0; + smtp = nullptr; } } +int SmtpClient::maxRetries() const +{ + // Bounded like retryDelaySeconds(): an unset/blank value must not turn the + // first transient failure into a permanent one (attempts >= 0 is immediate). + return qBound(1, settingsCache->value("smtp/maxretries", DEFAULT_MAX_RETRIES).toInt(), 100); +} + +int SmtpClient::retryDelaySeconds() const +{ + return qBound(1, settingsCache->value("smtp/retrydelay", DEFAULT_RETRY_DELAY).toInt(), + static_cast(MAX_RETRY_DELAY)); +} + +QDateTime SmtpClient::nextRetryTime(int attempts, int retryDelay) const +{ + // Exponential backoff, capped to avoid hammering an unreachable server. + const int shift = qMax(0, qMin(attempts - 1, MAX_BACKOFF_SHIFT)); + const qint64 delay = qMin(retryDelay * qint64(1) << shift, MAX_RETRY_DELAY); + return QDateTime::currentDateTime().addSecs(delay); +} + +SmtpClient::PendingEmail SmtpClient::buildEmail(const QString &nickname, + EmailType type, + const QString &subject, + const QString &body, + const QString &recipient, + const QString &token, + const QString &email, + qint64 rowId) +{ + QString name = settingsCache->value("smtp/name", "").toString(); + + PendingEmail pending; + pending.userName = nickname; + pending.type = type; + pending.token = token; + pending.rowId = rowId; + pending.maxAttempts = maxRetries(); + pending.retryDelay = retryDelaySeconds(); + pending.nextAttempt = QDateTime::currentDateTime(); + + QString emailBody = body; + emailBody.replace("%username", nickname).replace("%token", token); + + pending.message.setSender(name + " <" + email + ">"); + pending.message.addRecipient(recipient); + pending.message.setSubject(subject); + pending.message.setBody(emailBody); + + return pending; +} + +bool SmtpClient::enqueue(const PendingEmail &email) +{ + const QString key = pendingKey(email.userName, email.type, email.token); + if (pendingEmails.contains(key)) { + qCDebug(SmtpClientLog) << "Email already pending for" << email.userName << "- not duplicating"; + return false; + } + + // A fresher database row for the same user/type supersedes a stale pending + // entry: drop the stale one so only the current token is ever delivered. + // (Any in-flight mapping for the dropped key is removed as well; the + // mailSent/recordFailed lookups tolerate missing entries.) + for (auto it = pendingEmails.begin(); it != pendingEmails.end();) { + if (it.value().userName == email.userName && it.value().type == email.type) { + for (auto mapIt = mailIdToUser.begin(); mapIt != mailIdToUser.end();) { + if (mapIt.value() == it.key()) { + mapIt = mailIdToUser.erase(mapIt); + } else { + ++mapIt; + } + } + qCDebug(SmtpClientLog) << "Replacing stale pending email for" << email.userName; + it = pendingEmails.erase(it); + } else { + ++it; + } + } + + pendingEmails.insert(key, email); + qCDebug(SmtpClientLog) << "Enqueued" << (email.type == EmailType::Activation ? "activation" : "password reset") + << "mail to" << email.userName; + return true; +} + bool SmtpClient::enqueueActivationTokenMail(const QString &nickname, const QString &recipient, const QString &token) { - QString email = settingsCache->value("smtp/email", "").toString(); - QString name = settingsCache->value("smtp/name", "").toString(); + if (pendingEmails.contains(pendingKey(nickname, EmailType::Activation, token))) { + qCDebug(SmtpClientLog) << "Activation email already pending for" << nickname << "- skipping"; + return false; + } + QString subject = settingsCache->value("smtp/subject", "").toString(); QString body = settingsCache->value("smtp/body", "").toString(); + QString email = settingsCache->value("smtp/email", "").toString(); - if (email.isEmpty()) { - qDebug() << "[MAIL] Missing sender email in configuration"; + if (!validateEmailFields(email, subject, body, recipient, token, nickname)) { return false; } - if (subject.isEmpty()) { - qDebug() << "[MAIL] Missing subject field in configuration"; - return false; - } - - if (body.isEmpty()) { - qDebug() << "[MAIL] Missing body field in configuration"; - return false; - } - - if (recipient.isEmpty()) { - qDebug() << "[MAIL] Missing recipient field for user " << nickname; - return false; - } - - if (token.isEmpty()) { - qDebug() << "[MAIL] Missing token field for user " << nickname; - return false; - } - - QxtMailMessage message; - message.setSender(name + " <" + email + ">"); - message.addRecipient(recipient); - message.setSubject(subject); - message.setBody(body.replace("%username", nickname).replace("%token", token)); - - int id = smtp->send(message); - qDebug() << "[MAIL] Enqueued mail to" << recipient << "as" << id; - return true; + return enqueue(buildEmail(nickname, EmailType::Activation, subject, body, recipient, token, email)); } -bool SmtpClient::enqueueForgotPasswordTokenMail(const QString &nickname, const QString &recipient, const QString &token) +bool SmtpClient::enqueueForgotPasswordTokenMail(const QString &nickname, + const QString &recipient, + const QString &token, + qint64 rowId) { - QString email = settingsCache->value("smtp/email", "").toString(); - QString name = settingsCache->value("smtp/name", "").toString(); + if (pendingEmails.contains(pendingKey(nickname, EmailType::ForgotPassword, token))) { + qCDebug(SmtpClientLog) << "Password reset email already pending for" << nickname << "- skipping"; + return false; + } + QString subject = settingsCache->value("forgotpassword/subject", "").toString(); QString body = settingsCache->value("forgotpassword/body", "").toString(); + QString email = settingsCache->value("smtp/email", "").toString(); - if (email.isEmpty()) { - qDebug() << "[MAIL] Missing sender email in configuration"; + if (!validateEmailFields(email, subject, body, recipient, token, nickname)) { return false; } - if (subject.isEmpty()) { - qDebug() << "[MAIL] Missing subject field in configuration"; - return false; - } - - if (body.isEmpty()) { - qDebug() << "[MAIL] Missing body field in configuration"; - return false; - } - - if (recipient.isEmpty()) { - qDebug() << "[MAIL] Missing recipient field for user " << nickname; - return false; - } - - if (token.isEmpty()) { - qDebug() << "[MAIL] Missing token field for user " << nickname; - return false; - } - - QxtMailMessage message; - message.setSender(name + " <" + email + ">"); - message.addRecipient(recipient); - message.setSubject(subject); - message.setBody(body.replace("%username", nickname).replace("%token", token)); - - int id = smtp->send(message); - qDebug() << "[MAIL] Enqueued mail to" << recipient << "as" << id; - return true; + return enqueue(buildEmail(nickname, EmailType::ForgotPassword, subject, body, recipient, token, email, rowId)); } void SmtpClient::sendAllEmails() { // still connected from the previous round - if (smtp->socket()->state() == QAbstractSocket::ConnectedState) { + if (smtp->socket()->state() == QAbstractSocket::ConnectedState || + smtp->socket()->state() == QAbstractSocket::ConnectingState || + smtp->socket()->state() == QAbstractSocket::HostLookupState || + smtp->socket()->state() == QAbstractSocket::ClosingState) { return; } - if (smtp->pendingMessages() == 0) { + if (pendingEmails.isEmpty()) { + return; + } + + // Respect connection-level backoff after auth/conn/encryption failures. + if (connectionAttempts > 0 && QDateTime::currentDateTime() < connectionBackoffUntil) { return; } @@ -151,6 +237,16 @@ void SmtpClient::sendAllEmails() smtp->setUsername(username); smtp->setPassword(password); + // Push due emails into QxtSmtp *before* connecting so they are already + // queued when the authenticated() signal fires and QxtSmtp's internal + // sendNext() runs. + relayoutForSending(); + + // Nothing due right now — skip the SMTP round-trip. + if (mailIdToUser.isEmpty()) { + return; + } + // Connect if (connectionType == "ssl") { if (acceptAllCerts) { @@ -162,64 +258,286 @@ void SmtpClient::sendAllEmails() } } -void SmtpClient::authenticated() +void SmtpClient::relayoutForSending() { - qDebug() << "[MAIL] authenticated"; -} + // Rebuild the SMTP buffer exclusively from our authoritative set, so a reconnect + // after a partial transmission never results in duplicate deliveries. + // reset() also clears the internal state machine so that send() does not + // fire sendNext() prematurely on a not-yet-connected socket. + smtp->reset(); + mailIdToUser.clear(); -void SmtpClient::authenticationFailed(const QByteArray &msg) -{ - qDebug() << "[MAIL] authenticationFailed" << QString(msg); -} - -void SmtpClient::connected() -{ - qDebug() << "[MAIL] connected"; -} - -void SmtpClient::connectionFailed(const QByteArray &msg) -{ - qDebug() << "[MAIL] connectionFailed" << QString(msg); -} - -void SmtpClient::disconnected() -{ - qDebug() << "[MAIL] disconnected"; -} - -void SmtpClient::encrypted() -{ - qDebug() << "[MAIL] encrypted"; -} - -void SmtpClient::encryptionFailed(const QByteArray &msg) -{ - qDebug() << "[MAIL] encryptionFailed" << QString(msg); - qDebug() << "[MAIL] Try enabling the \"acceptallcerts\" option in servatrice.ini"; + const QDateTime now = QDateTime::currentDateTime(); + for (auto it = pendingEmails.begin(); it != pendingEmails.end(); ++it) { + PendingEmail &pending = it.value(); + if (pending.nextAttempt > now) { + continue; // not yet due; honours the backoff schedule + } + int id = smtp->send(pending.message); + mailIdToUser.insert(id, pendingKey(pending.userName, pending.type, pending.token)); + } } void SmtpClient::finished() { - qDebug() << "[MAIL] finished"; + qCDebug(SmtpClientLog) << "finished"; smtp->disconnectFromHost(); } +void SmtpClient::authenticated() +{ + qCDebug(SmtpClientLog) << "authenticated"; + // The session has actually succeeded (TCP connect alone proves nothing: + // TLS/auth may still fail), so the connection-level backoff resets here. + connectionAttempts = 0; + // Start the idle timer here, once the connection is fully ready to send, + // rather than on TCP connect: SSL/STARTTLS handshake and auth may take a + // while and must not be cut short by the idle timeout. + idleTimer->start(IDLE_TIMEOUT_MS); +} + +void SmtpClient::authenticationFailed(const QByteArray &msg) +{ + qCWarning(SmtpClientLog) << "authenticationFailed" << QString::fromUtf8(msg); + idleTimer->stop(); + recordConnectionFailure(); + mailIdToUser.clear(); + smtp->disconnectFromHost(); +} + +void SmtpClient::connected() +{ + qCDebug(SmtpClientLog) << "connected"; + // NOTE: connectionAttempts is deliberately not reset here. This fires on + // TCP connect, before the TLS handshake and AUTH; resetting here would pin + // the connection-level backoff at its base delay for exactly the failures + // (bad password/cert/STARTTLS) it is meant to cover. It resets in + // authenticated() and on the first successful mailSent instead. +} + +void SmtpClient::connectionFailed(const QByteArray &msg) +{ + qCWarning(SmtpClientLog) << "connectionFailed" << QString::fromUtf8(msg); + idleTimer->stop(); + recordConnectionFailure(); + mailIdToUser.clear(); + smtp->disconnectFromHost(); +} + +void SmtpClient::disconnected() +{ + qCDebug(SmtpClientLog) << "disconnected - emails still pending:" << pendingEmails.count(); + idleTimer->stop(); + // A mid-session drop (peer close/RST past the connect phase, or the idle + // timeout firing mid-batch) emits no mailFailed/connectionFailed signal, so + // route every in-flight mail through recordFailed(): otherwise attempts and + // backoff are bypassed and the mails requeue with stale state forever. + const QList inFlight = mailIdToUser.keys(); + for (int mailID : inFlight) { + recordFailed(mailID); + } + mailIdToUser.clear(); +} + +void SmtpClient::encrypted() +{ + qCDebug(SmtpClientLog) << "encrypted"; +} + +void SmtpClient::encryptionFailed(const QByteArray &msg) +{ + qCWarning(SmtpClientLog) << "encryptionFailed" << QString::fromUtf8(msg); + qCWarning(SmtpClientLog) << "Try enabling the \"acceptallcerts\" option in servatrice.ini"; + idleTimer->stop(); + recordConnectionFailure(); + mailIdToUser.clear(); + smtp->disconnectFromHost(); +} + +void SmtpClient::idleTimeout() +{ + qCDebug(SmtpClientLog) << "idle timeout - disconnecting"; + smtp->disconnectFromHost(); +} + +void SmtpClient::recordFailed(int mailID) +{ + auto it = mailIdToUser.find(mailID); + if (it == mailIdToUser.end()) { + return; + } + const QString key = it.value(); + mailIdToUser.erase(it); + + auto pendingIt = pendingEmails.find(key); + if (pendingIt == pendingEmails.end()) { + return; + } + + PendingEmail &pending = pendingIt.value(); + pending.attempts++; + + if (pending.attempts >= pending.maxAttempts) { + const QString userName = pending.userName; + const EmailType type = pending.type; + const QString token = pending.token; + const qint64 rowId = pending.rowId; + qCWarning(SmtpClientLog) << "Email to" << userName << "permanently failed after" << pending.attempts + << "attempts - dropping from pending set"; + pendingEmails.erase(pendingIt); + emit mailPermanentlyFailed(userName, type, FailureReason::RetryExhausted, token, rowId); + return; + } + + pending.nextAttempt = nextRetryTime(pending.attempts, pending.retryDelay); + qCWarning(SmtpClientLog) << "Email to" << pending.userName << "failed (attempt" << pending.attempts << "of" + << pending.maxAttempts << "), retrying" << pending.nextAttempt.toString(Qt::ISODate); +} + +void SmtpClient::recordConnectionFailure() +{ + ++connectionAttempts; + connectionBackoffUntil = nextRetryTime(connectionAttempts, retryDelaySeconds()); + qCWarning(SmtpClientLog) << "Connection-layer failure #" << connectionAttempts << "- next attempt after" + << connectionBackoffUntil.toString(Qt::ISODate); + + // Connection failures count toward the per-email retry budget so that + // undeliverable mail eventually gives up rather than retrying forever. + QList keysToRemove; + for (auto it = pendingEmails.begin(); it != pendingEmails.end(); ++it) { + PendingEmail &pending = it.value(); + pending.attempts++; + if (pending.attempts >= pending.maxAttempts) { + qCWarning(SmtpClientLog) << "Email to" << pending.userName << "permanently failed after" << pending.attempts + << "connection failures - dropping from pending set"; + keysToRemove.append(it.key()); + emit mailPermanentlyFailed(pending.userName, pending.type, FailureReason::RetryExhausted, pending.token, + pending.rowId); + } + } + for (const QString &key : keysToRemove) { + pendingEmails.remove(key); + } +} + +bool SmtpClient::validateEmailFields(const QString &email, + const QString &subject, + const QString &body, + const QString &recipient, + const QString &token, + const QString &nickname) +{ + if (email.isEmpty()) { + qCCritical(SmtpClientLog) << "Missing sender email in configuration"; + return false; + } + if (subject.isEmpty()) { + qCCritical(SmtpClientLog) << "Missing subject field in configuration"; + return false; + } + if (body.isEmpty()) { + qCCritical(SmtpClientLog) << "Missing body field in configuration"; + return false; + } + if (recipient.isEmpty()) { + qCCritical(SmtpClientLog) << "Missing recipient field for user" << nickname; + return false; + } + if (token.isEmpty()) { + qCCritical(SmtpClientLog) << "Missing token field for user" << nickname; + return false; + } + return true; +} + void SmtpClient::mailFailed(int mailID, int errorCode, const QByteArray &msg) { - qDebug() << "[MAIL] mailFailed id=" << mailID << " errorCode=" << errorCode << "msg=" << QString(msg); + qCWarning(SmtpClientLog) << "mailFailed id=" << mailID << " errorCode=" << errorCode + << "msg=" << QString::fromUtf8(msg); + // Progress was made (the server answered), so the idle timer measures from + // here rather than acting as a cap on the whole session. + idleTimer->start(IDLE_TIMEOUT_MS); + recordFailed(mailID); } void SmtpClient::mailSent(int mailID) { - qDebug() << "[MAIL] mailSent" << mailID; + qCDebug(SmtpClientLog) << "mailSent" << mailID; + // A delivered mail proves the session works: reset the connection-level + // backoff and restart the idle timer from this point of progress. + connectionAttempts = 0; + idleTimer->start(IDLE_TIMEOUT_MS); + + auto it = mailIdToUser.find(mailID); + if (it == mailIdToUser.end()) { + return; + } + const QString key = it.value(); + mailIdToUser.erase(it); + + auto pendingIt = pendingEmails.find(key); + if (pendingIt == pendingEmails.end()) { + return; + } + + const QString userName = pendingIt.value().userName; + const EmailType type = pendingIt.value().type; + const QString token = pendingIt.value().token; + const qint64 rowId = pendingIt.value().rowId; + pendingEmails.erase(pendingIt); + qCDebug(SmtpClientLog) << "Delivered email to" << userName; + emit mailDelivered(userName, type, token, rowId); } void SmtpClient::recipientRejected(int mailID, const QString &address, const QByteArray &msg) { - qDebug() << "[MAIL] recipientRejected id=" << mailID << " address=" << address << "msg=" << QString(msg); + qCWarning(SmtpClientLog) << "recipientRejected id=" << mailID << " address=" << address + << "msg=" << QString::fromUtf8(msg); + if (!isPermanentSmtpReply(msg)) { + // Transient 4xx (e.g. greylisting): consume one attempt and reschedule + // with backoff instead of destroying the durable queue row. + recordFailed(mailID); + return; + } + // A rejected recipient is a permanent condition; consider the mail undeliverable. + auto it = mailIdToUser.find(mailID); + if (it != mailIdToUser.end()) { + const QString key = it.value(); + if (auto pendingIt = pendingEmails.find(key); pendingIt != pendingEmails.end()) { + const QString userName = pendingIt.value().userName; + const EmailType type = pendingIt.value().type; + const QString token = pendingIt.value().token; + const qint64 rowId = pendingIt.value().rowId; + pendingEmails.erase(pendingIt); + qCWarning(SmtpClientLog) << "Email to" << userName << "rejected - dropping from pending set"; + emit mailPermanentlyFailed(userName, type, FailureReason::RecipientRejected, token, rowId); + } + mailIdToUser.erase(it); + } } void SmtpClient::senderRejected(int mailID, const QString &address, const QByteArray &msg) { - qDebug() << "[MAIL] senderRejected id=" << mailID << " address=" << address << "msg=" << QString(msg); + qCWarning(SmtpClientLog) << "senderRejected id=" << mailID << " sender=" << address + << "msg=" << QString::fromUtf8(msg); + if (!isPermanentSmtpReply(msg)) { + // Transient 4xx on MAIL FROM (e.g. rate limiting): retry with backoff. + recordFailed(mailID); + return; + } + // A rejected sender is a permanent condition; consider the mail undeliverable. + auto it = mailIdToUser.find(mailID); + if (it != mailIdToUser.end()) { + const QString key = it.value(); + if (auto pendingIt = pendingEmails.find(key); pendingIt != pendingEmails.end()) { + const QString userName = pendingIt.value().userName; + const EmailType type = pendingIt.value().type; + const QString token = pendingIt.value().token; + const qint64 rowId = pendingIt.value().rowId; + pendingEmails.erase(pendingIt); + qCWarning(SmtpClientLog) << "Email to" << userName << "sender rejected - dropping from pending set"; + emit mailPermanentlyFailed(userName, type, FailureReason::SenderRejected, token, rowId); + } + mailIdToUser.erase(it); + } } diff --git a/servatrice/src/smtpclient.h b/servatrice/src/smtpclient.h index 8fc4e35cb..9fc11bf36 100644 --- a/servatrice/src/smtpclient.h +++ b/servatrice/src/smtpclient.h @@ -1,26 +1,67 @@ #ifndef SMTPCLIENT_H #define SMTPCLIENT_H +#include "smtp/qxtmailmessage.h" + +#include +#include #include #include +#include #include class QxtSmtp; class QByteArray; +/** + * @brief Types of e-mail the SMTP client is able to deliver. + */ +enum class EmailType +{ + Activation, + ForgotPassword +}; + +Q_DECLARE_METATYPE(EmailType) + +/** + * @brief Why a permanent e-mail delivery failure occurred. + */ +enum class FailureReason +{ + RetryExhausted, + RecipientRejected, + SenderRejected +}; + +Q_DECLARE_METATYPE(FailureReason) + class SmtpClient : public QObject { Q_OBJECT public: - SmtpClient(QObject *parent = 0); - ~SmtpClient(); + SmtpClient(QObject *parent = nullptr); + ~SmtpClient() override; + +public slots: + bool enqueueActivationTokenMail(const QString &nickname, const QString &recipient, const QString &token); + bool enqueueForgotPasswordTokenMail(const QString &nickname, + const QString &recipient, + const QString &token, + qint64 rowId = -1); + void sendAllEmails(); + +signals: + void mailDelivered(const QString &userName, EmailType type, const QString &token, qint64 rowId); + void mailPermanentlyFailed(const QString &userName, + EmailType type, + FailureReason reason, + const QString &token, + qint64 rowId); protected: QxtSmtp *smtp; -public slots: - bool enqueueActivationTokenMail(const QString &nickname, const QString &recipient, const QString &token); - bool enqueueForgotPasswordTokenMail(const QString &nickname, const QString &recipient, const QString &token); - void sendAllEmails(); + protected slots: void authenticated(); void authenticationFailed(const QByteArray &msg); @@ -30,10 +71,55 @@ protected slots: void encrypted(); void encryptionFailed(const QByteArray &msg); void finished(); + void idleTimeout(); void mailFailed(int mailID, int errorCode, const QByteArray &msg); void mailSent(int mailID); void recipientRejected(int mailID, const QString &address, const QByteArray &msg); void senderRejected(int mailID, const QString &address, const QByteArray &msg); + +private: + struct PendingEmail + { + QString userName; + EmailType type; + QString token; + // Database row id for tables that allow multiple rows per user + // ({prefix}_forgot_password); -1 when the name alone identifies the row. + qint64 rowId = -1; + QxtMailMessage message; + int attempts = 0; + int maxAttempts = 0; + int retryDelay = 0; + QDateTime nextAttempt; + }; + + PendingEmail buildEmail(const QString &nickname, + EmailType type, + const QString &subject, + const QString &body, + const QString &recipient, + const QString &token, + const QString &email, + qint64 rowId = -1); + bool enqueue(const PendingEmail &email); + void relayoutForSending(); + QDateTime nextRetryTime(int attempts, int retryDelay) const; + void recordFailed(int mailID); + void recordConnectionFailure(); + bool validateEmailFields(const QString &email, + const QString &subject, + const QString &body, + const QString &recipient, + const QString &token, + const QString &nickname); + int maxRetries() const; + int retryDelaySeconds() const; + + QHash pendingEmails; + QHash mailIdToUser; + QTimer *idleTimer; + int connectionAttempts = 0; + QDateTime connectionBackoffUntil; }; -#endif \ No newline at end of file +#endif From 96b819d7c8fddd89695476b175ac3b2dd022b7b7 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 11 Oct 2026 21:19:10 +0200 Subject: [PATCH 15/15] Support tab-separated deck imports (#4222) (#7267) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third-party deck tools (e.g. Delver Lens) export decks as tab-separated rows such as "2 Card Name Set Name". Card names never contain tab characters, so a leading numeric column marks a tabular record: the first two columns are read as the quantity and card name, and any further columns are discarded as printer/set metadata. - deck_list: parse tab-delimited rows before whitespace collapsing - tests: cover tab-separated mainboard and sideboard imports - docs: document the accepted plain-text import format (fills the TODO) Co-authored-by: Lukas Brübach --- .../deck_management/importing_decks.md | 13 +++++- .../deck_list/deck_list_plain_text_parser.cpp | 40 ++++++++++++++++--- .../loading_from_clipboard_test.cpp | 14 +++++++ 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/doc/doxygen/pages/user_documentation/deck_management/importing_decks.md b/doc/doxygen/pages/user_documentation/deck_management/importing_decks.md index e9626d57f..4aab7ad2c 100644 --- a/doc/doxygen/pages/user_documentation/deck_management/importing_decks.md +++ b/doc/doxygen/pages/user_documentation/deck_management/importing_decks.md @@ -38,7 +38,18 @@ Selecting this action will open a new text editor dialog with the contents of yo The import dialog expects each line to be a card with the following format: -TODO +`[quantity] Card Name [options]` + +- `quantity` is the number of copies and can be written as `2`, `2x` or `2X`. If omitted, a single copy is imported. +- `options` are appended after the card name and are understood on a best-effort basis: + - a set code and collector number, either as `Card Name (SET) 123` or `Card Name (SET) 123-1` + - a foil marker, `Card Name *F*` +- Lines can also carry a `SB:` prefix or a set code prefix (`[SET]`) — Cockatrice tries to stay compatible with the most + common deck formats. + +Tab-separated exports from third-party apps (such as Delver Lens) are also accepted: any line whose first column is a +plain quantity treats the following columns as the card name and printer/set metadata (which is discarded), for example +`2\tCard Name\tSet Name`. Each card should be on a separate line and there should be no empty lines between cards. The first empty line between two blocks of cards will be considered as the divider between mainboard and sideboard. diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_plain_text_parser.cpp b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_plain_text_parser.cpp index 61af36540..fb6a61769 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_plain_text_parser.cpp +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_plain_text_parser.cpp @@ -33,6 +33,7 @@ bool parse(QTextStream &in, // Regex for advanced card parsing static const QRegularExpression reMultiplier(R"(^[xX\(\[]*(\d+)[xX\*\)\]]* ?(.+))"); + static const QRegularExpression reTabQuantity(R"(^\d+[xX]?$)"); // Regex for extracting set code and collector number with attached symbols static const QRegularExpression reHyphenFormat(R"(\((\w{3,})\)\s+(\w{3,})-(\d+[^\w\s]*))"); @@ -110,8 +111,34 @@ bool parse(QTextStream &in, continue; } - QString cardName = match.captured().simplified(); + const QString rawLine = match.captured(); + QString cardName = rawLine.simplified(); bool sideboard = false; + int amount = 1; + + // Card names never contain tab characters, so a leading numeric field + // marks a tabular record emitted by third-party deck tools (e.g. Delver + // Lens "2\tCard Name\tSet Name"). The first two columns are the quantity + // and the card name; any further columns are printer/set metadata and + // are discarded. + bool hasTabQuantity = false; + const auto tabFields = rawLine.split('\t'); + int firstField = 0; + while (firstField < tabFields.size() && tabFields.at(firstField).trimmed().isEmpty()) { + ++firstField; + } + if (firstField + 1 < tabFields.size()) { + const QString quantity = tabFields.at(firstField).trimmed(); + if (reTabQuantity.match(quantity).hasMatch()) { + hasTabQuantity = true; + QString digits = quantity; + if (digits.endsWith('x') || digits.endsWith('X')) { + digits.chop(1); + } + amount = digits.toInt(); + cardName = tabFields.at(firstField + 1).simplified(); + } + } // Sideboard detection if (sBStart < 0) { @@ -156,11 +183,12 @@ bool parse(QTextStream &in, } // check if a specific amount is mentioned - int amount = 1; - match = reMultiplier.match(cardName); - if (match.hasMatch()) { - amount = match.captured(1).toInt(); - cardName = match.captured(2); + if (!hasTabQuantity) { + match = reMultiplier.match(cardName); + if (match.hasMatch()) { + amount = match.captured(1).toInt(); + cardName = match.captured(2); + } } // Normalize the card name diff --git a/tests/loading_from_clipboard/loading_from_clipboard_test.cpp b/tests/loading_from_clipboard/loading_from_clipboard_test.cpp index 840d926ff..e1797818e 100644 --- a/tests/loading_from_clipboard/loading_from_clipboard_test.cpp +++ b/tests/loading_from_clipboard/loading_from_clipboard_test.cpp @@ -80,6 +80,20 @@ TEST(LoadingFromClipboardTest, WeirdWhitespaceIsIgnored) testDeck(clipboard, result); } +TEST(LoadingFromClipboardTest, TabSeparatedImport) +{ + QString clipboard("2\tMystic Snake\tStreets of New Capenna\n" + "3x\tCounterspell\tNPH\n" + "\t2\tSol Ring\tFMB\n" + "1\tForest\n" + "\n" + "2x\tDoom Blade\tM11\n"); + + Result result("", "", {{"Mystic Snake", 2}, {"Counterspell", 3}, {"Sol Ring", 2}, {"Forest", 1}}, + {{"Doom Blade", 2}}); + testDeck(clipboard, result); +} + TEST(LoadingFromClipboardTest, RemoveBlankEntriesFromBeginningAndEnd) { QString clipboard("\n"