Additional castling tests & test name cleanup #531
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously pawn tests seemed "duplicated", as the test output was something like: chess.PawnTest: + move towards rank by 1 square 0.143s + not move to positions that are occupied by the same color 0.0s + capture in diagonal 0.001s + require a capture to move in diagonal 0.0s + move towards rank by 2 squares 0.002s + capture en passant 0.004s + move towards rank by 1 square-1 0.001s + not move to positions that are occupied by the same color-1 0.0s + capture in diagonal-1 0.001s + require a capture to move in diagonal-1 0.0s + move towards rank by 2 squares-1 0.002s + capture en passant-1 0.001s By a quick glance it seemed the test were identical duplicates. However, the first ones were white pawn variants and latter ones were black pawn variants. Rename the tests to include note on which one they're testing. The new test output is something like: chess.PawnTest: + move towards rank by 1 square (white pawn) 0.163s + not move to positions that are occupied by the same color (white pawn) 0.001s + capture in diagonal (white pawn) 0.002s + require a capture to move in diagonal (white pawn) 0.0s + move towards rank by 2 squares (white pawn) 0.002s + capture en passant (white pawn) 0.004s + move towards rank by 1 square (black pawn) 0.001s + not move to positions that are occupied by the same color (black pawn) 0.0s + capture in diagonal (black pawn) 0.001s + require a capture to move in diagonal (black pawn) 0.0s + move towards rank by 2 squares (black pawn) 0.003s + capture en passant (black pawn) 0.004s
In test output, it seemed there were two instances of the same test: + threat on rook does not prevent castling king side 0.003s + threat on rook does not prevent castling king side-1 0.001s In fact the latter one was queens side test with copypaste name
|
Thank you for the contribution! These changes are indeed improve these tests situations. I'm actually thinking about removing these legacy tests. Because We have better tests coverage with Bitboard and perft tests. But in the other hands, why not just keep them so 🤷 . |
|
Interesting to know, @lenguyenthanh. If there is division between the new and old styles of tests, perhaps is it mentioned somewhere? From observing the tests alone it is not clear if some part is becoming legacy or not. |
|
Hey, sorry for the late reply! Unfortunately, that there is no clear division between new and legacy tests (and there is no documents :( sorry for that). It requires a bit of involving (historically) with this library and this is also just my personal opinion. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In test output, there were some confusing "duplicated" test cases being run. Upon closer inspection they were not duplicates, but their names were somewhat misleading.
While checking these, I also added additional castling tests. Nothing of note was revealed through them, but adds a bit of coverage for different scenarios.