test: guard the pokersolver behaviours bestFive slices on - #102
Open
pip-robot[bot] wants to merge 1 commit into
Open
test: guard the pokersolver behaviours bestFive slices on#102pip-robot[bot] wants to merge 1 commit into
pip-robot[bot] wants to merge 1 commit into
Conversation
bestFive() does slice(0, 5) and that line is only correct because the solver returns the cards that make the hand first, in its own order. That was asserted in a doc comment. Three of the shapes were already covered; two were not. - A full house whose trips rank below its pair (3s 3h 3d As Ah). This is the one case where "first five" and "five highest" disagree, so it is the case that pins the slice. - The royal-flush name/descr split. handPhrase() reads descr because a royal is *named* "Straight Flush"; if descr ever changed, the recap and every drill grade would call a royal a straight flush and no name assertion would fail. Also covers every category name handPhrase maps, so a rename shows up as a failing test rather than a dropped clause. - One direct assertion on the raw solved.cards shape (six-card flush, two trips, six-card straight flush returning five, the low ace as '1'), because anything reading solved.cards inherits those traps and bestFive hides them. Two fixes to the same trap while here: pokersolver.d.ts said cards was always five, which is what leads someone to read it raw; and bestFiveOf in learnExamples.test.ts did read it raw, through an unknown cast, and was safe only because no fixture is a six-card flush. It now goes through bestFive. Verified against the installed pokersolver@2.1.4, which is pinned at ^2.1.4 and last published July 2020. Refs playpip/technology#78.
Deploying pip-web with
|
| Latest commit: |
c8aaa98
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://83aaf76e.pip-web-9oj.pages.dev |
| Branch Preview URL: | https://guard-pokersolver-contract.pip-web-9oj.pages.dev |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
bestFive()insrc/lib/poker/handEval.tsdoesslice(0, 5). That line is only correct because pokersolver returns the cards that make the hand first, in its own order, and that was asserted in a doc comment rather than a test.The failure shape if it ever stopped being true: every showdown reveal shows the wrong five cards, the recap explains hands wrongly, and the suite stays green, because every hand name is still right.
Three of the shapes were already covered in
tests/handEval.test.ts(six-card flush, two trips, the low-ace'1'rewrite). Two were not:3s 3h 3d As Ah). The one case where "first five" and "five highest" disagree, so it is the case that actually pins the slice. Sorted by rank the reveal would show two pair.name/descrsplit.handPhrase()readsdescrbecause a royal is named"Straight Flush". Ifdescrever stopped being exactly"Royal Flush", the recap and every drill grade would call a royal a straight flush, and no name assertion anywhere would fail. There was no test onhandPhraseat all, so this adds one covering every category name it maps.Plus one direct assertion on the raw
solved.cardsshape (six-card flush, two trips, six-card straight flush returning five rather than overflowing, the low ace as'1').bestFivehides all of that from its own callers, so anything new that readssolved.cardsinherits the traps with nothing to warn it.Two fixes to that same trap while in here:
src/types/pokersolver.d.tssaidcardswas "the five cards the solver used". It is not always five, and that sentence is what leads someone to read it raw.bestFiveOfintests/learnExamples.test.tsdid read it raw, through anas unknown ascast, and was safe only because no fixture is a six-card flush. It goes throughbestFivenow, which also drops the cast.What I checked: every behaviour was run against the installed
pokersolver@2.1.4before being written down, not recalled.pnpm test:allgreen (554 tests). No production code changed, so there is nothing UI-shaped here to verify, and I have no browser on this runner either way.Credit: the read that turned this up is the CMO's, on playpip/technology#78. Her count was five unguarded behaviours; three of them already had tests, and this adds the two that did not.