SZUT/casino

Archived

Fork 0

test: dice service #237

Merged

jank merged 3 commits from refactor-blackjack into main

2025-06-02 12:16:43 +00:00

ptran commented

2025-05-28 10:51:07 +00:00

Member

No description provided.

Claude requested changes 2025-05-28 10:52:36 +00:00

Dismissed

Claude left a comment

Collaborator

-m This PR has several critical issues that need to be addressed:

CRITICAL: Multiple test cases have incorrect logic - Several tests expect wrong outcomes, particularly around edge cases with target values of 1 and 100.
Missing critical edge case testing - No tests for when winChance equals zero, which could cause division issues.
Poor test organization - Test method names don't clearly indicate what edge cases they're testing.
Unrelated changes mixed in - BlackJackGameEntity import cleanup belongs in a separate commit.
Insufficient validation - Tests don't verify Random bean injection works properly.

Fix the test logic, separate unrelated changes, and add proper edge case coverage before resubmitting. The dependency injection approach is sound, but the test implementation is fundamentally flawed.

-m This PR has several critical issues that need to be addressed: 1. **CRITICAL: Multiple test cases have incorrect logic** - Several tests expect wrong outcomes, particularly around edge cases with target values of 1 and 100. 2. **Missing critical edge case testing** - No tests for when winChance equals zero, which could cause division issues. 3. **Poor test organization** - Test method names don't clearly indicate what edge cases they're testing. 4. **Unrelated changes mixed in** - BlackJackGameEntity import cleanup belongs in a separate commit. 5. **Insufficient validation** - Tests don't verify Random bean injection works properly. Fix the test logic, separate unrelated changes, and add proper edge case coverage before resubmitting. The dependency injection approach is sound, but the test implementation is fundamentally flawed.

Claude commented

2025-05-28 11:00:40 +00:00

Collaborator

REJECTED - Application will fail to start due to missing Spring Bean configuration.

Critical Issues:

DiceService.java:17 - Constructor requires Random parameter but no @Bean configuration exists in Spring context. Add @Bean Random random() method to a @Configuration class.

Minor Issues:

DiceServiceTest.java:195 - Remove confusing comment about \typo in original\ when code is already correct
DiceServiceTest.java:218-219 - Hardcoded expected values (39.6, 41.25) make tests harder to understand. Consider showing calculation in comments.

Positive: Good test coverage and dependency injection pattern, but fix the Spring config before merge.

**REJECTED** - Application will fail to start due to missing Spring Bean configuration. **Critical Issues:** - **DiceService.java:17** - Constructor requires Random parameter but no @Bean configuration exists in Spring context. Add @Bean Random random() method to a @Configuration class. **Minor Issues:** - **DiceServiceTest.java:195** - Remove confusing comment about \typo in original\ when code is already correct - **DiceServiceTest.java:218-219** - Hardcoded expected values (39.6, 41.25) make tests harder to understand. Consider showing calculation in comments. **Positive:** Good test coverage and dependency injection pattern, but fix the Spring config before merge.

ptran force-pushed refactor-blackjack from 63f4ad9984 to 99506e98e0

2025-05-28 11:01:24 +00:00

Compare

Claude requested changes 2025-05-28 11:03:11 +00:00

Dismissed

Claude left a comment

Collaborator

This PR has several critical issues that need immediate attention:

Critical Issues:

CI/Workflow Changes (.gitea/workflows/ci.yml:32-58): You removed the entire backend test job from CI pipeline! This completely defeats the purpose of having automated testing. What's the point of writing tests if they don't run in CI? This is unacceptable.
Missing DiceDto Constructor (backend/src/main/java/de/szut/casino/dice/DiceDto.java:15): Adding @NoArgsConstructor without understanding why it's needed suggests poor understanding of the framework requirements. This change appears arbitrary.
Removed Import Statements (backend/src/main/java/de/szut/casino/blackjack/BlackJackGameEntity.java:7-8): You removed @NotNull and @Positive imports but left no explanation. If these validations were removed, where's the reasoning? This could introduce validation gaps.

Test Quality Issues:

DiceServiceTest.java line 129: Your comment calculation is wrong. For target value 1 with rollOver=true, win chance is 99, multiplier should be 99/99 = 1, but you calculated (100-1)/99 = 1. The math is coincidentally correct but the reasoning is flawed.
DiceServiceTest.java line 177: You're using hardcoded BigDecimal division with 4 decimal places precision, but then comparing exact values. This is fragile and could break with floating point precision issues.
DiceServiceTest.java line 225: Using stripTrailingZeros for comparison suggests you don't understand BigDecimal equality. Use compareTo instead.

Architectural Concerns:

DiceService.java line 17: Injecting Random as a dependency is overkill. The original approach was perfectly fine for this use case. You're over-engineering a simple gambling service.

Fix these issues, especially restore the CI backend tests, before resubmitting.

This PR has several critical issues that need immediate attention: **Critical Issues:** 1. **CI/Workflow Changes (.gitea/workflows/ci.yml:32-58)**: You removed the entire backend test job from CI pipeline\! This completely defeats the purpose of having automated testing. What's the point of writing tests if they don't run in CI? This is unacceptable. 2. **Missing DiceDto Constructor (backend/src/main/java/de/szut/casino/dice/DiceDto.java:15)**: Adding @NoArgsConstructor without understanding why it's needed suggests poor understanding of the framework requirements. This change appears arbitrary. 3. **Removed Import Statements (backend/src/main/java/de/szut/casino/blackjack/BlackJackGameEntity.java:7-8)**: You removed @NotNull and @Positive imports but left no explanation. If these validations were removed, where's the reasoning? This could introduce validation gaps. **Test Quality Issues:** 4. **DiceServiceTest.java line 129**: Your comment calculation is wrong. For target value 1 with rollOver=true, win chance is 99, multiplier should be 99/99 = 1, but you calculated (100-1)/99 = 1. The math is coincidentally correct but the reasoning is flawed. 5. **DiceServiceTest.java line 177**: You're using hardcoded BigDecimal division with 4 decimal places precision, but then comparing exact values. This is fragile and could break with floating point precision issues. 6. **DiceServiceTest.java line 225**: Using stripTrailingZeros for comparison suggests you don't understand BigDecimal equality. Use compareTo instead. **Architectural Concerns:** 7. **DiceService.java line 17**: Injecting Random as a dependency is overkill. The original approach was perfectly fine for this use case. You're over-engineering a simple gambling service. Fix these issues, especially restore the CI backend tests, before resubmitting.

ptran force-pushed refactor-blackjack from 99506e98e0 to 3f37f3fda8

2025-05-28 11:04:19 +00:00

Compare

ptran force-pushed refactor-blackjack from 3f37f3fda8 to 3e1c15e023

2025-05-28 11:05:00 +00:00

Compare

Claude requested changes 2025-05-28 11:06:57 +00:00

Dismissed

Claude left a comment

Collaborator

Critical Issues: DiceService.java line 17-20 unnecessarily complicates dependency injection with Random bean when new Random() was fine. BlackJackGameEntity.java lines 4-5 removes NotNull and Positive validation constraints without justification, weakening input validation. Minor: DiceDto.java line 15 adds redundant NoArgsConstructor annotation. Test quality issues include hardcoded calculations making tests brittle and overly verbose method names.

Claude commented

2025-05-28 11:07:36 +00:00

Collaborator

Code Review for PR #237: Test Dice Service

POSITIVE ASPECTS:

✅ Comprehensive test coverage with 15 well-structured test cases in DiceServiceTest.java
✅ Good refactor in DiceService.java:17-20 to inject Random as dependency for testability
✅ Tests follow best practices with @BeforeEach setup and descriptive names
✅ Edge cases properly tested (target values 1, 99, 100)
✅ NoArgsConstructor addition in DiceDto.java:15 improves framework compatibility

MINOR OBSERVATIONS:

⚠️ BlackJackGameEntity.java:7-8 - Removed validation imports, ensure validation not compromised elsewhere
⚠️ DiceServiceTest.java:175-177 - Test precision handling should match service implementation

OVERALL:

Solid PR adding comprehensive test coverage. Dependency injection refactor improves testability. No blocking issues found.

APPROVE ✅

**Code Review for PR #237: Test Dice Service** ## POSITIVE ASPECTS: ✅ Comprehensive test coverage with 15 well-structured test cases in DiceServiceTest.java ✅ Good refactor in DiceService.java:17-20 to inject Random as dependency for testability ✅ Tests follow best practices with @BeforeEach setup and descriptive names ✅ Edge cases properly tested (target values 1, 99, 100) ✅ NoArgsConstructor addition in DiceDto.java:15 improves framework compatibility ## MINOR OBSERVATIONS: ⚠️ BlackJackGameEntity.java:7-8 - Removed validation imports, ensure validation not compromised elsewhere ⚠️ DiceServiceTest.java:175-177 - Test precision handling should match service implementation ## OVERALL: Solid PR adding comprehensive test coverage. Dependency injection refactor improves testability. No blocking issues found. **APPROVE** ✅