Replace unstable benches with Criterion

[oshikiri]

Summary

• remove the nightly-only benchmark module from tests/board.rs
• add a Criterion-powered benchmark in engine/benches/board_bench.rs
• configure Cargo to use Criterion for the new bench without the unstable harness

Testing

• cargo test (engine)
• cargo bench --bench board_bench (engine)

---

https://chatgpt.com/codex/tasks/task_e_6900bff69068832ba8f77f8a7fa87bd4

Summary by CodeRabbit

• Chores
  • Added a modern benchmarking tool as a development dependency to support performance testing.
  • Introduced a new benchmark to measure board move-generation performance.
  • Removed legacy benchmark/test scaffolding previously used for nightly benchmarking.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Migrates benchmarking from nightly Rust benches to Criterion: adds Criterion as a dev-dependency, introduces a new Criterion benchmark file for Board::get_all_legal_moves, and removes the legacy nightly bench module and feature gate from tests.

Changes

Cohort / File(s)	Summary
Benchmark Manifest engine/Cargo.toml	Adds criterion as a dev-dependency and registers a new board_bench bench with harness = false.
Criterion benchmark engine/benches/board_bench.rs	New Criterion benchmark measuring Board::get_all_legal_moves, constructs a board from string input and uses black_box in the benchmark loop; exports criterion_group! and criterion_main!.
Legacy bench removal engine/tests/board.rs	Removes the nightly-only benches module and the #![feature(test)] attribute; unit tests remain unchanged.

Sequence Diagram(s)

sequenceDiagram
  participant Dev as Developer
  participant Cargo as Cargo
  participant Criterion as Criterion runner
  participant OldBench as Nightly bench harness
  rect rgba(46,139,87,0.08)
    Note over Cargo,Criterion: New flow (post-change)
    Dev->>Cargo: cargo bench
    Cargo->>Criterion: run `engine/benches/board_bench.rs` (harness=false)
    Criterion->>Board: call get_all_legal_moves (bench loop)
  end
  rect rgba(220,20,60,0.06)
    Note over Cargo,OldBench: Old flow (removed)
    Dev->>Cargo: cargo bench
    Cargo->>OldBench: run #[bench] harness (nightly/test)
    OldBench->>Board: call get_all_legal_moves (removed)
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Areas to spot-check:
  • engine/Cargo.toml bench entry and harness = false correctness.
  • engine/benches/board_bench.rs usage of black_box and Criterion wiring.
  • Ensure removal in engine/tests/board.rs didn't remove any unit tests.

Poem

🐇 I hopped from feature-gated night,
To Criterion's steady light—
Bench file planted, harness neat,
Measurements now calm and sweet. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Replace unstable benches with Criterion" directly and accurately summarizes the main change in the changeset. The title is specific and clear, indicating that unstable (nightly-only) benchmarks are being replaced with Criterion-based benchmarking. This aligns with the actual changes: removing the benchmark module from engine/tests/board.rs (which used unstable test crate features), adding a new Criterion benchmark in engine/benches/board_bench.rs, and updating Cargo.toml to configure Criterion. The title is concise, avoids vague language, and provides enough context for a teammate reviewing git history to understand the primary objective without needing to inspect the details.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch codex/run-node.js-tests

---

📜 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 f42a7b5 and 6bb7c63.

📒 Files selected for processing (1)

• engine/Cargo.toml (2 hunks)

🔇 Additional comments (2)

engine/Cargo.toml (2)

29-29: ✓ Criterion version updated to latest stable.

The previous critical issue regarding outdated Criterion version has been resolved. Version 0.7 is the current stable release and is properly specified.

---

41-43: ✓ Benchmark configuration correctly structured for Criterion.

The [[bench]] section is properly configured:

• name = "board_bench" correctly identifies the benchmark harness
• harness = false is required for Criterion (Criterion manages its own test harness)

This setup aligns with the Criterion-based benchmark implementation.

Please verify that cargo bench --bench board_bench runs successfully in the engine directory to confirm the benchmark builds and executes without errors.

---

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.}

[Copilot]

Pull Request Overview

This PR migrates the benchmark suite from Rust's unstable nightly-only test feature to Criterion, a stable benchmarking framework. This eliminates the need for nightly Rust when running benchmarks.

Key changes:

• Removed the #![feature(test)] attribute and the benches module from tests/board.rs
• Created a new Criterion-based benchmark in benches/board_bench.rs that tests get_all_legal_moves performance
• Added Criterion as a dev dependency and configured the benchmark harness in Cargo.toml

Reviewed Changes

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

File	Description
engine/tests/board.rs	Removed nightly-only benchmark module and feature flag
engine/benches/board_bench.rs	Added new Criterion benchmark for get_all_legal_moves
engine/Cargo.toml	Added Criterion dev dependency and benchmark configuration

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

engine/benches/board_bench.rs (1)

5-25: LGTM!

The benchmark implementation is correct:

• Properly uses black_box to prevent compiler optimizations
• Tests a representative board state
• Focuses on measuring get_all_legal_moves performance

Optionally, you could benchmark multiple board states (early game, mid-game, endgame) to get more comprehensive performance metrics, but this is not necessary for the initial migration.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d916b64 and f42a7b5.

📒 Files selected for processing (3)

• engine/Cargo.toml (2 hunks)
• engine/benches/board_bench.rs (1 hunks)
• engine/tests/board.rs (0 hunks)

💤 Files with no reviewable changes (1)

• engine/tests/board.rs

🧰 Additional context used

🧬 Code graph analysis (1)

engine/benches/board_bench.rs (1)

engine/src/board.rs (1)

• create_from_str (315-332)

🔇 Additional comments (3)

engine/Cargo.toml (1)

41-43: LGTM!

The benchmark configuration is correct. Setting harness = false is required for Criterion benchmarks, and the name matches the benchmark file.

engine/benches/board_bench.rs (2)

1-3: LGTM!

All necessary imports are present and correctly structured.

---

27-28: LGTM!

The Criterion setup is correct and follows the standard pattern for defining and running benchmark groups.