Cleaned up engine tests #41
Conversation
|
""" WalkthroughThe test suite for UCI chess engines was refactored to support multiple engines by dynamically checking for their availability and running tests as subtests per engine. Test cases were updated for consistency, expected move changes, and multipv settings. The logging test and related code were removed, and unused imports were cleaned up. Changes
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
uci/engine_test.go (2)
132-132: Consider using a more descriptive variable name.Using
err2doesn't provide clear context about what this error represents. Consider using a more descriptive name likerunErrorcmdErr.
130-131: Consider adding a test timeout.The test is running chess games until completion, which could potentially run for a very long time or even forever in some edge cases. Consider adding a test timeout or a maximum number of moves to prevent long-running tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
uci/engine_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: vulncheck
🔇 Additional comments (6)
uci/engine_test.go (6)
1-2: Good use of build tags for conditional testing.Using build tags instead of skipping tests is a more efficient approach, as tagged tests are conditionally compiled rather than compiled and then skipped at runtime.
7-7: Appropriate import addition for engine availability check.Adding the
os/execpackage is necessary for theLookPathfunctionality used to check engine availability.
15-15: Improved test flexibility with multiple engines.Supporting multiple engines makes the tests more comprehensive and robust.
17-20: Good implementation of engine availability check.The
isEngineAvailablefunction elegantly handles the engine availability verification usingexec.LookPath.
22-57: Test refactoring improves maintainability and coverage.The test has been well-refactored to support multiple engines with proper subtest structure and appropriate skipping when engines aren't available.
59-105: Consistent test structure with appropriate assertions.This test follows the same good pattern as the previous test, with appropriate expectations for multipv results.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
uci/engine_test.go (1)
110-144: Test logic fixed to be outside the skip condition.The test code has been moved outside the skip condition, addressing the issue raised in the previous review. The test now correctly skips when engines aren't available and runs the test logic only when they are.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
uci/engine_test.go(1 hunks)
🔇 Additional comments (10)
uci/engine_test.go (10)
1-2: Good approach using build tags for slow engine tests.Using the
//go:build enginetag allows developers to choose when to run these integration tests, which is great for CI performance. This is a good improvement from the previous approach.
7-7: Import change appropriate for new functionality.Adding the
os/execimport is necessary for the newexec.LookPathfunctionality used to check engine availability.
15-15: Good approach for supporting multiple engines.Using a slice of engine names makes it easy to add or remove engines in the future without modifying the test logic.
17-20: Well-implemented engine availability check.The
isEngineAvailablehelper function correctly usesexec.LookPathto check if the engine is available in the system PATH, making the tests robust against missing dependencies.
23-29: Tests now properly run as subtests for each engine.The test now uses Go's subtest functionality to run for each configured engine, with appropriate skipping when an engine is not available. This is a good improvement for test organization and clarity.
24-24: Test position updated for better engine compatibility.The FEN position used for testing has been updated, which is likely more appropriate for testing with both Stockfish and Lc0 engines.
42-47: Updated MultiPV setting for better test stability.The MultiPV option is now set to 2 (previously 3), which is adequate for the test cases and likely more stable across different engines.
49-54: Expected move has been updated to match the new position.The test now expects "Ne5" as the best move which corresponds to the updated FEN position. This makes the test more accurate for the multiple engine setup.
88-90: Test expectation updated to match multipv setting.The test now correctly expects 2 MultiPV lines to match the
multipvvalue of 2 set earlier, ensuring logical consistency in the test.
98-102: Secondary move expectation added for multipv testing.Testing the second best move "e5" ensures the MultiPV functionality works correctly across different engines.
| for game.Outcome() == chess.NoOutcome { | ||
| cmdPos := uci.CmdPosition{Position: game.Position()} | ||
| cmdGo := uci.CmdGo{MoveTime: time.Second / 100} | ||
| if err2 := eng.Run(cmdPos, cmdGo); err2 != nil { | ||
| t.Fatal("failed to run command", err2) | ||
| } | ||
|
|
||
| move := eng.SearchResults().BestMove | ||
| pos := game.Position() | ||
| san := notation.Encode(pos, move) | ||
|
|
||
| err = game.PushMove(san, nil) | ||
| if err != nil { | ||
| t.Fatal(fmt.Sprintf("failed to push move %s - %s - %v. Pos: %s", san, move.String(), move.HasTag(chess.Capture), pos.String()), err) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify game termination in the test loop.
The test loop runs until the game has an outcome, but it might be good to add a safety measure like a maximum number of moves to prevent potential infinite loops if an engine behaves unexpectedly.
🏁 Script executed:
#!/bin/bash
# Verify if there's a risk of infinite loops in this test
# Look for similar patterns in the code that use a game outcome loop with a safety limit
rg "for .* Outcome\(\) == chess.NoOutcome" -A 5 --glob "*.go" | grep -E "count|limit|max"Length of output: 91
Add a maximum move limit in the test loop to prevent infinite execution
The loop in uci/engine_test.go (around lines 129–144) currently relies solely on game.Outcome() to terminate, which could hang indefinitely if the engine misbehaves. Introduce a simple counter guard:
• File: uci/engine_test.go (lines 129–144)
• Add at the top of the test:
const maxMoves = 200
moves := 0• Wrap the loop with a move-count check:
for game.Outcome() == chess.NoOutcome {
+ if moves++; moves > maxMoves {
+ t.Fatalf("test loop exceeded max moves (%d)", maxMoves)
+ }
cmdPos := uci.CmdPosition{Position: game.Position()}
cmdGo := uci.CmdGo{MoveTime: time.Second / 100}
…
}This ensures the test fails fast if it ever exceeds a reasonable move limit.
2 participants
Just some clean up for the engine tests.
Summary by CodeRabbit
Refactor
Bug Fixes