chore: add Github live sync integration tests#542
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds GitHub integration test infrastructure and configs; introduces integration test base classes and multiple end-to-end tests; applies runtime GitHub API bytecode patches at startup; changes label/milestone sync to batched listing with stale-entry cleanup; widens pull-request-review and review-comment processing to accept fallback pull request and user context. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Webhook Handler
participant SyncSvc as PR Review Sync Service
participant GitHub as GitHub API
participant DB as Local DB
Handler->>Handler: receive webhook
Handler->>SyncSvc: processPullRequestReview(review, pullRequest, sender)
note right of SyncSvc `#DDEBF7`: Defensive resolution with fallbacks
SyncSvc->>SyncSvc: if PR null -> try resolve
SyncSvc->>SyncSvc: if author null -> use sender fallback
SyncSvc->>GitHub: optional fetch missing refs
GitHub-->>SyncSvc: data or null
SyncSvc->>DB: persist/update review with resolved PR & author
DB-->>SyncSvc: confirmation
SyncSvc-->>Handler: return persisted review
sequenceDiagram
participant Test as Integration Test
participant SyncSvc as Label/Milestone Sync Service
participant GitHub as GitHub API
participant DB as Local DB
Test->>SyncSvc: trigger sync(repository)
SyncSvc->>GitHub: listAll remote entities
GitHub-->>SyncSvc: remote list
SyncSvc->>SyncSvc: record seen IDs
SyncSvc->>DB: create/update local entities
SyncSvc->>DB: query local entities
DB-->>SyncSvc: local list
SyncSvc->>SyncSvc: compute stale = local - seen
SyncSvc->>DB: delete stale entities
DB-->>SyncSvc: confirm deletions
SyncSvc-->>Test: sync complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
application-server
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive live integration tests for GitHub synchronization features, including repositories, issues, pull requests, labels, milestones, and teams. The tests interact with real GitHub API endpoints and validate that the sync services correctly handle create, update, and delete operations. The PR also enhances the label and milestone sync services to properly handle deletions by tracking which entities were seen during sync and removing any that are no longer present remotely.
Key changes:
- Implements end-to-end GitHub integration test infrastructure with automatic credential validation and resource cleanup
- Enhances label and milestone sync services to detect and remove stale entities
- Adds test scheduler and PostgreSQL shutdown configurations for the GitHub integration profile
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/application-server/src/test/resources/application-github-integration.yml | Configuration for GitHub integration tests with environment variable overrides |
| server/application-server/src/test/resources/application-github-integration-local.example.yml | Example local configuration template for developers to add their GitHub credentials |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/TestSchedulerConfiguration.java | Test-specific scheduler configuration with daemon threads and immediate shutdown |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/PostgreSQLTestContainer.java | Adds logging for PostgreSQL container initialization to aid debugging |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/GitHubIntegrationPostgresShutdown.java | Ensures PostgreSQL container is stopped cleanly for GitHub integration tests |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.java | Imports GitHub integration shutdown component for proper cleanup |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.java | Base class providing credential validation and common utilities for GitHub tests |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/AbstractGitHubSyncIntegrationTest.java | Abstract test class with helper methods for creating ephemeral GitHub resources and cleanup |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubTeamSyncIntegrationTest.java | Tests team synchronization and repository permission management |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubRepositorySyncIntegrationTest.java | Comprehensive test covering repository, issue, PR, label, milestone, collaborator, and team sync |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubMilestoneSyncIntegrationTest.java | Tests milestone sync including updates and deletions |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubLabelSyncIntegrationTest.java | Tests label sync including metadata updates and deletions |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/milestone/github/GitHubMilestoneSyncService.java | Enhances sync to detect and remove stale milestones that no longer exist remotely |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/milestone/MilestoneRepository.java | Adds query method to find milestones by repository ID |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/label/github/GitHubLabelSyncService.java | Enhances sync to detect and remove stale labels that no longer exist remotely |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/label/LabelRepository.java | Adds query method to find labels by repository ID |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/common/github/app/GitHubAppTokenService.java | Improves key material detection to check file content length, not just existence |
| server/application-server/pom.xml | Increases test timeouts to accommodate slower live API integration tests |
| .gitignore | Excludes local GitHub integration configuration containing secrets |
| } | ||
|
|
||
| protected String nextEphemeralSlug(String suffix) { | ||
| var prefix = environment.getProperty("integration-tests.github.ephemeral-prefix", "hephaestus-it"); |
There was a problem hiding this comment.
Property path mismatch: The configuration file defines integration-tests.ephemeral-prefix (line 28), but the code accesses integration-tests.github.ephemeral-prefix (line 89). This will cause the property to not be resolved correctly, and it will always fall back to the default value "hephaestus-it".
Update line 89 to use the correct property path:
var prefix = environment.getProperty("integration-tests.ephemeral-prefix", "hephaestus-it");| var prefix = environment.getProperty("integration-tests.github.ephemeral-prefix", "hephaestus-it"); | |
| var prefix = environment.getProperty("integration-tests.ephemeral-prefix", "hephaestus-it"); |
| if (raw == null || raw.isBlank()) { | ||
| throw new IllegalStateException("GitHub installation id not configured for integration tests."); | ||
| } | ||
| return Long.parseLong(raw); |
There was a problem hiding this comment.
Potential uncaught 'java.lang.NumberFormatException'.
| return Long.parseLong(raw); | |
| try { | |
| return Long.parseLong(raw); | |
| } catch (NumberFormatException e) { | |
| throw new IllegalStateException("GitHub installation id is not a valid number: '" + raw + "'", e); | |
| } |
github-actions
Bot
added
the
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/common/github/app/GitHubAppTokenService.java (1)
245-249: Consider logging the IOException for operational visibility.While treating IOException as "not present" is a reasonable fallback strategy, silently swallowing the exception could mask real configuration or file system issues (e.g., permission denied, I/O errors). For production troubleshooting, it's valuable to distinguish between "intentionally not configured" and "configured but inaccessible."
Consider logging at WARN or DEBUG level:
try { return privateKeyRes.exists() && privateKeyRes.contentLength() > 0; -} catch (IOException ignored) { +} catch (IOException e) { + log.warn("Failed to verify GitHub App private key resource: {}", e.getMessage()); return false; }Note: You'll need to add a logger field if not already present:
private static final org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger(GitHubAppTokenService.class);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.gitignore(1 hunks)docs/contributor/testing.mdx(1 hunks)package.json(1 hunks)server/application-server/pom.xml(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/common/github/app/GitHubAppTokenService.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/label/LabelRepository.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/label/github/GitHubLabelSyncService.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/milestone/MilestoneRepository.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/milestone/github/GitHubMilestoneSyncService.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreview/github/GitHubPullRequestReviewMessageHandler.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreview/github/GitHubPullRequestReviewSyncService.java(2 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/AbstractGitHubSyncIntegrationTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubLabelSyncIntegrationTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubMilestoneSyncIntegrationTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubRepositorySyncIntegrationTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubTeamSyncIntegrationTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreviewcomment/github/GitHubPullRequestReviewCommentMessageHandlerIntegrationTest.java(4 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/GitHubIntegrationPostgresShutdown.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/PostgreSQLTestContainer.java(2 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/TestSchedulerConfiguration.java(1 hunks)server/application-server/src/test/resources/application-github-integration-local.example.yml(1 hunks)server/application-server/src/test/resources/application-github-integration.yml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/application-server/src/test/**/*.java
📄 CodeRabbit inference engine (.github/instructions/java-tests.instructions.md)
server/application-server/src/test/**/*.java: Single responsibility in tests: one behavior/assertion per test method
Clear and concise tests: state the objective simply
Independent tests: avoid hidden dependencies between tests
Traceable tests: link each test directly to requirements (e.g., IDs/refs)
Repeatable tests: consistent setup and data to yield deterministic results
Maintainable tests: easy to update when system changes
Focus on risk: prioritize critical flows and edge cases first
Minimal setup: avoid unnecessary test setup steps
Fast execution: ensure tests run quickly for CI
Use realistic data: representative scenarios and inputs
Concise expected results: assert one clear outcome per test
Use Arrange-Act-Assert (AAA) structure to keep tests clear
Tests may run in parallel: avoid required cleanup and assume possible preexisting DB data
Files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubTeamSyncIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreviewcomment/github/GitHubPullRequestReviewCommentMessageHandlerIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/GitHubIntegrationPostgresShutdown.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubLabelSyncIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/AbstractGitHubSyncIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubMilestoneSyncIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/TestSchedulerConfiguration.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/PostgreSQLTestContainer.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubRepositorySyncIntegrationTest.java
🧠 Learnings (9)
📚 Learning: 2025-06-08T13:29:19.408Z
Learnt from: FelixTJDietrich
Repo: ls1intum/Hephaestus PR: 357
File: server/application-server/src/test/resources/github/push.json:130-149
Timestamp: 2025-06-08T13:29:19.408Z
Learning: Test webhook payload files in the Hephaestus project under `server/application-server/src/test/resources/github/` contain real events from the test organization "HephaestusTest" and test repositories, so they legitimately include actual user information like "FelixTJDietrich" rather than requiring anonymization.
Applied to files:
docs/contributor/testing.mdxserver/application-server/src/test/resources/application-github-integration-local.example.ymlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubRepositorySyncIntegrationTest.java
📚 Learning: 2025-09-29T02:06:20.575Z
Learnt from: ddmukh
Repo: ls1intum/Hephaestus PR: 401
File: server/application-server/src/main/java/de/tum/in/www1/hephaestus/organization/OrganizationSyncService.java:78-124
Timestamp: 2025-09-29T02:06:20.575Z
Learning: In the Hephaestus codebase, Organization entities use the GitHub organization ID directly as the primary key via BaseGitServiceEntity converter pattern. OrganizationMembershipRepository methods expect the GitHub ID as the organizationId parameter, not a separate auto-generated database ID.
Applied to files:
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/milestone/github/GitHubMilestoneSyncService.java
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Independent tests: avoid hidden dependencies between tests
Applied to files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubTeamSyncIntegrationTest.java.gitignoreserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubLabelSyncIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/AbstractGitHubSyncIntegrationTest.javaserver/application-server/pom.xmlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubMilestoneSyncIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.javaserver/application-server/src/test/resources/application-github-integration.ymlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubRepositorySyncIntegrationTest.java
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Minimal setup: avoid unnecessary test setup steps
Applied to files:
.gitignoreserver/application-server/pom.xmlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.java
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Maintainable tests: easy to update when system changes
Applied to files:
.gitignoreserver/application-server/pom.xmlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.java
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Fast execution: ensure tests run quickly for CI
Applied to files:
.gitignoreserver/application-server/pom.xml
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Tests may run in parallel: avoid required cleanup and assume possible preexisting DB data
Applied to files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.java
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Repeatable tests: consistent setup and data to yield deterministic results
Applied to files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.java
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Traceable tests: link each test directly to requirements (e.g., IDs/refs)
Applied to files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubRepositorySyncIntegrationTest.java
🧬 Code graph analysis (5)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubTeamSyncIntegrationTest.java (1)
server/intelligence-service/app/db/models_gen.py (1)
TeamRepositoryPermission(696-720)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/GitHubIntegrationPostgresShutdown.java (1)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/PostgreSQLTestContainer.java (1)
PostgreSQLTestContainer(17-203)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/AbstractGitHubSyncIntegrationTest.java (2)
server/intelligence-service/app/db/models_gen.py (2)
RepositoryToMonitor(657-693)Workspace(534-572)webapp/src/api/sdk.gen.ts (1)
getRepositoriesToMonitor(296-301)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/milestone/MilestoneRepository.java (1)
server/intelligence-service/app/db/models_gen.py (1)
Repository(472-513)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubRepositorySyncIntegrationTest.java (1)
server/intelligence-service/app/db/models_gen.py (4)
Milestone(599-628)PullRequestReview(932-960)RepositoryCollaborator(631-654)TeamRepositoryPermission(696-720)
🪛 Gitleaks (8.28.0)
server/application-server/src/test/resources/application-github-integration-local.example.yml
[high] 16-18: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 LanguageTool
docs/contributor/testing.mdx
[uncategorized] ~117-~117: The official name of this software platform is spelled with a capital “H”.
Context: ...ation id, and either an inline PEM key (github.app.privateKey) or a readable `private...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: docker-build / webapp-build / Build linux/amd64 Docker Image for ls1intum/hephaestus/webapp
- GitHub Check: docker-build / webhook-ingest-build / Build linux/amd64 Docker Image for ls1intum/hephaestus/webhook-ingest
- GitHub Check: test-suite / webapp-visual
- GitHub Check: test-suite / application-server-integration
- GitHub Check: quality-gates / database-schema-validation
- GitHub Check: quality-gates / database-models-validation
- GitHub Check: quality-gates / webapp-quality
- GitHub Check: quality-gates / openapi-validation
- GitHub Check: quality-gates / docs-quality
- GitHub Check: security-scan / dependencies
- GitHub Check: security-scan / sast
🔇 Additional comments (18)
package.json (1)
40-40: Clarify the purpose ofpatch-packagein the PR context.While the dependency addition is syntactically correct, the PR description doesn't explain why
patch-packageis needed for the GitHub integration tests. Please document:
- What patches will be applied and to which dependencies
- Why these patches are necessary for the test infrastructure
- Whether this is a temporary workaround or a permanent part of the build
docs/contributor/testing.mdx (2)
99-147: Documentation is clear, comprehensive, and well-structured. ✓The new section provides actionable prerequisites, straightforward setup instructions (with both file-based and environment-variable alternatives), clear running commands, and practical troubleshooting guidance. The content follows the existing documentation style and integrates naturally with the rest of the testing guide.
117-117: Static analysis false positive: capitalization flag is not applicable here.The linter flagged the capitalization of "GitHub" on this line, but the reference is to a code property name (
github.app.privateKey) within backticks, so it should not be changed. No action required.server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/TestSchedulerConfiguration.java (2)
9-11: LGTM! Good use of test profile isolation.The configuration class is properly scoped to the test profile, ensuring it won't interfere with production scheduler configuration.
15-23: No changes needed — the scheduler configuration is appropriate.The review comment's concerns about flaky tests from aggressive shutdown are unfounded. Tests do not schedule tasks or depend on task completion. The GitHub integration tests use direct API calls with
awaitConditionpolling (viaThread.sleep), not theTaskSchedulerbean. Production code uses the scheduler for long-running background jobs with cron-based triggers (hourly/12-hourly intervals), which don't interfere with individual test execution. The aggressive shutdown configuration is actually desirable for tests—preventing thread leaks and enabling fast teardown for CI.Likely an incorrect or invalid review comment.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreview/github/GitHubPullRequestReviewSyncService.java (1)
98-117: Graceful fallback for missing review parent.Catching the Hub4j NPE and reusing the payload pull request avoids losing reviews when the webhook omits the parent.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreview/github/GitHubPullRequestReviewMessageHandler.java (1)
47-48: Good to pass review context.Forwarding the pull request and sender ensures the sync service can persist even when Hub4j misses references.
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreviewcomment/github/GitHubPullRequestReviewCommentMessageHandlerIntegrationTest.java (1)
62-66: Preloading the review keeps tests deterministic.Seeding the review via the updated sync method mirrors production and removes hidden dependencies.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/label/LabelRepository.java (1)
22-22: LGTM!The derived query method follows Spring Data JPA conventions correctly. The underscore notation (
Repository_Id) properly navigates the nestedrepository.idproperty path, enabling efficient retrieval of all labels for a given repository.server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.java (1)
18-18: LGTM!The import of
GitHubIntegrationPostgresShutdownis conditionally activated via the@Profile("github-integration")annotation on that component. This ensures the shutdown hook only engages when GitHub integration tests run with bothtestandgithub-integrationprofiles active, while remaining inactive for standard integration tests.server/application-server/src/test/resources/application-github-integration.yml (1)
1-28: LGTM!The configuration follows best practices for optional integration tests:
- Imports local overrides via
optional:classpath:application-github-integration-local.yml- Uses environment variables with safe defaults
- Disables web layer and banner for cleaner test execution
- Tests relying on this configuration should validate credentials before running
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/label/github/GitHubLabelSyncService.java (3)
53-69: Excellent defensive programming and error handling.The addition of the
@Transactionalannotation ensures atomicity for the synchronization operation. The null check and IOException handling with early returns prevent partial synchronization when remote listing fails, maintaining data consistency.
70-74: LGTM!The batched synchronization with ID tracking is efficient. Using
HashSetwith a size hint minimizes allocations, and collecting remote labels before processing enables the subsequent cleanup pass.
76-88: LGTM!The stale label cleanup logic correctly identifies and removes local labels that no longer exist remotely. Logging the deletion count provides useful operational visibility.
server/application-server/pom.xml (2)
38-38: LGTM!The timeout increases (180s for tests, 60s for process exit) appropriately accommodate longer-running GitHub integration tests that interact with live APIs. The values align with the existing
local-db-timeoutprofile, maintaining consistency.Also applies to: 437-437
39-39: LGTM!The profile mechanism correctly implements opt-in GitHub integration testing:
- Default: excludes
github-integrationtagged tests- With
-Pgithub-integration-tests: clears exclusions, enabling all testsThis prevents accidental execution of tests requiring live credentials while maintaining easy activation when needed.
Also applies to: 438-438, 562-567
server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/GitHubIntegrationPostgresShutdown.java (1)
22-31: Verify shutdown hook coordination.
PostgreSQLTestContainer.startDockerContainer()already registers a JVM shutdown hook that stops the container. This@PreDestroyhook adds a second shutdown mechanism. While theisRunning()check prevents double-stop errors, verify that:
- Spring's
@PreDestroyexecution before JVM shutdown hooks doesn't cause timing issues- The shutdown hook is necessary given the existing JVM-level hook
- Parallel test execution doesn't create race conditions
The duplication might be intentional to ensure cleanup happens during Spring context shutdown rather than waiting for JVM exit, but confirming the rationale would be valuable.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/milestone/github/GitHubMilestoneSyncService.java (1)
63-99: LGTM!The synchronization logic correctly mirrors the pattern in
GitHubLabelSyncService, maintaining consistency across sync services. The implementation includes:
- Defensive null checks and error handling
- Efficient batch processing with ID tracking
- Stale milestone cleanup with logging
The consistent approach across sync services will simplify maintenance.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/contributor/testing.mdx(1 hunks)server/application-server/pom.xml(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/Application.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/config/GitHubApiPatches.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreviewcomment/github/GitHubPullRequestReviewCommentMessageHandler.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreviewcomment/github/GitHubPullRequestReviewCommentSyncService.java(4 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/common/GitHubPayloadExtension.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/application-server/src/test/**/*.java
📄 CodeRabbit inference engine (.github/instructions/java-tests.instructions.md)
server/application-server/src/test/**/*.java: Single responsibility in tests: one behavior/assertion per test method
Clear and concise tests: state the objective simply
Independent tests: avoid hidden dependencies between tests
Traceable tests: link each test directly to requirements (e.g., IDs/refs)
Repeatable tests: consistent setup and data to yield deterministic results
Maintainable tests: easy to update when system changes
Focus on risk: prioritize critical flows and edge cases first
Minimal setup: avoid unnecessary test setup steps
Fast execution: ensure tests run quickly for CI
Use realistic data: representative scenarios and inputs
Concise expected results: assert one clear outcome per test
Use Arrange-Act-Assert (AAA) structure to keep tests clear
Tests may run in parallel: avoid required cleanup and assume possible preexisting DB data
Files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/common/GitHubPayloadExtension.java
🧠 Learnings (3)
📚 Learning: 2025-06-08T13:29:19.408Z
Learnt from: FelixTJDietrich
Repo: ls1intum/Hephaestus PR: 357
File: server/application-server/src/test/resources/github/push.json:130-149
Timestamp: 2025-06-08T13:29:19.408Z
Learning: Test webhook payload files in the Hephaestus project under `server/application-server/src/test/resources/github/` contain real events from the test organization "HephaestusTest" and test repositories, so they legitimately include actual user information like "FelixTJDietrich" rather than requiring anonymization.
Applied to files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/common/GitHubPayloadExtension.java
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Independent tests: avoid hidden dependencies between tests
Applied to files:
server/application-server/pom.xml
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Maintainable tests: easy to update when system changes
Applied to files:
server/application-server/pom.xml
🪛 LanguageTool
docs/contributor/testing.mdx
[uncategorized] ~117-~117: The official name of this software platform is spelled with a capital “H”.
Context: ...ation id, and either an inline PEM key (github.app.privateKey) or a readable `private...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: docker-build / webhook-ingest-build / Build linux/amd64 Docker Image for ls1intum/hephaestus/webhook-ingest
- GitHub Check: docker-build / webhook-ingest-build / Build linux/arm64 Docker Image for ls1intum/hephaestus/webhook-ingest
- GitHub Check: docker-build / intelligence-service-build / Build linux/amd64 Docker Image for ls1intum/hephaestus/intelligence-service
- GitHub Check: docker-build / intelligence-service-build / Build linux/arm64 Docker Image for ls1intum/hephaestus/intelligence-service
- GitHub Check: docker-build / application-server-build / Build linux/arm64 Docker Image for ls1intum/hephaestus/application-server
- GitHub Check: docker-build / application-server-build / Build linux/amd64 Docker Image for ls1intum/hephaestus/application-server
- GitHub Check: docker-build / webapp-build / Build linux/amd64 Docker Image for ls1intum/hephaestus/webapp
- GitHub Check: docker-build / webapp-build / Build linux/arm64 Docker Image for ls1intum/hephaestus/webapp
- GitHub Check: quality-gates / openapi-validation
- GitHub Check: quality-gates / database-schema-validation
- GitHub Check: quality-gates / webapp-quality
- GitHub Check: quality-gates / docs-quality
- GitHub Check: test-suite / webapp-visual
- GitHub Check: test-suite / application-server-integration
- GitHub Check: security-scan / sast
🔇 Additional comments (5)
docs/contributor/testing.mdx (2)
136-139: I need to verify how the property is used to actually filter tests. Let me check the Surefire configuration:Now I need to check the actual maven-surefire-plugin configuration in the pom.xml to see how it uses the
hephaestus.githubIntegrationExcludedGroupsproperty:All verification checks passed — documentation is accurate.
The configuration correctly implements the described behavior:
- ✅ Maven profile exists: The
github-integration-testsprofile is configured with activation on thegroupsproperty- ✅ Tests excluded by default: Default property
hephaestus.githubIntegrationExcludedGroups=github-integrationensures GitHub integration tests (marked with@Tag("github-integration")) are excluded by the maven-surefire-plugin- ✅ Opt-in mechanism works: When the
groupsproperty is passed, the profile activates and setshephaestus.githubIntegrationExcludedGroupsto empty, allowing tagged tests to runThe documentation accurately reflects how maven-surefire-plugin uses excludedGroups configuration to exclude tests annotated with @tag.
99-155: All referenced components are correctly implemented and documented.Verification confirms:
- Test base classes (
AbstractGitHubSyncIntegrationTest,BaseGitHubIntegrationTest) exist- All helper methods are properly defined in
AbstractGitHubSyncIntegrationTest- Configuration files follow the correct pattern (
.example.ymltemplate + user-created-local.yml)- Maven profile
github-integration-testsis properly activated on thegroupsproperty with valuegithub-integration- Cleanup handlers are correctly implemented with
@BeforeEachclearing and@AfterEachexecuting deletions from provided lists- Documentation accurately reflects the implementation
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreviewcomment/github/GitHubPullRequestReviewCommentMessageHandler.java (1)
56-60: No issues found. The code correctly handles null sender.The downstream service
GitHubPullRequestReviewCommentSyncService.attachAuthor()implements a comprehensive three-tier fallback strategy: it attempts to fetch the user from the review comment itself, falls back to the provided sender if that fails, and gracefully handles the case where both are null by setting author to null without throwing exceptions. The change at lines 56-60 is safe and requires no modifications.server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreviewcomment/github/GitHubPullRequestReviewCommentSyncService.java (1)
167-197: Robust fallback author handlingGracefully using the provided
GHUserwhen the primary lookup fails keeps author mappings intact without introducing new failure modes. Nicely done.server/application-server/pom.xml (1)
38-38: Timeout adjustments align with longer-running integration tests.The increase from 60s to 180s and the forked process exit timeout from 10s to 60s are appropriate for integration tests that interact with external services like GitHub.
Also applies to: 437-438
| cd server/application-server/src/test/resources | ||
| cp application-github-integration-local.example.yml application-github-integration-local.yml | ||
| ``` | ||
|
|
||
| Fill in the placeholders with the sandbox organisation slug, the installation id, and either an inline PEM key (`github.app.privateKey`) or a readable `privateKeyLocation`. Keep this file out of version control—it is already listed in `.gitignore`. |
There was a problem hiding this comment.
🧩 Analysis chain
Verify configuration property names match implementation.
The section references an inline github.app.privateKey property and privateKeyLocation. Confirm these property names match the actual application configuration schema and that the documentation accurately reflects supported configuration options.
🏁 Script executed:
#!/bin/bash
# Description: Verify GitHub app property names in configuration
# Search for property definitions and usage
rg "github\.app\.privateKey|privateKeyLocation" --type=java --type=yaml -B 2 -A 2 | head -50Length of output: 5687
Update documentation to use the correct property name github.app.privateKeyLocation.
The documentation references privateKeyLocation, but the actual configuration property is github.app.privateKeyLocation. Update line 117 to use the full qualified property name for consistency with the inline github.app.privateKey reference.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~117-~117: The official name of this software platform is spelled with a capital “H”.
Context: ...ation id, and either an inline PEM key (github.app.privateKey) or a readable `private...
(GITHUB)
🤖 Prompt for AI Agents
In docs/contributor/testing.mdx around lines 113 to 117, the docs mention the
config key `privateKeyLocation` but the correct fully-qualified property is
`github.app.privateKeyLocation`; update the text on line 117 to reference
`github.app.privateKeyLocation` (so it matches the inline
`github.app.privateKey` reference) and ensure the example and explanatory
sentence use the fully qualified name.
| <maven.test.skip>false</maven.test.skip> | ||
| <hephaestus.surefire.timeout>60</hephaestus.surefire.timeout> | ||
| <hephaestus.surefire.timeout>180</hephaestus.surefire.timeout> | ||
| <hephaestus.githubIntegrationExcludedGroups>github-integration</hephaestus.githubIntegrationExcludedGroups> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify test group exclusion strategy and profile activation semantics.
The approach uses a property-driven exclusion model:
- Default:
githubIntegrationExcludedGroups=github-integrationexcludes GitHub integration tests from Surefire - Profile
github-integration-tests: activated by presence ofgroupsproperty, sets excludedGroups to empty
However, the integration is unclear:
- The profile activation property
<name>groups</name>is generic and matches any-Dgroups=...flag, but doesn't explicitly control which groups to run - Setting
excludedGroupsto empty allows all tests to run, but relies on integration tests being properly tagged with@Tag("github-integration") - Contrast with Failsafe (line 452), which uses
<groups>integration</groups>to explicitly include tests
Please verify:
- Are GitHub integration tests consistently tagged with
@Tag("github-integration")? - Does setting an empty
excludedGroupsstring behave as expected in Surefire? - Is the activation on the
groupsproperty sufficient, or should the profile be activated more explicitly?
Run the following to verify test tagging and the profile setup:
Also applies to: 438-438, 562-572
🏁 Script executed:
#!/bin/bash
# Description: Verify GitHub integration tests are tagged correctly and profile activation works
# Find all test files under gitprovider/github/
echo "=== GitHub integration test files ==="
find server/application-server/src/test/java -path '*/github/*' -name '*Test.java' -o -name '*IntegrationTest.java' | head -20
# Check for @Tag("github-integration") annotations in these files
echo "=== Checking for @Tag(\"github-integration\") annotations ==="
rg -n '@Tag\s*\(\s*["\x27]github-integration["\x27]\s*\)' server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/
# Verify that integration test files exist and check their structure
echo "=== Sample of integration test classes ==="
find server/application-server/src/test/java -path '*/github/*' -name '*IntegrationTest.java' -exec head -30 {} \; | head -60Length of output: 6291
I need to verify the inheritance model for the @tag annotation and check the pom.xml profile configuration more closely.
Now I need to check the pom.xml configuration to understand the profile setup and the Surefire/Failsafe configuration more clearly.
🏁 Script executed:
#!/bin/bash
# Extract pom.xml sections around the mentioned lines for analysis
echo "=== Maven profiles section (around line 438) ==="
sed -n '430,450p' server/application-server/pom.xml
echo ""
echo "=== Maven profiles section (around line 562-572) ==="
sed -n '555,580p' server/application-server/pom.xml
echo ""
echo "=== Search for profile 'github-integration-tests' ==="
rg -n -A15 'github-integration-tests' server/application-server/pom.xmlLength of output: 2479
Now I need to check the JUnit 5 version in use and verify the test inheritance chain:
Now I need to check the inheritance chain and JUnit version to understand the complete picture:
🏁 Script executed:
#!/bin/bash
# Check JUnit version and test inheritance chain
echo "=== JUnit Jupiter version in pom.xml ==="
rg -n 'junit-jupiter-api|junit-jupiter-engine' server/application-server/pom.xml | head -5
echo ""
echo "=== BaseGitHubIntegrationTest source ==="
cat server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.java
echo ""
echo "=== BaseIntegrationTest source (first 40 lines) ==="
find server/application-server/src/test/java -name 'BaseIntegrationTest.java' -exec head -40 {} \;Length of output: 6278
Confirm tagging issue: GitHub integration tests are NOT properly tagged for exclusion.
The concrete GitHub integration test classes (GitHubInstallationMessageHandlerIntegrationTest, GitHubIssueMessageHandlerIntegrationTest, etc.) extend BaseIntegrationTest and inherit only @Tag("integration"). They do NOT inherit @Tag("github-integration") because BaseGitHubIntegrationTest is separate and not in their inheritance chain.
Since Surefire's default excludedGroups=github-integration targets the "github-integration" tag specifically, these tests will NOT be excluded by default and will run during normal unit test phases—contrary to the intended behavior of keeping GitHub integration tests separate unless explicitly activated.
The inheritance structure should be:
- Concrete tests → BaseGitHubIntegrationTest → BaseIntegrationTest
Or each concrete test must carry an explicit @Tag("github-integration") annotation.
🤖 Prompt for AI Agents
In server/application-server/pom.xml around line 39, the Maven Surefire
exclusion for github-integration groups is ineffective because concrete GitHub
integration tests don't carry the "github-integration" tag (they only inherit
@Tag("integration") from BaseIntegrationTest); update the test inheritance or
annotations so Surefire excludes work: either make concrete test classes extend
BaseGitHubIntegrationTest (so inheritance becomes Concrete →
BaseGitHubIntegrationTest → BaseIntegrationTest) or add explicit
@Tag("github-integration") to each concrete GitHub integration test class; apply
the chosen fix consistently to all GitHub integration test classes so they are
excluded by the existing excludedGroups setting.
| var userField = clazz.getDeclaredField("user"); | ||
| userField.setAccessible(true); | ||
| Object user = userField.get(self); | ||
| GHUser user = (GHUser) userField.get(self); | ||
|
|
||
| var ownerField = clazz.getDeclaredField("owner"); | ||
| ownerField.setAccessible(true); | ||
| Object owner = ownerField.get(self); | ||
|
|
||
| if (owner == null) { | ||
| return user; | ||
| } | ||
|
|
||
| // owner has isOffline() | ||
| var isOfflineMethod = owner.getClass().getMethod("isOffline"); | ||
| boolean isOffline = (Boolean) isOfflineMethod.invoke(owner); | ||
| if (isOffline) { | ||
| Object offline = owner.getClass().getMethod("isOffline").invoke(owner); | ||
| if (Boolean.TRUE.equals(offline)) { | ||
| return user; | ||
| } | ||
|
|
||
| if (user == null) { | ||
| return null; | ||
| } | ||
|
|
||
| // user has login field (package-private), fallback to getLogin() | ||
| String login = null; | ||
| try { | ||
| var loginField = user.getClass().getDeclaredField("login"); | ||
| loginField.setAccessible(true); | ||
| Object v = loginField.get(user); | ||
| login = v != null ? v.toString() : null; | ||
| } catch (NoSuchFieldException ignore) { | ||
| try { | ||
| var getLogin = user.getClass().getMethod("getLogin"); | ||
| Object v = getLogin.invoke(user); | ||
| login = v != null ? v.toString() : null; | ||
| } catch (ReflectiveOperationException ex) { | ||
| // ignore | ||
| } | ||
| } | ||
|
|
||
| if (login == null) { | ||
| return user; | ||
| } | ||
|
|
||
| // owner.root().getUser(login) | ||
| var rootMethod = owner.getClass().getMethod("root"); | ||
| Object root = rootMethod.invoke(owner); | ||
| if (root == null) { | ||
| if (user != null) { | ||
| return user; | ||
| } | ||
| var getUserMethod = root.getClass().getMethod("getUser", String.class); | ||
| return getUserMethod.invoke(root, login); | ||
| } catch (Throwable t) { | ||
| // On any unexpected error, return the embedded user to be safe | ||
| try { | ||
| var userField = self.getClass().getDeclaredField("user"); | ||
| userField.setAccessible(true); | ||
| return userField.get(self); | ||
| } catch (Throwable inner) { | ||
| var field = self.getClass().getDeclaredField("user"); | ||
| field.setAccessible(true); | ||
| return (GHUser) field.get(self); | ||
| } catch (Throwable ignored) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix field lookup to handle superclass declarations.
getDeclaredField("user") (and later "owner") only sees fields declared on the concrete type. For classes like GHPullRequestReviewComment, both members live in superclasses (GHComment / GHObject). The first lookup therefore throws, we hit the catch block, and ultimately return null. Because @Advice.OnMethodEnter then yields the default value, Byte Buddy executes the original getUser(), so the HTTP call (and the offline crash you are trying to avoid) still happens. This regresses the existing workaround for review comments and reintroduces the production failure.
Please resolve by walking the class hierarchy (or otherwise making the field accessible) before accessing it. One option:
- var clazz = self.getClass();
-
- var userField = clazz.getDeclaredField("user");
- userField.setAccessible(true);
- GHUser user = (GHUser) userField.get(self);
+ var clazz = self.getClass();
+
+ var userField = findField(clazz, "user");
+ if (userField == null) {
+ return null;
+ }
+ GHUser user = (GHUser) userField.get(self);
@@
- var ownerField = clazz.getDeclaredField("owner");
- ownerField.setAccessible(true);
+ var ownerField = findField(clazz, "owner");
+ if (ownerField == null) {
+ return user;
+ }
Object owner = ownerField.get(self);
@@
- var field = self.getClass().getDeclaredField("user");
- field.setAccessible(true);
- return (GHUser) field.get(self);
+ var field = findField(self.getClass(), "user");
+ return field != null ? (GHUser) field.get(self) : null;
} catch (Throwable ignored) {
return null;
}
}
@@
}
+
+ private static java.lang.reflect.Field findField(Class<?> type, String name) {
+ for (Class<?> current = type; current != null; current = current.getSuperclass()) {
+ try {
+ var field = current.getDeclaredField(name);
+ field.setAccessible(true);
+ return field;
+ } catch (NoSuchFieldException ignored) {
+ // try next superclass
+ }
+ }
+ return null;
+ }This restores the intent: we short-circuit when the embedded user is available and never invoke the remote API in offline mode.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var userField = clazz.getDeclaredField("user"); | |
| userField.setAccessible(true); | |
| Object user = userField.get(self); | |
| GHUser user = (GHUser) userField.get(self); | |
| var ownerField = clazz.getDeclaredField("owner"); | |
| ownerField.setAccessible(true); | |
| Object owner = ownerField.get(self); | |
| if (owner == null) { | |
| return user; | |
| } | |
| // owner has isOffline() | |
| var isOfflineMethod = owner.getClass().getMethod("isOffline"); | |
| boolean isOffline = (Boolean) isOfflineMethod.invoke(owner); | |
| if (isOffline) { | |
| Object offline = owner.getClass().getMethod("isOffline").invoke(owner); | |
| if (Boolean.TRUE.equals(offline)) { | |
| return user; | |
| } | |
| if (user == null) { | |
| return null; | |
| } | |
| // user has login field (package-private), fallback to getLogin() | |
| String login = null; | |
| try { | |
| var loginField = user.getClass().getDeclaredField("login"); | |
| loginField.setAccessible(true); | |
| Object v = loginField.get(user); | |
| login = v != null ? v.toString() : null; | |
| } catch (NoSuchFieldException ignore) { | |
| try { | |
| var getLogin = user.getClass().getMethod("getLogin"); | |
| Object v = getLogin.invoke(user); | |
| login = v != null ? v.toString() : null; | |
| } catch (ReflectiveOperationException ex) { | |
| // ignore | |
| } | |
| } | |
| if (login == null) { | |
| return user; | |
| } | |
| // owner.root().getUser(login) | |
| var rootMethod = owner.getClass().getMethod("root"); | |
| Object root = rootMethod.invoke(owner); | |
| if (root == null) { | |
| if (user != null) { | |
| return user; | |
| } | |
| var getUserMethod = root.getClass().getMethod("getUser", String.class); | |
| return getUserMethod.invoke(root, login); | |
| } catch (Throwable t) { | |
| // On any unexpected error, return the embedded user to be safe | |
| try { | |
| var userField = self.getClass().getDeclaredField("user"); | |
| userField.setAccessible(true); | |
| return userField.get(self); | |
| } catch (Throwable inner) { | |
| var field = self.getClass().getDeclaredField("user"); | |
| field.setAccessible(true); | |
| return (GHUser) field.get(self); | |
| } catch (Throwable ignored) { | |
| return null; | |
| } | |
| } | |
| var clazz = self.getClass(); | |
| var userField = findField(clazz, "user"); | |
| if (userField == null) { | |
| return null; | |
| } | |
| GHUser user = (GHUser) userField.get(self); | |
| var ownerField = findField(clazz, "owner"); | |
| if (ownerField == null) { | |
| return user; | |
| } | |
| Object owner = ownerField.get(self); | |
| if (owner == null) { | |
| return user; | |
| } | |
| Object offline = owner.getClass().getMethod("isOffline").invoke(owner); | |
| if (Boolean.TRUE.equals(offline)) { | |
| return user; | |
| } | |
| if (user != null) { | |
| return user; | |
| } | |
| } catch (Throwable t) { | |
| try { | |
| var field = findField(self.getClass(), "user"); | |
| return field != null ? (GHUser) field.get(self) : null; | |
| } catch (Throwable ignored) { | |
| return null; | |
| } | |
| } | |
| } | |
| private static java.lang.reflect.Field findField(Class<?> type, String name) { | |
| for (Class<?> current = type; current != null; current = current.getSuperclass()) { | |
| try { | |
| var field = current.getDeclaredField(name); | |
| field.setAccessible(true); | |
| return field; | |
| } catch (NoSuchFieldException ignored) { | |
| // try next superclass | |
| } | |
| } | |
| return null; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
AGENTS.md(8 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/Application.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequestreviewcomment/github/GitHubPullRequestReviewCommentSyncService.java(3 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/common/GitHubPayloadExtension.java(2 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/AbstractGitHubSyncIntegrationTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubLabelSyncIntegrationTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (3)
- server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/GitHubLabelSyncIntegrationTest.java
- server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/common/GitHubPayloadExtension.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/Application.java
🧰 Additional context used
📓 Path-based instructions (1)
server/application-server/src/test/**/*.java
📄 CodeRabbit inference engine (.github/instructions/java-tests.instructions.md)
server/application-server/src/test/**/*.java: Single responsibility in tests: one behavior/assertion per test method
Clear and concise tests: state the objective simply
Independent tests: avoid hidden dependencies between tests
Traceable tests: link each test directly to requirements (e.g., IDs/refs)
Repeatable tests: consistent setup and data to yield deterministic results
Maintainable tests: easy to update when system changes
Focus on risk: prioritize critical flows and edge cases first
Minimal setup: avoid unnecessary test setup steps
Fast execution: ensure tests run quickly for CI
Use realistic data: representative scenarios and inputs
Concise expected results: assert one clear outcome per test
Use Arrange-Act-Assert (AAA) structure to keep tests clear
Tests may run in parallel: avoid required cleanup and assume possible preexisting DB data
Files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/AbstractGitHubSyncIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.java
🧠 Learnings (4)
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Independent tests: avoid hidden dependencies between tests
Applied to files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/AbstractGitHubSyncIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.java
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Focus on risk: prioritize critical flows and edge cases first
Applied to files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/AbstractGitHubSyncIntegrationTest.java
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Repeatable tests: consistent setup and data to yield deterministic results
Applied to files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.java
📚 Learning: 2025-09-09T15:56:45.899Z
Learnt from: CR
Repo: ls1intum/Hephaestus PR: 0
File: .github/instructions/java-tests.instructions.md:0-0
Timestamp: 2025-09-09T15:56:45.899Z
Learning: Applies to server/application-server/src/test/**/*.java : Minimal setup: avoid unnecessary test setup steps
Applied to files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.java
🧬 Code graph analysis (1)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/AbstractGitHubSyncIntegrationTest.java (1)
server/intelligence-service/app/db/models_gen.py (2)
RepositoryToMonitor(657-693)Workspace(534-572)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: docker-build / application-server-build / Build linux/arm64 Docker Image for ls1intum/hephaestus/application-server
- GitHub Check: docker-build / webapp-build / Build linux/amd64 Docker Image for ls1intum/hephaestus/webapp
- GitHub Check: docker-build / webapp-build / Build linux/arm64 Docker Image for ls1intum/hephaestus/webapp
- GitHub Check: quality-gates / webapp-quality
- GitHub Check: quality-gates / webhook-ingest-quality
- GitHub Check: quality-gates / openapi-validation
- GitHub Check: quality-gates / docs-quality
- GitHub Check: test-suite / webapp-visual
- GitHub Check: test-suite / application-server-integration
- GitHub Check: test-suite / application-server-unit
- GitHub Check: security-scan / sast
| private void attachAuthor( | ||
| GHPullRequestReviewComment ghPullRequestReviewComment, | ||
| PullRequestReviewComment comment, | ||
| GHUser fallbackUser | ||
| ) { | ||
| GHUser user = null; | ||
| try { | ||
| GHUser user = ghPullRequestReviewComment.getUser(); | ||
| if (user == null) { | ||
| comment.setAuthor(null); | ||
| return; | ||
| } | ||
| var resultAuthor = userRepository | ||
| .findById(user.getId()) | ||
| .orElseGet(() -> userRepository.save(userConverter.convert(user))); | ||
| comment.setAuthor(resultAuthor); | ||
| user = ghPullRequestReviewComment.getUser(); | ||
| } catch (IOException | NullPointerException e) { | ||
| logger.error( | ||
| "Failed to link author for pull request review comment {}: {}", | ||
| "Failed to fetch author via API for pull request review comment {}: {}", | ||
| ghPullRequestReviewComment.getId(), | ||
| e.getMessage() | ||
| ); | ||
| } | ||
|
|
||
| if (user == null) { | ||
| user = fallbackUser; | ||
| } | ||
|
|
||
| if (user == null) { | ||
| comment.setAuthor(null); | ||
| return; | ||
| } | ||
|
|
||
| var resultAuthor = userRepository.findById(user.getId()).orElse(null); | ||
| if (resultAuthor == null) { | ||
| resultAuthor = userRepository.save(userConverter.convert(user)); | ||
| } | ||
| comment.setAuthor(resultAuthor); | ||
| } |
There was a problem hiding this comment.
Guard fallback author to avoid misattribution
pull_request_review_comment webhooks fire for created, edited, and deleted actions, and the payload’s sender is simply the actor who triggered the event. (docs.github.com) When an editor with write access tweaks someone else’s review comment and the GitHub API can’t return the original user, this branch now overwrites the stored author with the editor instead of preserving the true author. Please gate the fallback so it only runs when we know the sender is the comment owner (e.g., creation events) or add a verification step before replacing the author; otherwise, keep the existing author to avoid corrupting attribution.
| long appId = environment.getProperty("github.app.id", Long.class, 0L); | ||
| var privateKey = environment.getProperty("github.app.privateKey", ""); | ||
| var privateKeyLocation = environment.getProperty("github.app.privateKeyLocation", ""); | ||
| var authToken = environment.getProperty("github.meta.auth-token", ""); | ||
| var installationId = environment.getProperty("integration-tests.github.installation-id", ""); | ||
| boolean missingKeyMaterial = privateKey.isBlank(); | ||
| if (missingKeyMaterial && !privateKeyLocation.isBlank()) { | ||
| var resource = resourceLoader.getResource(privateKeyLocation); | ||
| try (var input = resource.exists() ? resource.getInputStream() : null) { | ||
| missingKeyMaterial = input == null || input.read() == -1; | ||
| } catch (Exception ignored) { | ||
| missingKeyMaterial = true; | ||
| } | ||
| } | ||
| boolean missingToken = authToken.isBlank(); | ||
| boolean missingInstallation = installationId.isBlank(); | ||
| boolean missingAppId = appId <= 0; | ||
|
|
||
| Assumptions.assumeFalse( | ||
| missingAppId || missingKeyMaterial || missingToken || missingInstallation, | ||
| () -> | ||
| "GitHub integration credentials missing. Copy the example file to " + | ||
| "application-github-integration-local.yml (kept out of VCS) or provide " + | ||
| "environment overrides." | ||
| ); |
There was a problem hiding this comment.
Add organization property to assumption guard.
assumeGitHubCredentialsConfigured() skips the suite when key credentials are missing, but it never checks integration-tests.github.organization. If that property is absent, the assumption still passes, the tests run, and any call to fetchOrganization() will fail with a confusing GitHub API error instead of being skipped. Please include the organization in the missing-credential check so misconfigured environments short-circuit cleanly.
var privateKey = environment.getProperty("github.app.privateKey", "");
var privateKeyLocation = environment.getProperty("github.app.privateKeyLocation", "");
var authToken = environment.getProperty("github.meta.auth-token", "");
var installationId = environment.getProperty("integration-tests.github.installation-id", "");
+ var organization = environment.getProperty("integration-tests.github.organization", "");
boolean missingKeyMaterial = privateKey.isBlank();
if (missingKeyMaterial && !privateKeyLocation.isBlank()) {
var resource = resourceLoader.getResource(privateKeyLocation);
try (var input = resource.exists() ? resource.getInputStream() : null) {
missingKeyMaterial = input == null || input.read() == -1;
} catch (Exception ignored) {
missingKeyMaterial = true;
}
}
boolean missingToken = authToken.isBlank();
boolean missingInstallation = installationId.isBlank();
boolean missingAppId = appId <= 0;
+ boolean missingOrganization = organization.isBlank();
Assumptions.assumeFalse(
- missingAppId || missingKeyMaterial || missingToken || missingInstallation,
+ missingAppId || missingKeyMaterial || missingToken || missingInstallation || missingOrganization,
() ->
"GitHub integration credentials missing. Copy the example file to " +
"application-github-integration-local.yml (kept out of VCS) or provide " +
"environment overrides."
);🤖 Prompt for AI Agents
In
server/application-server/src/test/java/de/tum/in/www1/hephaestus/gitprovider/github/BaseGitHubIntegrationTest.java
around lines 45 to 69, the assumption guard currently checks appId,
privateKey/privateKeyLocation, authToken, and installationId but omits
integration-tests.github.organization; read
environment.getProperty("integration-tests.github.organization", "") into a
variable, set a missingOrganization boolean if it's blank, include
missingOrganization in the combined predicate passed to Assumptions.assumeFalse,
and update the assumption message if desired so that missing organization will
cause the suite to be skipped cleanly instead of letting tests run and fail with
API errors.
|
🎉 This PR is included in version 0.10.0-rc.21 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 0.10.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Description
This pull request updates repository documentation and build/test configuration to support live GitHub sync integration tests, clarify contributor expectations, and improve test reliability. The most important changes include enhancements to the
AGENTS.mdcontributor guide, new documentation for running GitHub integration tests, updates to Maven configuration for test profiles and timeouts, and the addition of a new dependency for patch management.Contributor documentation and workflow improvements:
AGENTS.mdwith clearer architecture, toolchain requirements, routine commands, code generation rules, frontend/backend expectations, database workflow, commit checklist, and known caveats. This provides step-by-step guidance for contributors across all major repo areas. [1] [2] [3] [4] [5] [6] [7] [8]GitHub sync integration test support:
docs/contributor/testing.mdxdetailing how to run and author live GitHub sync integration tests, including prerequisites, configuration, troubleshooting, and best practices for resource clean-up.Build and test configuration updates:
server/application-server/pom.xmlto increase test timeouts, add agithub-integration-testsMaven profile, and ensure GitHub integration tests are excluded unless explicitly enabled. This improves reliability and makes it easier to opt-in to live sync tests locally. [1] [2] [3]Tooling and dependency management:
patch-packagetopackage.jsondevDependencies for managing hotfixes to npm packages.Application server initialization:
Application.javato apply GitHub API patches on startup, ensuring customizations are loaded for integration scenarios. [1] [2] [3]Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests
Chores