Conversation
Birdmannn
left a comment
Birdmannn
left a comment
There was a problem hiding this comment.
That said, for each should_panic function, please remove the ENTRYPOINT_FAILED from the expected panic message's tuple. Graçias. @guha-rahul
Know that this review is not abnormal... though I have no idea how your previous PRs were reviewed. I usually try to reduce review overhead, so please pay attention to the details in this one. If there's anything that you couldn't understand after you have googled it, then you can ask. I just hope I'd see more good on the next review, and corrections very close to none
Congrats to us 🌾
|
|
||
| // Simulate small blind deduction (Player 1 is next to dealer) | ||
| let small_blind = game.params.small_blind; | ||
| player_1.chips -= small_blind.into(); |
There was a problem hiding this comment.
why is it you doing the deduction? please we are testing the contract itself, so what you should do is call the contract's abi. Please don't just mock the storage because dojo allows you to do so. You're free to mock the initial player and game states when the game begins (actually I'd prefer you even call the abi for this... the initialize_game, join_game, etc)
| let small_blind = initial_game.params.small_blind; | ||
|
|
||
| // Simulate round start (this would normally be called by _start_round) | ||
| simulate_round_start(ref world); |
There was a problem hiding this comment.
You can observe what I said earlier right? You are "testing" the automatic deduction, yet you're the one doing the deducting. It's not sounding right sir 👀
| let contracts = array![CoreContract::Actions]; | ||
| let (mut world, systems) = deploy_contracts(contracts); | ||
| setup_four_player_betting_game(ref world); | ||
| simulate_round_start(ref world); |
There was a problem hiding this comment.
this too. I suggest just calling the start_round. It's a complex task, don't worry... that's why its difficulty is hard. You're free to comment out the verification of signature in the contracts function just for the sake of this test. But you must call the start_round, and any issues found, it's only natural that you fix them... 🌚
|
|
||
| // Try to make small blind player (Player 1) play when it's Player 2's turn | ||
| set_contract_address(PLAYER_1()); | ||
| systems.actions.check(); |
|
|
||
| // Player 2 (big blind player) tries to raise with invalid amount | ||
| set_contract_address(PLAYER_2()); | ||
| systems.actions.raise(invalid_raise); |
| assert!(!game_state.showdown, "Game should not be in showdown when bets are unmatched"); | ||
| } | ||
|
|
||
| /// Test 9: Next player should skip all-in player |
There was a problem hiding this comment.
You must note that you haven't simulated any bit of the gameplay stated in the second section of the issue description sir @guha-rahul
I usually try my best to give you how and what is happening, and steps to follow in a long issue description, and yet some of you don't understand. If only you know how long it takes me to cook a single issue 😪
| // Manually simulate after_play logic to find next active player | ||
| // In a real game, this would happen after Player 2 makes a move | ||
|
|
There was a problem hiding this comment.
??
I need the code, not the comments. You should implement what you commented.
| assert!( | ||
| expected_next_player != Option::Some(PLAYER_3()), | ||
| "Next player should not be the all-in player", | ||
| ); |
There was a problem hiding this comment.
Obviously... because why not?
this test basically does nothing, by the way... I don't know if you know that sir 👀
| fn test_all_in_player_cannot_play_but_remains_in_round() { | ||
| // Setup | ||
| let contracts = array![CoreContract::Actions]; | ||
| let (mut world, systems) = deploy_contracts(contracts); | ||
| setup_four_player_betting_game(ref world); | ||
|
|
||
| // Set Player 3 as all-in | ||
| let mut player_3: Player = world.read_model(PLAYER_3()); | ||
| player_3.chips = 0; // All-in | ||
| player_3.in_round = true; // Still in round | ||
| world.write_model(@player_3); | ||
|
|
||
| let mut game: Game = world.read_model(1); | ||
| game.next_player = Option::Some(PLAYER_3()); // Force Player 3 to be next | ||
| world.write_model(@game); | ||
|
|
||
| // Try to make Player 3 play - should panic due to no chips | ||
| set_contract_address(PLAYER_3()); | ||
| systems.actions.check(); |
There was a problem hiding this comment.
Invalid ❌😪.
First off, it shouldn't panic... that's what we are trying to make sure.
Second, as usual, the gameplay wasn't simulated. You just changed one or two variables and you thought we were good to go.
| #[test] | ||
| fn test_all_in_player_state_verification() { | ||
| // Setup | ||
| let contracts = array![CoreContract::Actions]; | ||
| let (mut world, _systems) = deploy_contracts(contracts); | ||
| setup_four_player_betting_game(ref world); | ||
|
|
||
| // Manually set player to all-in state | ||
| let mut player_3: Player = world.read_model(PLAYER_3()); | ||
| player_3.chips = 0; | ||
| player_3.in_round = true; | ||
| world.write_model(@player_3); | ||
|
|
||
| // Verify Player 3's state after all-in | ||
| let player_3_after: Player = world.read_model(PLAYER_3()); | ||
|
|
||
| assert!(player_3_after.chips == 0, "Player should have 0 chips after all-in"); | ||
| assert!(player_3_after.in_round, "Player should still be in round after all-in"); | ||
| assert!(player_3_after.is_in_game(1), "Player should still be in game after all-in"); | ||
| } |
There was a problem hiding this comment.
This test tests absolutely nothing... in my opinion sir. I don't know of yours.
Anyway, it just basically means you first have to understand what a game simulation is. You can check out some tests in Azura's repo, and see how things were cooked there. It resembles how you test your normal regular smart contract. Ohayo. ⛅️
2 participants
Description
This work addresses the need for comprehensive testing of the betting flow, particularly:
Related Issues
fixes #187
Changes Made - [ ]
How to Test
Screenshots (if applicable)
Checklist