test: add unit tests for apply_move_to_track()

[wopdevries]

Extract common track-update logic from MakeSpecific, MakeNext, and
MakeNextSimple into apply_move_to_track().

• 128 lines of duplicated logic → single function
• Zero performance regression (benchmark: solve equal, calc +0.6% noise)
• 41/41 library tests pass
• Added 2 unit tests for apply_move_to_track() covering lead hand and
  follow suit scenarios

Closes #230 (Quick-win refactoring item)

[wopdevries]

All CI checks pass. Ready for review and merge when you have time.

[Copilot]

Pull request overview

Refactors move-tracking state updates by extracting the shared logic from Moves::MakeSpecific, Moves::MakeNext, and Moves::MakeNextSimple into a single helper (apply_move_to_track()), and adds unit tests to validate key scenarios.

Changes:

• Added Moves::apply_move_to_track() and rewired MakeSpecific / MakeNext / MakeNextSimple to use it.
• Added 2 unit tests for apply_move_to_track() covering lead-hand and follow-suit behavior.
• Reduced duplicated code in moves.cpp by centralizing per-card track updates.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
library/src/moves/moves.cpp	Extracts and uses apply_move_to_track() for track updates across move-selection paths.
library/src/moves/moves.hpp	Declares the new helper in the Moves interface.
library/tests/moves/moves_test.cpp	Adds unit tests for the new track-update helper.

[tameware]

I requested a Copilot review. I expect its second and third comments apply equally to the original code that you refactored. If so, you can address them in this PR to keep it as a pure refactor or add an issue to address them in the future - your choice.

The two phrases in Copilot's third comment seem equivalent, so I asked Cursor for an opinion:

Yes — Copilot is right.

The lead is Ace (rank = 14), the follow is King (rank = 13). Ace beats King, so the lead stays winning. The next lines already say that:

// King < Ace so high stays at 0 (lead hand wins)
EXPECT_EQ(moves->trackp->high[1], 0);

So // Follow with King of Spades — higher rank wins is wrong; // Follow with King of Spades — lower rank loses matches the test.

I won't have any other look at the PR myself until Copilot's comments are resolved.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

library/src/moves/moves.cpp:448

• apply_move_to_track() takes a trick index but relies on the caller to have already set trackp. That makes the API easy to misuse (trackp could point at a different trick than the provided trick), and in non-debug builds the assert won’t protect against null/incorrect trackp. Consider binding trackp from the trick argument inside this helper (and validating the trick index) so the precondition is enforced in one place.

auto Moves::apply_move_to_track(const MoveType &move, const int relHand,
                                const int trick) -> void {
  assert(trackp != nullptr && "apply_move_to_track: trackp must be set");
  assert(relHand >= 0 && relHand < DDS_HANDS);
  if (relHand == 3)

library/src/moves/moves.hpp:347

• The docstring for apply_move_to_track() says trick is in (1..12), but other code/tests index tricks with 0..12 (arrays are sized [13]). This comment should match the actual allowed range and call out the additional constraint only when relHand==3 (since the helper writes track[trick - 1]).

     * @brief Update TrackType state to reflect a played move.
     *
     * @param move Move to apply
     * @param relHand Relative hand index within the current trick (0..3)
     * @param trick Trick index (1..12); when relHand==3 updates next trick state
     */

library/tests/moves/moves_test.cpp:611

• The added unit tests cover leading and following suit, but they don’t exercise the two other important branches in apply_move_to_track(): (1) trump-played beats non-trump, and (2) relHand==3 completion updates track[trick - 1] (lead_hand and removed_ranks). Adding coverage for those branches would better lock in the refactor’s behavior and guard against regressions in MakeNext/MakeNextSimple.

TEST_F(MovesTest, ApplyMoveToTrackFollowSuit) {
  const unsigned short (*rankInSuit)[4] = getSampleRankInSuit();
  moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);
  moves->trackp = &moves->track[5];

[tameware]

First two new Copilot comments could be either addressed or postponed. Third seems relevant to this PR.