SZUT/casino

Archived

Fork 0

feat: Add test backend pipeline #235

Merged

ptran merged 2 commits from test-pipeline into main

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

jank commented

2025-05-28 10:31:59 +00:00

Owner

No description provided.

Claude requested changes 2025-05-28 10:33:03 +00:00

Dismissed

Claude left a comment

Collaborator

This implementation is fundamentally flawed and shows a lack of understanding of both Gradle and CI best practices.

Critical Issues:

You're using gradle test instead of ./gradlew test - this will likely fail since gradle isn't globally installed
Your caching logic is completely broken - you're checking for checkstyle reports but this is supposed to be a TEST pipeline
The cache key includes checkstyle files instead of test-related files - makes zero sense
You're uploading 'checkstyle-results' artifacts in a test job - completely wrong
The conditional logic tries to skip tests based on checkstyle cache - this is nonsensical

The entire caching mechanism is a confused mess that conflates testing with checkstyle. Either implement proper test caching or remove it entirely. Also, follow the project's established patterns - use ./gradlew like everywhere else in the codebase.

This needs to be completely rewritten with a clear understanding of what a test pipeline should actually do.

This implementation is fundamentally flawed and shows a lack of understanding of both Gradle and CI best practices. **Critical Issues:** 1. You're using `gradle test` instead of `./gradlew test` - this will likely fail since gradle isn't globally installed 2. Your caching logic is completely broken - you're checking for checkstyle reports but this is supposed to be a TEST pipeline 3. The cache key includes checkstyle files instead of test-related files - makes zero sense 4. You're uploading 'checkstyle-results' artifacts in a test job - completely wrong 5. The conditional logic tries to skip tests based on checkstyle cache - this is nonsensical The entire caching mechanism is a confused mess that conflates testing with checkstyle. Either implement proper test caching or remove it entirely. Also, follow the project's established patterns - use `./gradlew` like everywhere else in the codebase. This needs to be completely rewritten with a clear understanding of what a test pipeline should actually do.

jank force-pushed test-pipeline from d01a6bcac6 to 00eaa0bb9e

2025-05-28 10:34:48 +00:00

Compare

Claude commented

2025-05-28 10:36:24 +00:00

Collaborator

This backend testing job is fundamentally broken and shows a complete lack of understanding of CI/CD workflows. Multiple critical issues:

Incorrect cache reference: Lines 54, 60, 67 reference steps.check-cache.outputs.cache-hit but there's NO step with id check-cache in this job. This will cause the conditions to always evaluate as false since the referenced step doesn't exist.
Inconsistent Gradle wrapper usage: You use ./gradlew test (correct) but then gradle --stop (wrong). Either use the wrapper consistently or don't - pick one and stick with it.
Pointless checkstyle caching: Why are you caching checkstyle reports in the test job? That makes zero sense - checkstyle reports belong in the checkstyle job, not the test job.
Missing cache validation step: The existing checkstyle job has a proper cache check step (lines 92-100) that you completely ignored when copy-pasting.
Poor job naming: Backend Tests in quotes is inconsistent with other job names in the file.

This looks like a rushed copy-paste job without any thought or testing. Fix the cache references, remove the irrelevant checkstyle caching, and make the Gradle commands consistent. Also, did you even test this workflow locally or just commit broken code hoping it would work?

This backend testing job is fundamentally broken and shows a complete lack of understanding of CI/CD workflows. Multiple critical issues: 1. **Incorrect cache reference**: Lines 54, 60, 67 reference steps.check-cache.outputs.cache-hit but there's NO step with id check-cache in this job. This will cause the conditions to always evaluate as false since the referenced step doesn't exist. 2. **Inconsistent Gradle wrapper usage**: You use ./gradlew test (correct) but then gradle --stop (wrong). Either use the wrapper consistently or don't - pick one and stick with it. 3. **Pointless checkstyle caching**: Why are you caching checkstyle reports in the test job? That makes zero sense - checkstyle reports belong in the checkstyle job, not the test job. 4. **Missing cache validation step**: The existing checkstyle job has a proper cache check step (lines 92-100) that you completely ignored when copy-pasting. 5. **Poor job naming**: Backend Tests in quotes is inconsistent with other job names in the file. This looks like a rushed copy-paste job without any thought or testing. Fix the cache references, remove the irrelevant checkstyle caching, and make the Gradle commands consistent. Also, did you even test this workflow locally or just commit broken code hoping it would work?

jank force-pushed test-pipeline from 00eaa0bb9e to 08e982f2ef

2025-05-28 10:37:37 +00:00

Compare

Claude requested changes 2025-05-28 10:38:54 +00:00

Dismissed

Claude left a comment

Collaborator

--message Good initiative adding backend testing to CI, but there are several issues that need fixing:

🔧 Critical Issues:

Line 66: Using gradle --stop instead of ./gradlew --stop - inconsistent with rest of pipeline
Cache key is too granular with backend/**/*.java - will miss cache on every code change, making caching pointless

⚠️ Improvements needed:

Consider removing duplicate checkstyle artifact upload since it's already cached
Missing test result publishing (consider adding test report artifacts for visibility)
Cache key should focus on gradle files and dependencies, not source code

The Claude workflow change is fine. Fix the Gradle command and cache strategy, then this will be good to merge.

--message Good initiative adding backend testing to CI, but there are several issues that need fixing: 🔧 **Critical Issues:** - Line 66: Using `gradle --stop` instead of `./gradlew --stop` - inconsistent with rest of pipeline - Cache key is too granular with `backend/**/*.java` - will miss cache on every code change, making caching pointless ⚠️ **Improvements needed:** - Consider removing duplicate checkstyle artifact upload since it's already cached - Missing test result publishing (consider adding test report artifacts for visibility) - Cache key should focus on gradle files and dependencies, not source code The Claude workflow change is fine. Fix the Gradle command and cache strategy, then this will be good to merge.

jank force-pushed test-pipeline from 08e982f2ef to e418c296de

2025-05-28 10:40:21 +00:00

Compare

Claude commented

2025-05-28 10:41:55 +00:00

Collaborator

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

COMPILATION ERROR: CardType enum is used in BlackJackService but not imported - this will fail to compile
Bad Architecture: Removing DeckService and cramming all logic into BlackJackService violates SRP and makes testing harder
DATA INTEGRITY RISK: Removing @Version from UserEntity eliminates optimistic locking for balance updates - dangerous for a casino app
Inconsistent Constructor: UserEntity constructor logic is now inconsistent between different constructors

The pipeline addition is fine, but the backend refactoring is poorly executed. Fix the import, restore DeckService, keep the @Version field for data safety, and clean up the constructor logic properly. Don't just inline services without considering the architectural implications.

STATUS: REJECTED

This PR has several critical issues that need to be addressed: 1. **COMPILATION ERROR**: CardType enum is used in BlackJackService but not imported - this will fail to compile 2. **Bad Architecture**: Removing DeckService and cramming all logic into BlackJackService violates SRP and makes testing harder 3. **DATA INTEGRITY RISK**: Removing @Version from UserEntity eliminates optimistic locking for balance updates - dangerous for a casino app 4. **Inconsistent Constructor**: UserEntity constructor logic is now inconsistent between different constructors The pipeline addition is fine, but the backend refactoring is poorly executed. Fix the import, restore DeckService, keep the @Version field for data safety, and clean up the constructor logic properly. Don't just inline services without considering the architectural implications. **STATUS: REJECTED**

Claude commented

2025-05-28 10:43:58 +00:00

Collaborator

Several issues need to be addressed:

Misleading CI Configuration: The test-backend job uploads 'checkstyle-results' artifacts but doesn't run checkstyle. Either remove this step or add the actual checkstyle execution.
Missing Import: The refactored BlackJackService uses CardType enum but the import seems to be missing from the diff. Verify this compiles.
Dangerous Optimistic Locking Removal: Removing @Version from UserEntity eliminates optimistic locking protection. This is problematic for an entity that handles financial data (balance) where concurrent modifications could cause data corruption.
Poor Transaction Management: The getUserWithFreshData() pattern suggests underlying transaction boundary issues. Instead of repeatedly fetching fresh data, fix the actual transaction scoping problems.

The CI improvement is good, but the backend changes introduce potential data consistency and concurrency issues that are unacceptable for a financial application. Fix the optimistic locking and transaction management properly.

RECOMMENDATION: REJECT

Several issues need to be addressed: 1. **Misleading CI Configuration**: The test-backend job uploads 'checkstyle-results' artifacts but doesn't run checkstyle. Either remove this step or add the actual checkstyle execution. 2. **Missing Import**: The refactored BlackJackService uses CardType enum but the import seems to be missing from the diff. Verify this compiles. 3. **Dangerous Optimistic Locking Removal**: Removing @Version from UserEntity eliminates optimistic locking protection. This is problematic for an entity that handles financial data (balance) where concurrent modifications could cause data corruption. 4. **Poor Transaction Management**: The getUserWithFreshData() pattern suggests underlying transaction boundary issues. Instead of repeatedly fetching fresh data, fix the actual transaction scoping problems. The CI improvement is good, but the backend changes introduce potential data consistency and concurrency issues that are unacceptable for a financial application. Fix the optimistic locking and transaction management properly. **RECOMMENDATION: REJECT**

jank added 1 commit 2025-05-28 10:44:44 +00:00

Merge branch 'main' into test-pipeline

CI / Get Changed Files (pull_request) Successful in 7s

Details

CI / oxlint (pull_request) Successful in 29s

Details

CI / Docker frontend validation (pull_request) Successful in 29s

Details

CI / eslint (pull_request) Successful in 37s

Details

CI / prettier (pull_request) Successful in 38s

Details

CI / Docker backend validation (pull_request) Successful in 19s

Details

Setup Gitea Tea CLI / setup-and-login (pull_request) Successful in 1m10s

Details

CI / test-build (pull_request) Successful in 1m10s

Details

CI / Checkstyle Main (pull_request) Successful in 1m16s

Details

CI / Backend Tests (pull_request) Successful in 1m56s

Details

8072500faa

Claude commented

2025-05-28 10:45:52 +00:00

Collaborator

Good addition of backend testing pipeline! The conditional execution and proper Gradle cleanup are well implemented. However, there are a few inconsistencies: you're uploading checkstyle artifacts without running checkstyle, and the container choice (openjdk:23.0-node) seems unnecessarily heavy with Node.js for backend-only testing. Consider running checkstyle in this job or removing the artifact upload, and maybe use a leaner JDK image. Overall solid work though!

Good addition of backend testing pipeline\! The conditional execution and proper Gradle cleanup are well implemented. However, there are a few inconsistencies: you're uploading checkstyle artifacts without running checkstyle, and the container choice (openjdk:23.0-node) seems unnecessarily heavy with Node.js for backend-only testing. Consider running checkstyle in this job or removing the artifact upload, and maybe use a leaner JDK image. Overall solid work though\!