Feat/polyglot utils #17
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request reinstates a removed import in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PolyglotBook
Client->>PolyglotBook: AddMove(positionHash, move, weight)
PolyglotBook-->>PolyglotBook: Insert move & re-sort entries
PolyglotBook->>Client: Confirm addition
sequenceDiagram
participant Client
participant ZobristHasher
Client->>ZobristHasher: HashPosition(fen)
ZobristHasher-->>ZobristHasher: Process FEN (xorHash, parseEnPassant, etc.)
ZobristHasher->>Client: Return hash string or error
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
polyglot.go (3)
84-108: Consider returning an error instead of silently returning an empty Move.
Silently returning an empty Move may mask decoding errors; returning an error or logging might provide better visibility.
422-435: Frequent re-sorting could impact performance on large insertion volumes.
If you plan to add a large number of moves repeatedly, consider a more efficient insertion mechanism or deferring sort operations.
464-476: Potentially handle Move decoding errors more explicitly.
If a decode fails inside the loop, you might lose the opportunity to inform the caller about invalid data.polyglot_test.go (2)
108-109: Handle potential errors to avoid silent failures.
Currently, the errors fromos.OpenandNewReaderBookSourceare ignored. Please verify or handle them to ensure proper test failure if something goes wrong.
283-347: Comprehensive coverage for Move → Polyglot encoding.
Consider shortening the test function name to something likeTestMoveToPolyglotfor readability.zobrist.go (2)
24-33: Enhance deprecation notice with migration guidance.The deprecation notice should include guidance on how to migrate from the old constructor to the new one.
-// Deprecated: Use NewZobristHasher instead +// Deprecated: Use NewZobristHasher instead. This constructor will be removed in a future version. +// Migration: Replace NewChessHasher() with NewZobristHasher() in your code.
227-227: Improve error message clarity.The error messages could be more descriptive to help users understand and fix the issues.
- return "", errors.New("invalid FEN format") + return "", errors.New("invalid FEN format: insufficient fields") - return "", errors.New("invalid piece placement") + return "", errors.New("invalid piece placement: incorrect number of pieces or invalid piece positions")Also applies to: 245-245
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pgn.go(1 hunks)piece.go(1 hunks)polyglot.go(3 hunks)polyglot_test.go(4 hunks)zobrist.go(8 hunks)zobrist_test.go(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- pgn.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
zobrist.go
137-137: Function 'hashPieces' has too many statements (62 > 50)
(funlen)
🔇 Additional comments (21)
polyglot.go (8)
43-43: No issues found in the updated documentation comment.
58-62: Struct definition looks good.
63-72: Ensure input validation for file/rank bounds.
IfMove.S1()orMove.S2()could ever yield out-of-range values, consider adding checks or clarifying assumptions.
74-82: Symmetrical encoding logic is consistent and clear.
401-420: Method appears straightforward and correct.
437-451: Verify whether you want to update all matching moves or just the first.
Currently, the loop updates every matching move. If only updating the first match is desired, break after the update.
453-462: Bulk deletion logic is clear and efficient.
478-490: Conversion to map is well-structured and straightforward.piece.go (1)
149-162: Promotion value mapping is consistent with polyglot standards.polyglot_test.go (10)
206-206: Regular move test looks correct.
215-215: Castling move test logic is valid and provides good coverage.
349-400: Encoding tests are thorough, verifying normal and promotion moves.
402-457: Conversion tests (PolyglotMove → Move) effectively cover promotions and castling.
459-503: Tests confirm correct initialization from map data.
505-541: AddMove test accurately verifies new entry insertion and sorting.
543-581: UpdateMove test reliably checks weight updates and error handling for missing moves.
583-615: DeleteMoves test properly cleans up all related moves.
617-656: Enforces correct retrieval of multiple moves, including promotions.
Please verify the order checks at lines 644 and 647; the indexing might appear reversed, but if it’s intentional for weighting or ordering, everything is fine.
658-703: Map conversion test demonstrates robust coverage.zobrist.go (1)
136-210: Complexity is justified by domain requirements.While the function exceeds the recommended statement count (62 > 50), breaking it down would likely reduce readability given that it handles chess-specific piece placement and en passant logic in a cohesive way.
🧰 Tools
🪛 golangci-lint (1.62.2)
137-137: Function 'hashPieces' has too many statements (62 > 50)
(funlen)
144-144: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
147-147: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
zobrist_test.go (1)
10-10: LGTM!The constructor updates are consistent throughout all test cases.
Also applies to: 41-41, 82-82, 110-110, 132-132, 150-150, 183-183, 209-209
| func ZobristHashToUint64(hash string) uint64 { | ||
| var result uint64 | ||
|
|
||
| // Ensure the input is exactly 16 hex digits | ||
| if len(hash) != 16 { | ||
| return 0 | ||
| } | ||
|
|
||
| // Try to parse the hash | ||
| if _, err := fmt.Sscanf(hash, "%016x", &result); err != nil { | ||
| return 0 | ||
| } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning an error for invalid input.
Silently returning 0 for invalid input could mask errors in the calling code. Consider returning an error to allow callers to handle invalid cases explicitly.
-func ZobristHashToUint64(hash string) uint64 {
+func ZobristHashToUint64(hash string) (uint64, error) {
var result uint64
// Ensure the input is exactly 16 hex digits
if len(hash) != 16 {
- return 0
+ return 0, errors.New("invalid hash length: must be exactly 16 hex digits")
}
// Try to parse the hash
if _, err := fmt.Sscanf(hash, "%016x", &result); err != nil {
- return 0
+ return 0, fmt.Errorf("invalid hash format: %v", err)
}
- return result
+ return result, nil
}📝 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.
| func ZobristHashToUint64(hash string) uint64 { | |
| var result uint64 | |
| // Ensure the input is exactly 16 hex digits | |
| if len(hash) != 16 { | |
| return 0 | |
| } | |
| // Try to parse the hash | |
| if _, err := fmt.Sscanf(hash, "%016x", &result); err != nil { | |
| return 0 | |
| } | |
| return result | |
| } | |
| func ZobristHashToUint64(hash string) (uint64, error) { | |
| var result uint64 | |
| // Ensure the input is exactly 16 hex digits | |
| if len(hash) != 16 { | |
| return 0, errors.New("invalid hash length: must be exactly 16 hex digits") | |
| } | |
| // Try to parse the hash | |
| if _, err := fmt.Sscanf(hash, "%016x", &result); err != nil { | |
| return 0, fmt.Errorf("invalid hash format: %v", err) | |
| } | |
| return result, nil | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17 +/- ##
==========================================
+ Coverage 73.29% 73.96% +0.67%
==========================================
Files 27 27
Lines 4957 5078 +121
==========================================
+ Hits 3633 3756 +123
+ Misses 1194 1188 -6
- Partials 130 134 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
polyglot.go (3)
430-443: Consider optimizing the insertion.Instead of appending and re-sorting the entire slice, consider using binary search to find the correct insertion point and insert the entry directly at that position.
Apply this diff to optimize the insertion:
func (book *PolyglotBook) AddMove(positionHash uint64, move Move, weight uint16) { entry := PolyglotEntry{ Key: positionHash, Move: MoveToPolyglot(move), Weight: weight, Learn: 0, } - book.entries = append(book.entries, entry) - // Re-sort after adding - sort.Slice(book.entries, func(i, j int) bool { - return book.entries[i].Key < book.entries[j].Key - }) + // Find insertion point using binary search + idx := sort.Search(len(book.entries), func(i int) bool { + return book.entries[i].Key >= positionHash + }) + // Insert at the correct position + book.entries = append(book.entries, PolyglotEntry{}) + copy(book.entries[idx+1:], book.entries[idx:]) + book.entries[idx] = entry }
445-459: Consider optimizing the update operation.Instead of linearly searching through all entries, consider using binary search to find the range of entries with the matching position hash.
Apply this diff to optimize the update operation:
func (book *PolyglotBook) UpdateMove(positionHash uint64, move Move, newWeight uint16) error { target := MoveToPolyglot(move) - updated := false - for i, entry := range book.entries { - if entry.Key == positionHash && entry.Move == target { - book.entries[i].Weight = newWeight - updated = true - } + // Find the range of entries with matching position hash + idx := sort.Search(len(book.entries), func(i int) bool { + return book.entries[i].Key >= positionHash + }) + if idx >= len(book.entries) || book.entries[idx].Key != positionHash { + return errors.New("move not found for update") } - if !updated { - return errors.New("move not found for update") + // Search through entries with matching position hash + for i := idx; i < len(book.entries) && book.entries[i].Key == positionHash; i++ { + if book.entries[i].Move == target { + book.entries[i].Weight = newWeight + return nil + } } - return nil + return errors.New("move not found for update") }
461-470: Consider optimizing the deletion operation.Instead of linearly searching through all entries, consider using binary search to find the range of entries to delete.
Apply this diff to optimize the deletion operation:
func (book *PolyglotBook) DeleteMoves(positionHash uint64) { - var newEntries []PolyglotEntry - for _, entry := range book.entries { - if entry.Key != positionHash { - newEntries = append(newEntries, entry) - } + // Find the range of entries with matching position hash + start := sort.Search(len(book.entries), func(i int) bool { + return book.entries[i].Key >= positionHash + }) + if start >= len(book.entries) || book.entries[start].Key != positionHash { + return } - book.entries = newEntries + // Find the end of the range + end := start + for end < len(book.entries) && book.entries[end].Key == positionHash { + end++ + } + // Remove the range + copy(book.entries[start:], book.entries[end:]) + book.entries = book.entries[:len(book.entries)-(end-start)] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
polyglot.go(3 hunks)polyglot_test.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
polyglot.go
[warning] 89-94: polyglot.go#L89-L94
Added lines #L89 - L94 were not covered by tests
[warning] 104-105: polyglot.go#L104-L105
Added lines #L104 - L105 were not covered by tests
[warning] 109-109: polyglot.go#L109
Added line #L109 was not covered by tests
🔇 Additional comments (12)
polyglot.go (5)
58-62: LGTM!The helper struct is well-designed, simple, and focused on its purpose of coupling a chess move with its weight.
64-72: LGTM!The function correctly encodes chess moves into polyglot format using well-documented bit operations.
74-82: LGTM!The method correctly encodes polyglot moves using well-documented bit operations, maintaining consistency with
MoveToPolyglot.
84-103: LGTM!The method correctly converts polyglot moves to chess moves, handling file/rank indices, promotion pieces, and castling moves.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-94: polyglot.go#L89-L94
Added lines #L89 - L94 were not covered by tests
409-428: LGTM!The function correctly creates a PolyglotBook from a map, properly converting and sorting the entries.
polyglot_test.go (7)
283-347: LGTM!The test function is well-structured and covers important scenarios including regular moves, promotion moves, and castling moves.
349-400: LGTM!The test function thoroughly verifies the bit-level encoding of polyglot moves for various scenarios.
402-458: LGTM!The test function thoroughly verifies the conversion of polyglot moves to chess moves for various scenarios.
460-504: LGTM!The test function thoroughly verifies the creation and sorting of polyglot book entries from a map.
506-542: LGTM!The test function thoroughly verifies adding moves to a polyglot book and ensures proper sorting.
544-582: LGTM!The test function thoroughly verifies updating moves in a polyglot book, including error cases.
584-616: LGTM!The test function thoroughly verifies deleting moves from a polyglot book and ensures proper cleanup.
| func (book *PolyglotBook) ToMoveMap() map[uint64][]MoveWithWeight { | ||
| result := make(map[uint64][]MoveWithWeight) | ||
| for _, entry := range book.entries { | ||
| pm := DecodeMove(entry.Move) | ||
| move := pm.ToMove() | ||
| mw := MoveWithWeight{ | ||
| Move: move, | ||
| Weight: entry.Weight, | ||
| } | ||
| result[entry.Key] = append(result[entry.Key], mw) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling.
The method should handle potential errors from ToMove to ensure all moves are valid.
Apply this diff to improve error handling:
-func (book *PolyglotBook) ToMoveMap() map[uint64][]MoveWithWeight {
+func (book *PolyglotBook) ToMoveMap() (map[uint64][]MoveWithWeight, []error) {
result := make(map[uint64][]MoveWithWeight)
+ var errs []error
for _, entry := range book.entries {
pm := DecodeMove(entry.Move)
- move := pm.ToMove()
+ move, err := pm.ToMove()
+ if err != nil {
+ errs = append(errs, fmt.Errorf("failed to convert move: %w", err))
+ continue
+ }
mw := MoveWithWeight{
Move: move,
Weight: entry.Weight,
}
result[entry.Key] = append(result[entry.Key], mw)
}
- return result
+ return result, errs
}📝 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.
| func (book *PolyglotBook) ToMoveMap() map[uint64][]MoveWithWeight { | |
| result := make(map[uint64][]MoveWithWeight) | |
| for _, entry := range book.entries { | |
| pm := DecodeMove(entry.Move) | |
| move := pm.ToMove() | |
| mw := MoveWithWeight{ | |
| Move: move, | |
| Weight: entry.Weight, | |
| } | |
| result[entry.Key] = append(result[entry.Key], mw) | |
| } | |
| return result | |
| } | |
| func (book *PolyglotBook) ToMoveMap() (map[uint64][]MoveWithWeight, []error) { | |
| result := make(map[uint64][]MoveWithWeight) | |
| var errs []error | |
| for _, entry := range book.entries { | |
| pm := DecodeMove(entry.Move) | |
| move, err := pm.ToMove() | |
| if err != nil { | |
| errs = append(errs, fmt.Errorf("failed to convert move: %w", err)) | |
| continue | |
| } | |
| mw := MoveWithWeight{ | |
| Move: move, | |
| Weight: entry.Weight, | |
| } | |
| result[entry.Key] = append(result[entry.Key], mw) | |
| } | |
| return result, errs | |
| } |
| func (book *PolyglotBook) GetChessMoves(positionHash uint64) ([]Move, error) { | ||
| entries := book.FindMoves(positionHash) | ||
| if entries == nil { | ||
| return nil, errors.New("no moves found for the given position") | ||
| } | ||
| var moves []Move | ||
| for _, entry := range entries { | ||
| pm := DecodeMove(entry.Move) | ||
| move := pm.ToMove() | ||
| moves = append(moves, move) | ||
| } | ||
| return moves, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling.
The method should handle potential errors from ToMove to ensure all moves are valid.
Apply this diff to improve error handling:
-func (book *PolyglotBook) GetChessMoves(positionHash uint64) ([]Move, error) {
+func (book *PolyglotBook) GetChessMoves(positionHash uint64) ([]Move, []error) {
entries := book.FindMoves(positionHash)
if entries == nil {
- return nil, errors.New("no moves found for the given position")
+ return nil, []error{errors.New("no moves found for the given position")}
}
var moves []Move
+ var errs []error
for _, entry := range entries {
pm := DecodeMove(entry.Move)
- move := pm.ToMove()
- moves = append(moves, move)
+ move, err := pm.ToMove()
+ if err != nil {
+ errs = append(errs, fmt.Errorf("failed to convert move: %w", err))
+ continue
+ }
+ moves = append(moves, move)
}
- return moves, nil
+ return moves, errs
}📝 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.
| func (book *PolyglotBook) GetChessMoves(positionHash uint64) ([]Move, error) { | |
| entries := book.FindMoves(positionHash) | |
| if entries == nil { | |
| return nil, errors.New("no moves found for the given position") | |
| } | |
| var moves []Move | |
| for _, entry := range entries { | |
| pm := DecodeMove(entry.Move) | |
| move := pm.ToMove() | |
| moves = append(moves, move) | |
| } | |
| return moves, nil | |
| } | |
| func (book *PolyglotBook) GetChessMoves(positionHash uint64) ([]Move, []error) { | |
| entries := book.FindMoves(positionHash) | |
| if entries == nil { | |
| return nil, []error{errors.New("no moves found for the given position")} | |
| } | |
| var moves []Move | |
| var errs []error | |
| for _, entry := range entries { | |
| pm := DecodeMove(entry.Move) | |
| move, err := pm.ToMove() | |
| if err != nil { | |
| errs = append(errs, fmt.Errorf("failed to convert move: %w", err)) | |
| continue | |
| } | |
| moves = append(moves, move) | |
| } | |
| return moves, errs | |
| } |
| return Move{} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling.
Instead of silently returning an empty move on error, consider propagating the error to allow proper error handling by the caller.
Apply this diff to improve error handling:
-func (pm PolyglotMove) ToMove() Move {
+func (pm PolyglotMove) ToMove() (Move, error) {
fromSquare := fmt.Sprintf("%c%d", 'a'+pm.FromFile, pm.FromRank+1)
toSquare := fmt.Sprintf("%c%d", 'a'+pm.ToFile, pm.ToRank+1)
var promo string
switch pm.Promotion {
case 1:
promo = "n"
case 2:
promo = "b"
case 3:
promo = "r"
case 4:
promo = "q"
default:
promo = ""
}
moveStr := fromSquare + toSquare + promo
decode, err := UCINotation{}.Decode(nil, moveStr)
if err != nil {
- return Move{}
+ return Move{}, fmt.Errorf("failed to decode move: %w", err)
}
if pm.CastlingMove {
if pm.FromFile == 4 && pm.ToFile == 0 {
decode.addTag(QueenSideCastle)
} else {
decode.addTag(KingSideCastle)
}
}
- return *decode
+ return *decode, nil
}📝 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.
| return Move{} | |
| } | |
| func (pm PolyglotMove) ToMove() (Move, error) { | |
| fromSquare := fmt.Sprintf("%c%d", 'a'+pm.FromFile, pm.FromRank+1) | |
| toSquare := fmt.Sprintf("%c%d", 'a'+pm.ToFile, pm.ToRank+1) | |
| var promo string | |
| switch pm.Promotion { | |
| case 1: | |
| promo = "n" | |
| case 2: | |
| promo = "b" | |
| case 3: | |
| promo = "r" | |
| case 4: | |
| promo = "q" | |
| default: | |
| promo = "" | |
| } | |
| moveStr := fromSquare + toSquare + promo | |
| decode, err := UCINotation{}.Decode(nil, moveStr) | |
| if err != nil { | |
| return Move{}, fmt.Errorf("failed to decode move: %w", err) | |
| } | |
| if pm.CastlingMove { | |
| if pm.FromFile == 4 && pm.ToFile == 0 { | |
| decode.addTag(QueenSideCastle) | |
| } else { | |
| decode.addTag(KingSideCastle) | |
| } | |
| } | |
| return *decode, nil | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 104-105: polyglot.go#L104-L105
Added lines #L104 - L105 were not covered by tests
| func TestGetChessMoves(t *testing.T) { | ||
| pos := uint64(50) | ||
| move1 := Move{ | ||
| s1: D2, | ||
| s2: D4, | ||
| promo: NoPieceType, | ||
| } | ||
| move2 := Move{ | ||
| s1: G7, | ||
| s2: H8, | ||
| promo: Queen, // promotion move | ||
| } | ||
| book := &PolyglotBook{ | ||
| entries: []PolyglotEntry{ | ||
| {Key: pos, Move: MoveToPolyglot(move1), Weight: 120, Learn: 0}, | ||
| {Key: pos, Move: MoveToPolyglot(move2), Weight: 220, Learn: 0}, | ||
| }, | ||
| } | ||
| moves, err := book.GetChessMoves(pos) | ||
| if err != nil { | ||
| t.Fatalf("GetChessMoves returned error: %v", err) | ||
| } | ||
| if len(moves) != 2 { | ||
| t.Fatalf("Expected 2 moves, got %d", len(moves)) | ||
| } | ||
| // Since the conversion functions (ChessMoveToPolyglot, DecodeMove, PolyglotToChessMove) | ||
| // should produce a round-trip, the moves should equal the originals. | ||
| if !reflect.DeepEqual(moves[1], move1) { | ||
| t.Errorf("First move mismatch: expected %+v, got %+v", move1, moves[0]) | ||
| } | ||
| if !reflect.DeepEqual(moves[0], move2) { | ||
| t.Errorf("Second move mismatch: expected %+v, got %+v", move2, moves[1]) | ||
| } | ||
|
|
||
| // Test error case: request moves for a non-existent position. | ||
| _, err = book.GetChessMoves(999) | ||
| if err == nil { | ||
| t.Errorf("Expected error for non-existent position, got nil") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update test to handle improved error handling.
The test needs to be updated to match the improved error handling in GetChessMoves.
Apply this diff to update the test:
func TestGetChessMoves(t *testing.T) {
// ... test setup ...
- moves, err := book.GetChessMoves(pos)
- if err != nil {
- t.Fatalf("GetChessMoves returned error: %v", err)
+ moves, errs := book.GetChessMoves(pos)
+ if len(errs) > 0 {
+ t.Fatalf("GetChessMoves returned errors: %v", errs)
}
// ... rest of the test ...
// Test error case: request moves for a non-existent position.
- _, err = book.GetChessMoves(999)
- if err == nil {
- t.Errorf("Expected error for non-existent position, got nil")
+ _, errs = book.GetChessMoves(999)
+ if len(errs) == 0 {
+ t.Errorf("Expected errors for non-existent position, got none")
}
}📝 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.
| func TestGetChessMoves(t *testing.T) { | |
| pos := uint64(50) | |
| move1 := Move{ | |
| s1: D2, | |
| s2: D4, | |
| promo: NoPieceType, | |
| } | |
| move2 := Move{ | |
| s1: G7, | |
| s2: H8, | |
| promo: Queen, // promotion move | |
| } | |
| book := &PolyglotBook{ | |
| entries: []PolyglotEntry{ | |
| {Key: pos, Move: MoveToPolyglot(move1), Weight: 120, Learn: 0}, | |
| {Key: pos, Move: MoveToPolyglot(move2), Weight: 220, Learn: 0}, | |
| }, | |
| } | |
| moves, err := book.GetChessMoves(pos) | |
| if err != nil { | |
| t.Fatalf("GetChessMoves returned error: %v", err) | |
| } | |
| if len(moves) != 2 { | |
| t.Fatalf("Expected 2 moves, got %d", len(moves)) | |
| } | |
| // Since the conversion functions (ChessMoveToPolyglot, DecodeMove, PolyglotToChessMove) | |
| // should produce a round-trip, the moves should equal the originals. | |
| if !reflect.DeepEqual(moves[1], move1) { | |
| t.Errorf("First move mismatch: expected %+v, got %+v", move1, moves[0]) | |
| } | |
| if !reflect.DeepEqual(moves[0], move2) { | |
| t.Errorf("Second move mismatch: expected %+v, got %+v", move2, moves[1]) | |
| } | |
| // Test error case: request moves for a non-existent position. | |
| _, err = book.GetChessMoves(999) | |
| if err == nil { | |
| t.Errorf("Expected error for non-existent position, got nil") | |
| } | |
| } | |
| func TestGetChessMoves(t *testing.T) { | |
| pos := uint64(50) | |
| move1 := Move{ | |
| s1: D2, | |
| s2: D4, | |
| promo: NoPieceType, | |
| } | |
| move2 := Move{ | |
| s1: G7, | |
| s2: H8, | |
| promo: Queen, // promotion move | |
| } | |
| book := &PolyglotBook{ | |
| entries: []PolyglotEntry{ | |
| {Key: pos, Move: MoveToPolyglot(move1), Weight: 120, Learn: 0}, | |
| {Key: pos, Move: MoveToPolyglot(move2), Weight: 220, Learn: 0}, | |
| }, | |
| } | |
| moves, errs := book.GetChessMoves(pos) | |
| if len(errs) > 0 { | |
| t.Fatalf("GetChessMoves returned errors: %v", errs) | |
| } | |
| if len(moves) != 2 { | |
| t.Fatalf("Expected 2 moves, got %d", len(moves)) | |
| } | |
| // Since the conversion functions (ChessMoveToPolyglot, DecodeMove, PolyglotToChessMove) | |
| // should produce a round-trip, the moves should equal the originals. | |
| if !reflect.DeepEqual(moves[1], move1) { | |
| t.Errorf("First move mismatch: expected %+v, got %+v", move1, moves[0]) | |
| } | |
| if !reflect.DeepEqual(moves[0], move2) { | |
| t.Errorf("Second move mismatch: expected %+v, got %+v", move2, moves[1]) | |
| } | |
| // Test error case: request moves for a non-existent position. | |
| _, errs = book.GetChessMoves(999) | |
| if len(errs) == 0 { | |
| t.Errorf("Expected errors for non-existent position, got none") | |
| } | |
| } |
| func TestToMoveMap(t *testing.T) { | ||
| pos1 := uint64(10) | ||
| pos2 := uint64(20) | ||
| move1 := Move{ | ||
| s1: A2, | ||
| s2: A3, | ||
| promo: NoPieceType, | ||
| } | ||
| move2 := Move{ | ||
| s1: A3, | ||
| s2: A4, | ||
| promo: NoPieceType, | ||
| } | ||
| move3 := Move{ | ||
| s1: B2, | ||
| s2: B3, | ||
| promo: NoPieceType, | ||
| } | ||
| book := &PolyglotBook{ | ||
| entries: []PolyglotEntry{ | ||
| {Key: pos1, Move: MoveToPolyglot(move1), Weight: 50, Learn: 0}, | ||
| {Key: pos1, Move: MoveToPolyglot(move2), Weight: 75, Learn: 0}, | ||
| {Key: pos2, Move: MoveToPolyglot(move3), Weight: 100, Learn: 0}, | ||
| }, | ||
| } | ||
|
|
||
| moveMap := book.ToMoveMap() | ||
| if len(moveMap) != 2 { | ||
| t.Fatalf("Expected move map with 2 keys, got %d", len(moveMap)) | ||
| } | ||
| movesPos1, ok := moveMap[pos1] | ||
| if !ok { | ||
| t.Fatalf("Position %d not found in move map", pos1) | ||
| } | ||
| if len(movesPos1) != 2 { | ||
| t.Errorf("Expected 2 moves for position %d, got %d", pos1, len(movesPos1)) | ||
| } | ||
| movesPos2, ok := moveMap[pos2] | ||
| if !ok { | ||
| t.Fatalf("Position %d not found in move map", pos2) | ||
| } | ||
| if len(movesPos2) != 1 { | ||
| t.Errorf("Expected 1 move for position %d, got %d", pos2, len(movesPos2)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update test to handle improved error handling.
The test needs to be updated to match the improved error handling in ToMoveMap.
Apply this diff to update the test:
func TestToMoveMap(t *testing.T) {
// ... test setup ...
- moveMap := book.ToMoveMap()
+ moveMap, errs := book.ToMoveMap()
+ if len(errs) > 0 {
+ t.Fatalf("ToMoveMap returned errors: %v", errs)
+ }
// ... rest of the test ...
}📝 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.
| func TestToMoveMap(t *testing.T) { | |
| pos1 := uint64(10) | |
| pos2 := uint64(20) | |
| move1 := Move{ | |
| s1: A2, | |
| s2: A3, | |
| promo: NoPieceType, | |
| } | |
| move2 := Move{ | |
| s1: A3, | |
| s2: A4, | |
| promo: NoPieceType, | |
| } | |
| move3 := Move{ | |
| s1: B2, | |
| s2: B3, | |
| promo: NoPieceType, | |
| } | |
| book := &PolyglotBook{ | |
| entries: []PolyglotEntry{ | |
| {Key: pos1, Move: MoveToPolyglot(move1), Weight: 50, Learn: 0}, | |
| {Key: pos1, Move: MoveToPolyglot(move2), Weight: 75, Learn: 0}, | |
| {Key: pos2, Move: MoveToPolyglot(move3), Weight: 100, Learn: 0}, | |
| }, | |
| } | |
| moveMap := book.ToMoveMap() | |
| if len(moveMap) != 2 { | |
| t.Fatalf("Expected move map with 2 keys, got %d", len(moveMap)) | |
| } | |
| movesPos1, ok := moveMap[pos1] | |
| if !ok { | |
| t.Fatalf("Position %d not found in move map", pos1) | |
| } | |
| if len(movesPos1) != 2 { | |
| t.Errorf("Expected 2 moves for position %d, got %d", pos1, len(movesPos1)) | |
| } | |
| movesPos2, ok := moveMap[pos2] | |
| if !ok { | |
| t.Fatalf("Position %d not found in move map", pos2) | |
| } | |
| if len(movesPos2) != 1 { | |
| t.Errorf("Expected 1 move for position %d, got %d", pos2, len(movesPos2)) | |
| } | |
| } | |
| func TestToMoveMap(t *testing.T) { | |
| pos1 := uint64(10) | |
| pos2 := uint64(20) | |
| move1 := Move{ | |
| s1: A2, | |
| s2: A3, | |
| promo: NoPieceType, | |
| } | |
| move2 := Move{ | |
| s1: A3, | |
| s2: A4, | |
| promo: NoPieceType, | |
| } | |
| move3 := Move{ | |
| s1: B2, | |
| s2: B3, | |
| promo: NoPieceType, | |
| } | |
| book := &PolyglotBook{ | |
| entries: []PolyglotEntry{ | |
| {Key: pos1, Move: MoveToPolyglot(move1), Weight: 50, Learn: 0}, | |
| {Key: pos1, Move: MoveToPolyglot(move2), Weight: 75, Learn: 0}, | |
| {Key: pos2, Move: MoveToPolyglot(move3), Weight: 100, Learn: 0}, | |
| }, | |
| } | |
| moveMap, errs := book.ToMoveMap() | |
| if len(errs) > 0 { | |
| t.Fatalf("ToMoveMap returned errors: %v", errs) | |
| } | |
| if len(moveMap) != 2 { | |
| t.Fatalf("Expected move map with 2 keys, got %d", len(moveMap)) | |
| } | |
| movesPos1, ok := moveMap[pos1] | |
| if !ok { | |
| t.Fatalf("Position %d not found in move map", pos1) | |
| } | |
| if len(movesPos1) != 2 { | |
| t.Errorf("Expected 2 moves for position %d, got %d", pos1, len(movesPos1)) | |
| } | |
| movesPos2, ok := moveMap[pos2] | |
| if !ok { | |
| t.Fatalf("Position %d not found in move map", pos2) | |
| } | |
| if len(movesPos2) != 1 { | |
| t.Errorf("Expected 1 move for position %d, got %d", pos2, len(movesPos2)) | |
| } | |
| } |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zobrist_test.go(9 hunks)
🧰 Additional context used
🪛 GitHub Check: test
zobrist_test.go
[failure] 246-246:
assignment mismatch: 2 variables but ZobristHashToUint64 returns 1 value
[failure] 257-257:
assignment mismatch: 2 variables but ZobristHashToUint64 returns 1 value
[failure] 265-265:
assignment mismatch: 2 variables but ZobristHashToUint64 returns 1 value
🪛 GitHub Actions: CI
zobrist_test.go
[error] 246-246: Assignment mismatch: 2 variables but ZobristHashToUint64 returns 1 value.
🔇 Additional comments (2)
zobrist_test.go (2)
10-10: LGTM! Consistent renaming of hasher initialization.The changes from
NewChessHasher()toNewZobristHasher()are applied consistently across all test cases, maintaining the original test coverage and functionality.Also applies to: 41-41, 82-82, 110-110, 132-132, 150-150, 183-183, 209-209
243-253: LGTM! Well-structured test cases with good error handling.The test cases are well-organized and cover essential scenarios:
- Valid hash conversion
- Invalid hash format handling
- Empty hash handling
The error handling aligns with the recommended approach from previous reviews.
Also applies to: 255-261, 263-269
🧰 Tools
🪛 GitHub Check: test
[failure] 246-246:
assignment mismatch: 2 variables but ZobristHashToUint64 returns 1 value🪛 GitHub Actions: CI
[error] 246-246: Assignment mismatch: 2 variables but ZobristHashToUint64 returns 1 value.
zobrist_test.go
Outdated
| func TestZobristHashToUint64(t *testing.T) { | ||
| t.Run("valid 16-digit hexadecimal hash is converted correctly", func(t *testing.T) { | ||
| hash := "463b96181691fc9c" | ||
| expected := uint64(0x463b96181691fc9c) | ||
| result, err := ZobristHashToUint64(hash) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if result != expected { | ||
| t.Fatalf("expected value %v but got %v", expected, result) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("invalid hash format returns error", func(t *testing.T) { | ||
| hash := "invalidhash" | ||
| _, err := ZobristHashToUint64(hash) | ||
| if err == nil { | ||
| t.Fatal("expected error for invalid hash format") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("empty hash returns error", func(t *testing.T) { | ||
| hash := "" | ||
| _, err := ZobristHashToUint64(hash) | ||
| if err == nil { | ||
| t.Fatal("expected error for empty hash") | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fix assignment mismatch in test cases.
The test cases expect ZobristHashToUint64 to return (uint64, error), but the function appears to return only one value. This mismatch is causing pipeline failures.
Update the function signature in zobrist.go to match the test expectations:
-func ZobristHashToUint64(hash string) uint64 {
+func ZobristHashToUint64(hash string) (uint64, error) {📝 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.
| func TestZobristHashToUint64(t *testing.T) { | |
| t.Run("valid 16-digit hexadecimal hash is converted correctly", func(t *testing.T) { | |
| hash := "463b96181691fc9c" | |
| expected := uint64(0x463b96181691fc9c) | |
| result, err := ZobristHashToUint64(hash) | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if result != expected { | |
| t.Fatalf("expected value %v but got %v", expected, result) | |
| } | |
| }) | |
| t.Run("invalid hash format returns error", func(t *testing.T) { | |
| hash := "invalidhash" | |
| _, err := ZobristHashToUint64(hash) | |
| if err == nil { | |
| t.Fatal("expected error for invalid hash format") | |
| } | |
| }) | |
| t.Run("empty hash returns error", func(t *testing.T) { | |
| hash := "" | |
| _, err := ZobristHashToUint64(hash) | |
| if err == nil { | |
| t.Fatal("expected error for empty hash") | |
| } | |
| }) | |
| } | |
| package zobrist | |
| // ZobristHashToUint64 converts a hexadecimal Zobrist hash string to a uint64 value. | |
| // It returns an error if the hash format is invalid. | |
| func ZobristHashToUint64(hash string) (uint64, error) { | |
| // TODO: Update the implementation to handle error returns. | |
| // For example: | |
| // 1. Validate that the hash string conforms to a 16-digit hexadecimal format. | |
| // 2. Convert the string to uint64 and return any error encountered during conversion. | |
| // Example (dummy implementation): | |
| if len(hash) != 16 { | |
| return 0, fmt.Errorf("invalid hash length") | |
| } | |
| // Assuming proper conversion is done here | |
| var result uint64 | |
| // ... conversion logic ... | |
| return result, nil | |
| } |
🧰 Tools
🪛 GitHub Check: test
[failure] 246-246:
assignment mismatch: 2 variables but ZobristHashToUint64 returns 1 value
[failure] 257-257:
assignment mismatch: 2 variables but ZobristHashToUint64 returns 1 value
[failure] 265-265:
assignment mismatch: 2 variables but ZobristHashToUint64 returns 1 value
🪛 GitHub Actions: CI
[error] 246-246: Assignment mismatch: 2 variables but ZobristHashToUint64 returns 1 value.
Summary by CodeRabbit