Skip to content

Fix regression & Improve test coverage related to 'DijkstraMap'#100

Merged
utilForever merged 2 commits into
utilForever:mainfrom
ChrisLee02:Chrislee02/dijkstra-test
May 20, 2026
Merged

Fix regression & Improve test coverage related to 'DijkstraMap'#100
utilForever merged 2 commits into
utilForever:mainfrom
ChrisLee02:Chrislee02/dijkstra-test

Conversation

@ChrisLee02
Copy link
Copy Markdown
Contributor

@ChrisLee02 ChrisLee02 commented May 19, 2026

What

Restore Dijkstra start tiles as seeded distance values and add regression coverage for the restored behavior.

Why

DijkstraMap.map is public distance data, but the current build paths only write neighboring tiles during expansion. That leaves start tiles with a return-path cost instead of the expected seed distance, which can affect direct map reads and helper-based path selection.

This PR restores the intended seed behavior for build, build_weighted, and threaded build_parallel, then locks the behavior with tests for constructor output, weighted starts, empty maps, clearing, and lowest-exit selection.

Closes #26

Checklist

Required

  • cargo check --all passes
  • cargo fmt --all -- --check passes
  • cargo clippy --workspace --all-targets -- -D warnings -A clippy::multiple-crate-versions passes
  • cargo test --all passes
  • I linked the related issue

Threaded Dijkstra path was verified separately with cargo test -p bracket-pathfinding --features threaded dijkstra::test -- --nocapture

Functional Validation

  • Behavior related to this change was verified locally (if applicable)
  • Rendering/backend behavior was verified when runtime code changed (if applicable)
  • Algorithm behavior (pathfinding/FOV/noise/random) was verified when affected (if applicable)
  • I added or updated tests for changed behavior (if applicable)

Configuration & Docs

  • User-facing docs were updated (README.md, ARCHITECTURE.md, or relevant manual pages, if applicable)
  • New dependencies/configuration are documented (if applicable)
  • No sensitive values or credentials were introduced

If Applicable

  • Security impact considered (run cargo audit locally if needed)
  • Breaking behavior changes are clearly described in this PR

Summary by CodeRabbit

  • Bug Fixes
    • Improved pathfinding algorithm reliability through corrected initialization of distance calculations at starting positions, ensuring more accurate navigation results in pathfinding operations.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

📝 Walkthrough

Walkthrough

The PR updates Dijkstra pathfinding initialization to explicitly set start tile distances before propagation and adds comprehensive unit test coverage. Unweighted and weighted builds now initialize distances with min(current, 0.0) and min(current, start_weight) respectively, applied across all three build methods. Five new unit tests validate initialization, clearing, and pathfinding correctness.

Changes

Start tile initialization and test coverage

Layer / File(s) Summary
Start tile distance initialization
bracket-pathfinding/src/dijkstra.rs
build, build_weighted, and parallel build now initialize dm.map at each start index using min(current, 0.0) or min(current, start_weight) before enqueueing, ensuring correct per-start distances prior to open list propagation.
Test module and unit test coverage
bracket-pathfinding/src/dijkstra.rs
Test module diagram updated; new unit tests test_new, test_new_weighted, test_new_empty, test_clear, and test_lowest_exit assert expected depth arrays, verify f32::MAX initialization and reset behavior, and confirm pathfinding choices from specific positions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A rabbit hops through paths so true,
Start tiles now know their way anew—
With zero distance, weights in place,
Tests sprint ahead with steady grace! 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: fixing a regression in DijkstraMap initialization and adding comprehensive test coverage to prevent future regressions.
Linked Issues check ✅ Passed The PR directly addresses issue #26 by adding multiple test cases (test_new, test_new_weighted, test_new_empty, test_clear, test_lowest_exit) to strengthen test coverage and validate DijkstraMap behavior.
Out of Scope Changes check ✅ Passed All changes are within scope: regression fix for DijkstraMap seeding behavior and test additions directly support the stated objectives of fixing the bug and improving test coverage.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

@ChrisLee02 ChrisLee02 force-pushed the Chrislee02/dijkstra-test branch from e39fbd4 to e6756de Compare May 19, 2026 11:39
@ChrisLee02 ChrisLee02 marked this pull request as ready for review May 19, 2026 12:02
@ChrisLee02 ChrisLee02 changed the title [WIP] Fix regression & Improve test coverage related to 'DijkstraMap' Fix regression & Improve test coverage related to 'DijkstraMap' May 19, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@bracket-pathfinding/src/dijkstra.rs`:
- Around line 355-407: Add a threaded-only unit test that exercises
DijkstraMap::build_parallel by supplying at least THREADED_REQUIRED_STARTS (>=4)
start positions; create a new #[test] function annotated with #[cfg(feature =
"threaded")] (e.g., fn test_new_threaded()) in the same tests module, construct
a MiniMap, call DijkstraMap::new (or new_weighted) with 4+ start indices, and
assert the resulting map contents or properties match expected values so the
parallel code path (DijkstraMap::build_parallel) is exercised and validated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0c27ed64-bd1b-4929-bbce-be1b39669f0b

📥 Commits

Reviewing files that changed from the base of the PR and between 5612251 and e6756de.

📒 Files selected for processing (1)
  • bracket-pathfinding/src/dijkstra.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Format all Rust code using cargo fmt --all
Fix all cargo clippy warnings — the CI enforces -D warnings
Add tests for new functionality in the relevant module; for split domains, prefer colocated tests.rs

Files:

  • bracket-pathfinding/src/dijkstra.rs
🔇 Additional comments (1)
bracket-pathfinding/src/dijkstra.rs (1)

151-153: LGTM!

Also applies to: 183-185, 230-232

Comment thread bracket-pathfinding/src/dijkstra.rs
Copy link
Copy Markdown
Owner

@utilForever utilForever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your work! 🧪

@utilForever utilForever merged commit 3a48473 into utilForever:main May 20, 2026
10 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request May 24, 2026
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

테스트 코드 보강하기

2 participants