3 commits

Author	SHA1	Message	Date
BruebachL	5ea16c805f	[Server/Client/Protocol] Shuffle or reorder player seats before a game starts (#7398) ... * [Protocol] Add shuffle players option and player order command/event Adds a 'Shuffle players at start' option to game creation plus a local player id on the game state so clients can reconcile seat changes, and a SET_PLAYER_ORDER command (1206) with a PLAYER_ORDER_CHANGED event (2023) so the game host can set or randomize the seating order. Took 6 minutes * [Server] Shuffle and reorder player seats before a game starts Auto-shuffles the seats at game start when enabled (now for games with two or more players instead of three or more) and handles the new SET_PLAYER_ORDER command so the game host or a moderator can reorder or randomize the seats before the game starts. Re-seating re-keys the player ids while keeping the per-user game/seat routing and persistent player records in sync, then broadcasts the new order to everyone. * [Client] Add shuffle players option and turn order dialog Adds the 'Shuffle players at start' checkbox to game creation and a host-only 'Set turn order...' action that opens a dialog to reorder or randomize the seats before the game starts. Seating changes are shown in the game's message log. * [Client] Move the turn order dialog strings into retranslateUi Follows the DlgInviteToGame convention so the dialog can be retranslated instead of freezing its strings at construction time. * [Server] Keep player ids stable across seat shuffles * [Client] Lay out table and turn order dialog in seat order * [Server] Send game-joined player list in seat order * [Client] Reword the shuffle option to match its behaviour * [Client] Address the seat-order review comments - Give the game setup checkboxes a row of their own so they no longer collide in the create-game dialog - Rename DlgTurnOrder::randomize() to isRandomizeRequested() - Document what the ids in GameState::seatOrder mean - Keep "Set turn order..." in sync with the game: hidden for non-hosts and once the game has started, re-shown when the game ends or hosting moves to the local player (new GameState::hostChanged signal) * [Client] Do not preview a randomized turn order Randomizing is a request to the server, not a local shuffle. Shuffling the list locally meant the dialog showed an order the server then discarded, and let the host re-open it until a favourable order appeared. The dialog now switches to a mode that says the server will randomize and toggles back to the manual list on request. * [Client] Let moderators set the turn order The server accepts SET_PLAYER_ORDER from the host or a moderator, but the client hid the action from everyone except the host, making the moderator branch unreachable. Visibility and the handler now share canSetTurnOrder(). * [Server] Rely on the player order event instead of a full state resend cmdSetPlayerOrder re-sent the whole game state after reseating, so every client reloaded its deck view mid-lobby for no reason. The new seating reaches clients through the Event_PlayerOrderChanged that reseatPlayers broadcasts. * [Client] Show random turn order in the game selector The restrictions column listed "open decklists" but not the new seating option, so a joining player had no way to see that seats will be randomized each game. Game info already shows the disabled checkbox. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-04 15:44:50 +01:00
BruebachL	8822ade49d	[Tests] Register each test case with CTest instead of each test binary (#7399) ... * [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 <Bruebach.Lukas@bdosecurity.de> --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-11 14:50:09 +02:00
Vorliz	a20f3c0fb4	Fix #6659: Correct logging for bottom-of-library card moves (#6764) ... * Fix #6659: Correct logging for bottom-of-library card moves Cause: - This issue happens due to logic of moving the card from the top of the deck being reused when moving from the bottom of the deck, in a way that makes it impossible to check if the card came from the bottom. Resolution: - Updated the logging logic in the client for card moves. - Added a gRPC parameter ('is_from_bottom') for card moves. - Updates the server logic to reverse the order of the card move if the 'is_from_bottom' parameter is true. - Added a test to show the expected behaviour of the fix. NOTE: While the changes in this patch seem big, this is due to changing the loop in the moveCard function to a helper function, in order to make the bug fix change. The only change to the loop was to pass a variable attribution to the moveCard function because it was redundant to be in the loop. * chore: run format on test * refactor: new way to check if a move is from the bottom of the deck * refactor: change isFromBottom check to static function * update comments Co-authored-by: ebbit1q <ebbit1q@gmail.com> --------- Co-authored-by: ebbit1q <ebbit1q@gmail.com>	2026-04-21 19:05:31 +02:00