Add Zobrist/Polyglot and improve tests #10
Conversation
This commit introduces three tests to ensure the correctness of PGN parsing. The tests validate behavior for valid PGN files, invalid PGN data, and handling of empty inputs. These additions improve the robustness and reliability of the PGN parsing functionality.
Replaced all instances of MoveStr with PushMove in tests for consistency and to use the updated method. Removed deprecated methods Move and MoveStr, as well as MakeMove, which are now redundant. This simplifies the codebase and aligns with the updated API.
Replaced the placeholder "TODO" with a call to g.FEN() to ensure the Game's String method returns the FEN representation. This aligns with expected behavior and improves the code's utility.
The moveSlice type and its associated find method were unused and redundant. This cleanup improves code readability and reduces unnecessary complexity.
Implemented a new test function to validate the Bytes method for various chess piece types. This ensures the method produces the correct byte representations for all supported piece types. Also updated imports to include the "bytes" package.
Introduce ChessHasher to compute Zobrist hashes for chess positions, supporting operations like piece placement, en passant, castling, and side to move. Add comprehensive unit tests to validate functionality, error handling, and known position cases. Include polyglot hash constants for hashing computations.
Introduced functionality to handle Polyglot opening books, including loading from various sources (file, memory, reader), parsing moves, finding moves by position, and weighted random move selection. Extensive tests were added to verify behavior across scenarios such as invalid data, move decoding, and position-based lookups.
Replaced the placeholder random number generator with a cryptographically secure implementation using crypto/rand. This ensures unpredictability and prevents potential exploitation during move selection. Added error handling to catch and report failures in random number generation.
WalkthroughThis pull request introduces significant architectural changes to the chess library, focusing on move handling, game state management, and the introduction of new components like Polyglot opening book support and Zobrist hashing. The modifications streamline move processing by centralizing validation and application logic, deprecate older methods like Changes
Sequence DiagramsequenceDiagram
participant Game
participant PushMove
participant MoveValidator
participant GameState
Game->>PushMove: Call with algebraic move
PushMove->>MoveValidator: Validate move
MoveValidator-->>PushMove: Validation result
PushMove->>GameState: Update game state
GameState-->>PushMove: State updated
PushMove-->>Game: Return result
Possibly Related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
+ Coverage 66.23% 73.71% +7.48%
==========================================
Files 25 28 +3
Lines 3865 4935 +1070
==========================================
+ Hits 2560 3638 +1078
+ Misses 1196 1167 -29
- Partials 109 130 +21 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
zobrist.go (3)
10-10: Rename ChessHasher to Hasher to avoid stutter.
According to static analysis (revive), the name "ChessHasher" stutters when used under the package "chess". Consider renaming it to "Hasher" to follow Go naming conventions.🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 10-10: exported: type name will be used as chess.ChessHasher by other packages, and that stutters; consider calling this Hasher
(revive)
125-198: Function hashPieces too long.
Static analysis warns that hashPieces has too many statements (62 > 50). Consider breaking it into smaller helper functions to enhance readability and maintainability.🧰 Tools
🪛 golangci-lint (1.62.2)
125-125: Function 'hashPieces' has too many statements (62 > 50)
(funlen)
132-132: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
135-135: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
210-210: Use errors.New instead of fmt.Errorf for fixed messages.
Where the error messages have no string formatting, using errors.New can be more direct and slightly more efficient, e.g.:- return "", fmt.Errorf("invalid FEN format") + return "", errors.New("invalid FEN format")Also applies to: 215-215, 233-233
🧰 Tools
🪛 golangci-lint (1.62.2)
210-210: fmt.Errorf can be replaced with errors.New
(perfsprint)
polyglot.go (1)
198-198: Use errors.Is for checking io.EOF.
Directly comparing err with io.EOF may fail if err is wrapped. Consider using errors.Is for robust error checks:-if err == io.EOF { +if errors.Is(err, io.EOF) {🧰 Tools
🪛 golangci-lint (1.62.2)
198-198: comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error
(errorlint)
polyglot_test.go (3)
144-193: Consider adding weight-based test casesThe
TestFindMovesfunction could benefit from additional test cases:
- Equal weight moves to verify stable sorting
- Zero weight moves to verify handling
261-278: Add more invalid data test cases
TestInvalidBookDatacould be enhanced with additional cases:
- Corrupted entry data
- Invalid move encodings
- Maximum file size handling
1-278: Consider splitting test file for better organizationThe test file covers multiple distinct functionalities. Consider splitting it into:
polyglot_source_test.go: Book source testspolyglot_move_test.go: Move decoding and finding testspolyglot_book_test.go: Book loading and validation tests🧰 Tools
🪛 golangci-lint (1.62.2)
63-63: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error
(errorlint)
62-62: ineffectual assignment to n
(ineffassign)
zobrist_test.go (3)
40-79: Consider adding test cases for more complex positions.The known position tests cover standard openings well. Consider adding test cases for:
- Complex middlegame positions
- Endgame positions with various piece configurations
- Positions with multiple pawns and pieces
149-180: Consider adding more en passant test cases.While the current tests cover basic en passant scenarios, consider adding:
- Multiple possible en passant squares
- Invalid en passant squares on wrong ranks
208-239: Enhance piece placement tests with more edge cases.The piece placement tests could be expanded to include:
- Multiple pieces of the same type
- Pieces on edge squares
- Maximum number of pieces of each type
🧰 Tools
🪛 golangci-lint (1.62.2)
230-230: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
hashes.go (2)
3-787: Consider using a more maintainable approach for hash values.While the current implementation works, consider:
- Storing hash values in a separate file (e.g., JSON, YAML) and loading at runtime
- Using a const array or generating values programmatically
- Adding documentation about the source and purpose of these hash values
This would improve maintainability and make the code more readable.
🧰 Tools
🪛 golangci-lint (1.62.2)
3-3: Function 'GetPolyglotHashes' is too long (783 > 100)
(funlen)
1-787: Add documentation for the hash values.Please add:
- Function documentation explaining the purpose and usage of these hashes
- Source reference for the hash values
- Any specific requirements or constraints for using these values
🧰 Tools
🪛 golangci-lint (1.62.2)
3-3: Function 'GetPolyglotHashes' is too long (783 > 100)
(funlen)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
game.go(1 hunks)game_test.go(2 hunks)hashes.go(1 hunks)move.go(0 hunks)opening/opening_test.go(4 hunks)piece.go(1 hunks)piece_test.go(2 hunks)polyglot.go(1 hunks)polyglot_test.go(1 hunks)zobrist.go(1 hunks)zobrist_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- move.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
zobrist_test.go
8-8: cyclomatic complexity 33 of func TestChessHasher is high (> 30)
(gocyclo)
230-230: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
polyglot_test.go
63-63: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error
(errorlint)
62-62: ineffectual assignment to n
(ineffassign)
zobrist.go
38-38: Error return value of fmt.Sscanf is not checked
(errcheck)
125-125: Function 'hashPieces' has too many statements (62 > 50)
(funlen)
[warning] 10-10: exported: type name will be used as chess.ChessHasher by other packages, and that stutters; consider calling this Hasher
(revive)
60-60: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
132-132: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
135-135: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
35-35: Magic number: 2, in detected
(mnd)
97-97: Magic number: 780, in detected
(mnd)
210-210: fmt.Errorf can be replaced with errors.New
(perfsprint)
215-215: fmt.Errorf can be replaced with errors.New
(perfsprint)
233-233: fmt.Errorf can be replaced with errors.New
(perfsprint)
polyglot.go
27-27: Comment should end in a period
(godot)
69-69: Comment should end in a period
(godot)
77-77: Comment should end in a period
(godot)
92-92: Comment should end in a period
(godot)
107-107: Comment should end in a period
(godot)
112-112: Comment should end in a period
(godot)
117-117: Comment should end in a period
(godot)
123-123: Comment should end in a period
(godot)
128-128: Comment should end in a period
(godot)
198-198: comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error
(errorlint)
197-197: shadow: declaration of "err" shadows declaration at line 183
(govet)
195-195: Magic number: 16, in detected
(mnd)
332-332: Magic number: 0x7, in detected
(mnd)
333-333: Magic number: 0x7, in detected
(mnd)
334-334: Magic number: 0x7, in detected
(mnd)
335-335: Magic number: 0x7, in detected
(mnd)
336-336: Magic number: 0x7, in detected
(mnd)
337-337: Magic number: 0x7, in detected
(mnd)
386-386: Magic number: 4, in detected
(mnd)
93-93: named return "n" with type "int" found
(nonamedreturns)
129-129: named return "n" with type "int" found
(nonamedreturns)
162-162: named return "n" with type "int" found
(nonamedreturns)
hashes.go
3-3: Function 'GetPolyglotHashes' is too long (783 > 100)
(funlen)
🔇 Additional comments (11)
zobrist.go (2)
67-69: Magic numbers in xorHash.
The index passed to GetPolyglotHashes()[num] is a magic number. Ensure that 780 or other values used in the code correspond to correct offsets into the pre-defined polyglot array.
35-41: Check the error return from fmt.Sscanf.
The call to "fmt.Sscanf(s[i:i+2], "%02x", &value)" should check the error return value to detect malformed hex strings and handle them accordingly.
🧰 Tools
🪛 golangci-lint (1.62.2)
38-38: Error return value of fmt.Sscanf is not checked
(errcheck)
35-35: Magic number: 2, in detected
(mnd)
game.go (1)
291-291: Clarify String method's usage.
The String() method now returns FEN notation. If the previously expected behavior was to return a PGN or something else, ensure the change is intentional.
piece_test.go (2)
3-3: Import "bytes" for clarity.
Importing "bytes" is correct for comparing slices, and everything looks good here.
28-47: Bytes() test coverage for NoPieceType.
It's great that the test includes NoPieceType. This ensures that edge cases are covered, which improves overall robustness.
opening/opening_test.go (1)
13-14: LGTM! Consistent migration from MoveStr to PushMove
The changes correctly replace the deprecated MoveStr method with PushMove across all test functions, maintaining the same behavior while allowing for future extensibility through the options parameter.
Also applies to: 26-27, 44-47, 60-61
piece.go (1)
17-25: LGTM! Well-implemented color string conversion
The ColorFromString function is a clean implementation that:
- Handles case-insensitivity correctly
- Returns
NoColorfor invalid inputs - Maintains consistency with other string conversion functions in the file
polyglot_test.go (1)
13-30: LGTM! Well-structured test helpers
The helper functions createEntry and createTestBook are well-implemented and provide a clean way to create test data.
zobrist_test.go (2)
8-38: LGTM! Well-structured basic position validation tests.
The test cases effectively validate the core functionality:
- Starting position hash verification
- Empty board position hash verification
- Hash length validation
🧰 Tools
🪛 golangci-lint (1.62.2)
8-8: cyclomatic complexity 33 of func TestChessHasher is high (> 30)
(gocyclo)
81-107: LGTM! Comprehensive error handling tests.
Good coverage of error cases:
- Invalid FEN format
- Invalid piece placement
- Invalid ranks count
game_test.go (1)
1007-1013: LGTM! Clear empty data test.
Good verification of the ErrNoGameFound error case.
| _, err := source.Read(buf) | ||
| if err == io.EOF { | ||
| break | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Variable shadowing for err on line 197.
The declaration of err here shadows the outer scope variable, potentially causing confusion. Rename the inner variable to a different identifier to avoid accidental misuse.
-for {
- _, err := source.Read(buf)
+for {
+ _, readErr := source.Read(buf)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
198-198: comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error
(errorlint)
197-197: shadow: declaration of "err" shadows declaration at line 183
(govet)
|
|
||
| // Test EOF | ||
| source.index = 32 | ||
| n, err = source.Read(buf) |
There was a problem hiding this comment.
Remove ineffectual assignment
The n variable assignment is unused in the EOF test case.
Apply this fix:
- n, err = source.Read(buf)
+ _, err = source.Read(buf)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| n, err = source.Read(buf) | |
| _, err = source.Read(buf) |
🧰 Tools
🪛 golangci-lint (1.62.2)
62-62: ineffectual assignment to n
(ineffassign)
| source.index = 32 | ||
| n, err = source.Read(buf) | ||
| if err != io.EOF { | ||
| t.Errorf("Read() error = %v, want EOF", err) | ||
| } |
There was a problem hiding this comment.
Fix error handling in EOF test case
The error comparison should use errors.Is for proper error wrapping support.
Apply this fix:
- if err != io.EOF {
+ if !errors.Is(err, io.EOF) {
t.Errorf("Read() error = %v, want EOF", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| source.index = 32 | |
| n, err = source.Read(buf) | |
| if err != io.EOF { | |
| t.Errorf("Read() error = %v, want EOF", err) | |
| } | |
| source.index = 32 | |
| n, err = source.Read(buf) | |
| if !errors.Is(err, io.EOF) { | |
| t.Errorf("Read() error = %v, want EOF", err) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
63-63: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error
(errorlint)
62-62: ineffectual assignment to n
(ineffassign)
| func TestPGNWithValidData(t *testing.T) { | ||
| pgnData := mustParsePGN("fixtures/pgns/single_game.pgn") | ||
| r := strings.NewReader(pgnData) | ||
| updateFunc, err := PGN(r) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| g := NewGame() | ||
| updateFunc(g) | ||
| if g.Outcome() != WhiteWon { | ||
| t.Fatalf("expected outcome %s but got %s", WhiteWon, g.Outcome()) | ||
| } | ||
| if g.Method() != NoMethod { | ||
| t.Fatalf("expected method %s but got %s", NoMethod, g.Method()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more assertions for game state.
While the test verifies the outcome and method, consider adding assertions for:
- Move history
- Position after specific moves
- Tag pairs from the PGN
| func TestPGNWithInvalidData(t *testing.T) { | ||
| pgnData := "1. e4 e5 2. Nf3 Nc6 3. Bb5 a6 4. Ba4 Nf6 5. O-O Be7 6. Re1 b5 7. Bb3 d6 8. c3 O-O 9. h3 Nb8 10. d4 Nbd7 11. c4 c6 12. cxb5 axb5 13. Nc3 Bb7 14. Bg5 h6 15. Bh4 Re8 16. a3 Bf8 17. Rc1 Qb6 18. dxe5 dxe5 19. Qe2 Nh5 20. Qd2 Nc5 21. Bc2 Nf4 22. Bg3 Rad8 23. Qe3 Qc7 24. Rcd1 Rxd1 25. Rxd1 Nce6 26. Bb3 Bc5 27. Qe1 Nd4 28. Nxd4 Bxd4 29. Bxf4 exf4 30. Rxd4 c5 31. Rd1 c4 32. Bc2 Qe5 33. f3 Qc5+ 34. Qf2 Qe5 35. Qd4 Qg5 36. Qd7 Re7 37. Qd8+ Kh7 38. e5+ g6 39. Qd6 Bxf3 40. Rd2 Rxe5 41. Qd4 Re1+ 42. Kf2 Qg3# 0-1" | ||
| r := strings.NewReader(pgnData) | ||
| _, err := PGN(r) | ||
| if err == nil { | ||
| t.Fatal("expected error for invalid PGN data") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test specific PGN parsing error cases.
The test could be more specific about what makes the PGN data invalid. Consider testing:
- Malformed move notation
- Invalid move sequences
- Missing required fields
Summary by CodeRabbit
New Features
PushMove, replacing previous methods.Colortype.Bug Fixes
Tests
Chores