Cleanup+ fix randomly failing tests because of reset_for_tests caus…

[artemijan]

…e it resets all the state for other tests in parallel

Summary by CodeRabbit

• Bug Fixes

  • Improved client status handling during restart operations.
  • Added validation guards for movement operations to prevent invalid state transitions.
  • Enhanced error messaging for movement failures.

• Tests

  • Updated test infrastructure to ensure proper test execution isolation.

• Chores

  • Updated testing dependencies for better compatibility.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR replaces the ntest test utility crate with serial_test across workspace dependencies, removes corresponding test imports, and adds serial test attributes to ensure test isolation. Additionally, PlayerClient movement logic is hardened with InGame status guards, and restart handling now sets authenticated status before broadcasting.

Changes

Cohort / File(s)	Summary
Dependency Migration: ntest → serial_test Cargo.toml, game/Cargo.toml, l2-core/Cargo.toml, login/Cargo.toml	Replaced ntest = "0.9.3" with serial_test = "3.2.0" in workspace dependencies; removed ntest dev-dependency from game/Cargo.toml; added serial_test.workspace = true to l2-core/Cargo.toml dev-dependencies; removed ntest workspace member from login/Cargo.toml.
Test Import Cleanup game/src/packets/handleable/gs_auth_response.rs, game/src/packets/handleable/init_ls.rs	Removed unused ntest::timeout imports from test modules; no runtime behavior changes.
Test Serialization l2-core/src/id_factory.rs	Added #[serial_test::serial] attribute to test_allocation and test_reuse functions to enforce serial test execution.
PlayerClient Movement Guards & Restart Logic game/src/pl_client.rs, game/src/packets/from_client/restart.rs	Added InGame status checks to start_movement and GetMovementPosition handler with early bailout; imported Duration directly; expanded error messages; updated restart handler to set ClientStatus::Authenticated status before broadcasting deletion.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Movement guard logic (game/src/pl_client.rs): Verify InGame state checks are positioned correctly and don't introduce unintended side effects in movement flow
• Restart status update (game/src/packets/from_client/restart.rs): Ensure ClientStatus::Authenticated is the correct state to set before broadcasting and confirm no race conditions exist
• Dependency swap completeness: Confirm all ntest references are fully replaced and serial_test integration is consistent across workspace members

Possibly related PRs

• Fb broadcast move to location #21: Modifies overlapping PlayerClient movement logic (start_movement/stop_movement, GetMovementPosition) and restart packet handling; likely addresses related concerns or complements these changes.

Poem

🐰 We swapped our test crate for something more true,
Serial tests now guard what we do,
PlayerClient's movement now checked before go,
Authenticated restarts let players know! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title is truncated ('Cleanup+ fix randomly failing tests because of reset_for_tests caus…') and appears incomplete, making it vague about the actual changes. While it references the core issue (test failures and reset_for_tests), the truncation obscures the full context and main intent of the PR.	Complete the title with the full message. The current truncated version doesn't convey the complete idea. Consider: 'Fix randomly failing tests by using serial_test instead of ntest for test isolation.'

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fixes

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 267cf29 and 2d3d1af.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (9)

• Cargo.toml (1 hunks)
• game/Cargo.toml (0 hunks)
• game/src/packets/from_client/restart.rs (2 hunks)
• game/src/packets/handleable/gs_auth_response.rs (0 hunks)
• game/src/packets/handleable/init_ls.rs (0 hunks)
• game/src/pl_client.rs (4 hunks)
• l2-core/Cargo.toml (1 hunks)
• l2-core/src/id_factory.rs (2 hunks)
• login/Cargo.toml (0 hunks)

💤 Files with no reviewable changes (4)

• game/src/packets/handleable/gs_auth_response.rs
• login/Cargo.toml
• game/src/packets/handleable/init_ls.rs
• game/Cargo.toml

🔇 Additional comments (6)

game/src/pl_client.rs (2)

341-343: Good defensive guard for movement state.

The InGame status check prevents movement from starting when the PlayerClient is in an invalid state, which aligns with the PR's goal of fixing test isolation issues.

---

594-597: Proper cleanup before bailing.

Calling stop_movement() before bailing ensures the movement task is properly cancelled when the player is not in game. This defensive approach prevents dangling tasks.

game/src/packets/from_client/restart.rs (1)

36-36: Correct status transition on restart.

Setting the status to Authenticated before broadcasting ensures the PlayerClient state is consistent when returning to character selection. This prevents movement logic from executing in an invalid state during the restart flow.

l2-core/src/id_factory.rs (1)

109-109: Good use of serial test attributes.

The #[serial_test::serial] attributes on test_allocation and test_reuse correctly ensure these tests run serially, preventing race conditions when calling reset_for_tests() on the global singleton.

Also applies to: 120-120

l2-core/Cargo.toml (1)

46-46: LGTM!

The serial_test dev-dependency correctly enables the #[serial_test::serial] attributes used in the test files.

Cargo.toml (1)

62-62: No known security vulnerabilities exist for serial_test 3.2.0.

Web search of public advisory databases (GitHub Advisory Database, RustSec, NVD) found no security advisories or CVEs for this version. The crate documentation on docs.rs confirms version 3.2.0 exists and is maintained. If you require additional assurance, run cargo-audit against your project's dependency graph to verify against the latest advisory data.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}