chore(ci): cover ee-less backend build and frontend unit tests#3824
Open
Anty0 wants to merge 7 commits into
Open
chore(ci): cover ee-less backend build and frontend unit tests#3824Anty0 wants to merge 7 commits into
Anty0 wants to merge 7 commits into
Conversation
Add two CI jobs to close gaps in without-ee coverage: - backend-build-without-ee: removes ./ee and runs a full compile+package (classes jar bootJar) without SKIP_SERVER_BUILD, so the main backend is actually built without the enterprise modules. The existing runWithoutEeTests reuses the with-ee jar and never verifies compilation. - frontend-test-without-ee: removes webapp/src/ee before npm ci (so the prepare hook points the tg.ee symlink at the OSS stub) and runs the webapp unit tests. Unit tests previously always ran with ee present. Both jobs are wired into the everything-passed gate.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe workflow adds backend and frontend verification without EE code and includes both jobs in final status aggregation. The backend build conditionally includes EE Kotlin sources, and a new internal controller prepares branch-merge E2E data and creates an initial snapshot. ChangesEnterprise-excluded CI verification
Branch merge E2E data setup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant E2EDataClient
participant BranchMergeE2eDataController
participant BranchSnapshotService
E2EDataClient->>BranchMergeE2eDataController: submit branch-merge test data
BranchMergeE2eDataController->>BranchSnapshotService: createInitialSnapshot(project.id, mainBranch, featureBranch)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 32-36: Restrict GitHub token access for both build jobs in the
workflow by adding job-level permissions with contents read only, and update
each actions/checkout step to set persist-credentials to false. Grant no
additional scopes unless an existing step explicitly requires them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5058ee03-da04-468d-8957-98de74817532
📒 Files selected for processing (1)
.github/workflows/test.yml
BranchMergeE2eDataController lives in the :development module, which is unconditionally on server-app's classpath, but it imported the ee-only BranchSnapshotService directly. :development depends on :ee-app only when the ee directory exists, so the OSS build (no ee) failed to compile it. Move the controller into a new ee-only source root (src/mainEe/kotlin) that is added to the :development source set only when eeAppDirectoryExists, mirroring the existing conditional :ee-app dependency. Package is unchanged, so component scanning still registers it when ee is present.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/development/src/mainEe/kotlin/io/tolgee/controllers/internal/e2eData/BranchMergeE2eDataController.kt`:
- Around line 15-24: Make currentTestData in BranchMergeE2eDataController
request-local so concurrent requests cannot overwrite the test data used by
afterTestDataStored. Prefer request scope for the controller or propagate the
BranchMergeTestData instance through the callback, ensuring afterTestDataStored
reads the matching request’s data rather than singleton-shared state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41becb28-9e4d-4178-b2d1-4c21d00683d2
📒 Files selected for processing (2)
backend/development/build.gradlebackend/development/src/mainEe/kotlin/io/tolgee/controllers/internal/e2eData/BranchMergeE2eDataController.kt
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/development/src/mainEe/kotlin/io/tolgee/controllers/internal/e2eData/BranchMergeE2eDataController.kt`:
- Around line 15-24: Make currentTestData in BranchMergeE2eDataController
request-local so concurrent requests cannot overwrite the test data used by
afterTestDataStored. Prefer request scope for the controller or propagate the
BranchMergeTestData instance through the callback, ensuring afterTestDataStored
reads the matching request’s data rather than singleton-shared state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41becb28-9e4d-4178-b2d1-4c21d00683d2
📒 Files selected for processing (2)
backend/development/build.gradlebackend/development/src/mainEe/kotlin/io/tolgee/controllers/internal/e2eData/BranchMergeE2eDataController.kt
🛑 Comments failed to post (1)
backend/development/src/mainEe/kotlin/io/tolgee/controllers/internal/e2eData/BranchMergeE2eDataController.kt (1)
15-24: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -A10 -B4 'annotation class InternalController|`@RequestScope`|`@Scope`' backend --glob '*.kt'Repository: tolgee/tolgee-platform
Length of output: 40910
🏁 Script executed:
#!/bin/bash set -euo pipefail files=( backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/BranchMergeE2eDataController.kt backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/AbstractE2eDataController.kt backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/PermissionsE2eDataController.kt ) for f in "${files[@]}"; do echo "===== $f =====" wc -l "$f" echo "--- outline ---" ast-grep outline "$f" --view expanded || true echo done echo "===== targeted excerpts =====" sed -n '1,220p' backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/BranchMergeE2eDataController.kt echo "-----" sed -n '1,240p' backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/AbstractE2eDataController.kt echo "-----" sed -n '1,120p' backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/PermissionsE2eDataController.ktRepository: tolgee/tolgee-platform
Length of output: 417
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== locate files ==" fd -a 'BranchMergeE2eDataController.kt|AbstractE2eDataController.kt|PermissionsE2eDataController.kt|InternalController.kt' backend echo "== inspect outlines ==" for f in $(fd -a 'BranchMergeE2eDataController.kt|AbstractE2eDataController.kt|PermissionsE2eDataController.kt|InternalController.kt' backend); do echo "===== $f =====" wc -l "$f" ast-grep outline "$f" --view expanded || true echo done echo "== targeted excerpts ==" for f in $(fd -a 'BranchMergeE2eDataController.kt|AbstractE2eDataController.kt|PermissionsE2eDataController.kt|InternalController.kt' backend); do echo "----- $f -----" sed -n '1,240p' "$f" echo doneRepository: tolgee/tolgee-platform
Length of output: 11910
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' backend/data/src/main/kotlin/io/tolgee/data/service/TestDataGeneratingService.kt echo "-----" sed -n '1,220p' backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/BranchMergeE2eDataController.kt echo "-----" rg -n "afterTestDataStored\\(|createInitialSnapshot\\(|currentTestData" backend/development/src/main/kotlin backend/data/src/main/kotlin --glob '*.kt'Repository: tolgee/tolgee-platform
Length of output: 284
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' backend/data/src/main/kotlin/io/tolgee/data/service/TestDataGeneratingService.kt echo "-----" sed -n '1,220p' backend/development/src/mainEe/kotlin/io/tolgee/controllers/internal/e2eData/BranchMergeE2eDataController.kt echo "-----" rg -n "afterTestDataStored\\(|createInitialSnapshot\\(|currentTestData" backend/development/src/main backend/data/src/main --glob '*.kt'Repository: tolgee/tolgee-platform
Length of output: 284
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== locate generation service ==" fd -a 'TestDataGeneratingService.kt|BranchMergeE2eDataController.kt|AbstractE2eDataController.kt' backend echo "== read generation service ==" for f in $(fd -a 'TestDataGeneratingService.kt|BranchMergeE2eDataController.kt|AbstractE2eDataController.kt' backend); do echo "===== $f =====" wc -l "$f" sed -n '1,260p' "$f" echo done echo "== find lifecycle uses ==" rg -n "generate\\(|afterTestDataStored\\(|saveTestData\\(" backend --glob '*.kt'Repository: tolgee/tolgee-platform
Length of output: 50378
Make
currentTestDatarequest-local.
BranchMergeE2eDataControlleris singleton-scoped, so this shared mutable field can be overwritten by another request beforeafterTestDataStoredruns. Use request scope or pass theBranchMergeTestDatainstance through the callback instead of reading shared state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/development/src/mainEe/kotlin/io/tolgee/controllers/internal/e2eData/BranchMergeE2eDataController.kt` around lines 15 - 24, Make currentTestData in BranchMergeE2eDataController request-local so concurrent requests cannot overwrite the test data used by afterTestDataStored. Prefer request scope for the controller or propagate the BranchMergeTestData instance through the callback, ensuring afterTestDataStored reads the matching request’s data rather than singleton-shared state.
frontend-test-without-ee deletes the only ee-importing tests and the remaining webapp unit tests never touch tg.ee, so the OSS stub module (eeModule.oss) had no assertions: it could regress silently and the job would stay green. Add eeModule.oss.test.ts asserting the stub's load-bearing OSS behaviour (QA reported disabled, no billing/apps/task/ glossary contributions, menu adders leave items untouched). It imports eeModule.oss directly (like eeCheck.ts) so the stub is validated in both the with-ee and without-ee unit-test runs. Also correct the test.yml comment: tg.ee is a TypeScript path alias; the symlink prepareEe.js repoints is eeModule.current.
eeModule.oss.test.ts only restated the stub's own literals (() => false asserted toBe(false), literal [] asserted empty), so it could not catch a regression that matters, and it imported eeModule.oss directly, bypassing the tg.ee symlink production uses, so it gave frontend-test-without-ee no signal the ordinary frontend-test job lacks. Keep frontend-test-without-ee as a forward-looking guard: any future unit test that consumes ee through tg.ee is exercised against the OSS stub when src/ee is removed. Document that scope on the job; OSS type/build resolution is already covered by frontend-code-check's builds-without-ee step.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yml (1)
630-633: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the existing
frontend-testcheck in the final gate.This change appears to replace the normal frontend test result with
frontend-test-without-ee. Check both results; otherwise ordinary frontend test failures could be ignored when the enterprise-less job succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 630 - 633, Update the final gate to check both needs.frontend-test.result and needs.frontend-test-without-ee.result, adding each corresponding job to failed_jobs when its result is not success. Preserve the existing frontend-test failure detection while retaining the enterprise-less check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/test.yml:
- Around line 630-633: Update the final gate to check both
needs.frontend-test.result and needs.frontend-test-without-ee.result, adding
each corresponding job to failed_jobs when its result is not success. Preserve
the existing frontend-test failure detection while retaining the enterprise-less
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38a6ac3e-49af-4697-aa22-230933a2c482
📒 Files selected for processing (1)
.github/workflows/test.yml
The five-line comment restated the job name, carried PR-description rationale, and cross-referenced another job's step (which would rot if that step is renamed). The load-bearing ordering fact stays in the two-line comment at the Remove ee step.
- Drop the rot-prone symlink-internals parenthetical from the Remove ee comment; the pointer to prepareEe.js plus the ordering consequence stand on their own. - Pair src/mainEe/kotlin with src/mainEe/java in the :development source set, matching the kotlin+java pairing every sibling module uses (no-op today, but a future ee-only .java would otherwise be silently excluded).
Add job-level 'permissions: contents: read' and 'persist-credentials: false' to backend-build-without-ee and frontend-test-without-ee. Both only check out and build/test (no artifact upload, push, or check posting), so read-only access is sufficient. Cannot be set workflow-wide because the test-reporter jobs need checks: write.
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
1 participant
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.
Problem
CI covered building/testing the platform without the enterprise (ee) modules only partially:
runWithoutEeTestsreuses the with-ee jar (SKIP_SERVER_BUILD=true)frontend-code-checkremovessrc/ee)frontend-testalways ran with ee presentSo a change that broke the OSS build's compilation of the main backend could pass CI.
Change
Two new jobs in
test.yml, both wired into theeverything-passedgate:backend-build-without-ee—rm -rf ./ee, then a real./gradlew classes jar bootJarwithoutSKIP_SERVER_BUILD, verifying the main backend compiles and packages with the enterprise modules absent.frontend-test-without-ee—rm -rf ./src/eebeforenpm ciso thepreparehook (prepareEe.js) links the OSS stub (eeModule.current→eeModule.oss), then runs the webapp unit suite.Scope of
frontend-test-without-eeThis job runs the webapp unit suite with
src/eeremoved, so any unit test that reaches ee code through thetg.eealias is exercised against the OSS stub instead of the real ee module. No unit test does so today, so the job currently carries no unique signal — it is a forward-looking guard for future ee-consuming unit tests. OSS type/build resolution is already covered separately byfrontend-code-check's "Check it builds without ee directory" step. Kept as a deliberate tradeoff (small CI cost now for automatic OSS coverage of future ee-consuming unit tests).Defect this surfaced (also fixed here)
The new
backend-build-without-eejob immediately caught a pre-existing latent bug: the OSS backend did not compile without ee.BranchMergeE2eDataController(added in the branching feature, #3246) lives in the:developmentmodule, which is unconditionally onserver-app's classpath. But it imported the ee-onlyBranchSnapshotServicedirectly, while:developmentdepends on:ee-apponly when the ee directory exists. With ee removed, the module failed to compile.Fix: move the controller into a new ee-only source root (
src/mainEe/kotlin) added to the:developmentsource set only wheneeAppDirectoryExists, mirroring the existing conditional:ee-appdependency. Package is unchanged, so component scanning still registers it when ee is present.Summary by CodeRabbit