Repository navigation
RC-356 Extract Jetty incremental analysis into plugin integration tests - #6298
romainbrenguier wants to merge 8 commits into
Conversation
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 2 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 0403eb3 | Docs | View more details | Give us feedback! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
⚖️ Ruling update ready for review: #6325 |
Ruling Diff SummaryDetected changes in 8 rule files: 34 issues removed, 194 issues added. S9434 (
|
|
⚖️ Ruling update ready for review: #6325 |
fc08a49 to
e37fa35
Compare
|
RC-356 ruling audit: the large added-issue diff is the intentional conversion of the two Jetty PR corpora from PR-only dumps into independent full ruling scans, not an analyzer change. The ruling_java submodules explicitly use eclipse-jetty-same-issues-as-main and eclipse-jetty-same-issues-as-main-small; their full scans should match the main corpus. All three baselines contain 273 rule files / 39,264 issues and have been validated for unique JSON component keys, valid LITS lines (including project-level line 0), and the correct corpus project prefix. The rebase now provisions each full-scan corpus with its own key and preserves master’s RC-442 naming. Current-head ruling CI is the remaining end-to-end confirmation. Auto-created #6325 is already CLOSED and unmerged. It is not a valid update to apply: its files are under its/ruling/src/test/resources// rather than the expected// root, and its old project keys predate the rebase. I have not merged or imported it. #6298 remains draft. |
|
|
This PR is stale because it has been open 7 days with no activity. If there is no activity in the next 7 days it will be closed automatically |
cf9c7af to
319115d
Compare
Apply the S1695 fix and the new S9428 issue from master to the eclipse-jetty-similar-to-main and eclipse-jetty-similar-to-main-small baselines. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
319115d to
0403eb3
Compare
CI failed: Ruling QA for the Jetty incremental-analysis change failed on Windows: Sonar Maven reported 23 expected Jetty files not analyzed, and a separate ruling test reported 97 issue differences. Whether this PR caused it is unclear; the Linux job only generated a diff report without a shown failure.OverviewTwo unique error templates were analyzed across 8 logs. The Windows Ruling QA job shows a concrete test failure (expected files not analyzed, 97 issue differences), while the Linux Ruling QA excerpt only shows a diff report generation step with no explicit failing check. Neither finding establishes that this PR's changes caused the failure. FailuresJetty ruling test failure: expected files not analyzed and 97 issue differences (confidence: medium)
Linux Ruling QA diff report with no shown failure (confidence: low)
Summary
Code Review 👍 Approved with suggestions 3 closed / 4 findings🟡 Medium risk · Moving Jetty incremental checks into plugin tests and refreshing hundreds of ruling baselines could let incorrect analysis results pass unnoticed. Extracts Eclipse Jetty incremental analysis cache-reuse and timing checks into a dedicated 💡 Edge Case: -Dtest runs selected IT twice, once per surefire execution📄 its/plugin/tests/pom.xml:121-135 The new Gate the dedicated execution behind a property so targeted -Dtest runs (with -DskipIncrementalAnalysis) don't execute twice✅ 3 closed✅ Quality: New test file is missing the SSAL license header
✅ Bug: Jetty PR baselines were recorded with skipUnchanged; full analysis breaks them
✅ Edge Case: Timing assertions run alongside other IT classes under parallel=classes
🤖 Prompt for agentsVerification Rules 2/2 met Knowledge ✅ 1Functional validation No results Rules 2 passed 1 not applicable
Knowledge ✅ 1 expectation met
Auto-approval Not enabledFor more verification details, comment Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
|
⚖️ Ruling update ready for review: #6410 |
| <executions> | ||
| <execution> | ||
| <id>default-test</id> | ||
| <configuration> | ||
| <excludes> | ||
| <exclude>com/sonar/it/java/IncrementalAnalysisTest.java</exclude> | ||
| </excludes> | ||
| </configuration> | ||
| </execution> | ||
| <execution> | ||
| <id>incremental-analysis</id> | ||
| <goals> | ||
| <goal>test</goal> | ||
| </goals> | ||
| <configuration> |
There was a problem hiding this comment.
💡 Edge Case: -Dtest runs selected IT twice, once per surefire execution
The new incremental-analysis execution separates IncrementalAnalysisTest from the other tests using only <includes>/<excludes>. Surefire ignores both of those whenever -Dtest is passed (surefire test parameter), so a targeted run executes the selected class in both default-test and incremental-analysis. For example, the PR's own validation command -Dtest=IncrementalAnalysisTest runs the Jetty main-branch and PR analyses twice, and the default-test copy uses CI's -Dparallel=classes setting, which is the timing interference this change is meant to avoid. Any other -Dtest=FooTest run also executes FooTest a second time. Possible fixes: skip one execution when test is set (e.g. <skip> bound to a property), or move the incremental execution into its own profile or a property-gated execution.
Gate the dedicated execution behind a property so targeted -Dtest runs (with -DskipIncrementalAnalysis) don't execute twice:
<execution>
<id>incremental-analysis</id>
<goals>
<goal>test</goal>
</goals>
<configuration>
<skip>${skipIncrementalAnalysis}</skip>
<parallel>none</parallel>
<useUnlimitedThreads>false</useUnlimitedThreads>
<includes combine.self="override">
<include>com/sonar/it/java/IncrementalAnalysisTest.java</include>
</includes>
</configuration>
</execution>
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Summary
IncrementalAnalysisTestinits/plugin/tests.communityEditionTestsOnlywithout starting a server.JavaRulingTest.eclipse_jetty, with caching and unchanged-file skipping disabled. Fixtures and expected baselines stay in their existing locations.This branch is based directly on master and does not include the RC-442 changes.
Validation
mvn -o -pl its/plugin/tests,its/ruling -am test-compile -DskipTests -Ddownload.skip=true -Dcheckstyle.skip=true -Dlicense.skip=true -Dspotless.skip=truemvn -o -pl its/plugin/tests -am test -Pit-plugin -Dtest=IncrementalAnalysisTest -Dsurefire.failIfNoSpecifiedTests=false -DcommunityEditionTestsOnly=true -Ddownload.skip=true -Dcheckstyle.skip=true -Dlicense.skip=true -Dspotless.skip=true(test discovery and Community Edition guard only).git diff --check.its/ruling/src/test/java/org/sonar/java/it/JavaRulingTest.java:Summary by Gitar
ruling-update-notifynow runs whenruling-qasucceeds or fails, and flags ruling failure only on afailureresulteclipse-jetty-similar-to-mainexpected baselines and added new ruling expected filesErrorCollectorso all three Jetty comparisons run and report failures togetherThis will update automatically on new commits.
Agent workflow
Tool link: https://github.com/SonarSource/languages-experimental-tooling/tree/romain/my-tickets/personal/romain-brenguier
PR updated using
uv run update_with_claude.py --prompt "Implement the action plan described in the document Rebase on origin master fixing conflicts.." -a "Rebase on origin master fixing conflicts." -g "claude"