Skip to content

SONARJAVA-6591 Add ruling diff comment workflow#5761

Open
romainbrenguier wants to merge 3 commits into
masterfrom
romain/test-ruling-diff-action
Open

SONARJAVA-6591 Add ruling diff comment workflow#5761
romainbrenguier wants to merge 3 commits into
masterfrom
romain/test-ruling-diff-action

Conversation

@romainbrenguier

@romainbrenguier romainbrenguier commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Based on action from core-languages-tooling.
Example comment here: #5761 (comment)


Summary by Gitar

  • CI/CD Configuration:
    • Added .github/workflows/ruling-diff-comment.yml to automate posting ruling diff comments on PRs.
    • Configured workflow to trigger on changes to its/ruling/src/test/resources/**/*.json files and manual workflow_dispatch events.
  • Test Optimization:
    • Added @Ignore to seven test methods in JavaRulingTest.java to accelerate CI execution.
  • Resource Cleanup:
    • Updated its/ruling/src/test/resources/commons-beanutils/java-S1134.json by adjusting issue line numbers and removing a stale reference.

This will update automatically on new commits.

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod Bot changed the title Add ruling diff comment workflow SONARJAVA-6591 Add ruling diff comment workflow Jul 8, 2026
@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6591

Comment thread its/ruling/src/test/java/org/sonar/java/it/JavaRulingTest.java Outdated
romainbrenguier and others added 2 commits July 10, 2026 14:35
Based on action from core-languages-tooling.

Parametrize action for sonar-java

Initialize submodules for ruling diff workflow

The submodules must be initialized to access source files for
generating code snippets in ruling diff comments.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Optimize ruling tests for faster CI

- Exclude most ruling tests (guava, mall, jetty, etc.) using @ignore
- Keep only commons-beanutils test active for PR validation
- Add test changes to commons-beanutils S1134 to demonstrate diff comments

This is a temporary change to speed up CI for demonstrating the
ruling diff comment feature. Not intended for merge to master.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@romainbrenguier romainbrenguier force-pushed the romain/test-ruling-diff-action branch from 9c34fcb to ac526fa Compare July 10, 2026 12:35
@SonarSource SonarSource deleted a comment from github-actions Bot Jul 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Ruling Diff Summary

Detected changes in 1 rule files: 4 issues removed, 3 issues added.

S1134 (java) on commons-beanutils - 4 issues removed, 3 issues added

Added src/main/java/org/apache/commons/beanutils2/BasicDynaClass.java (line 215)

       210 |      *  constructor is not accessible
       211 |      * @throws InstantiationException if this Class represents an abstract
       212 |      *  class, an array class, a primitive type, or void; or if instantiation
       213 |      *  fails for some other reason
       214 |      */
>>>    215 |     @Override
       216 |     public DynaBean newInstance()
       217 |             throws IllegalAccessException, InstantiationException {
       218 | 
       219 |         try {
       220 |             // Refind the constructor after a deserialization (if needed)

Removed src/main/java/org/apache/commons/beanutils2/BeanUtilsBean.java (line 213)

       208 |      *
       209 |      * <p>If you know that no type conversions are required, the
       210 |      * <code>copyProperties()</code> method in {@link PropertyUtils} will
       211 |      * execute faster than this method.</p>
       212 |      *
>>>    213 |      * <p><strong>FIXME</strong> - Indexed and mapped properties that do not
       214 |      * have getter and setter methods for the underlying array or Map are not
       215 |      * copied by this method.</p>
       216 |      *
       217 |      * @param dest Destination bean whose properties are modified
       218 |      * @param orig Origin bean whose properties are retrieved

Removed src/main/java/org/apache/commons/beanutils2/DynaClass.java (line 60)

        55 |     /**
        56 |      * <p>Returns an array of {@code ProperyDescriptors} for the properties
        57 |      * currently defined in this DynaClass.  If no properties are defined, a
        58 |      * zero-length array will be returned.</p>
        59 |      *
>>>     60 |      * <p><strong>FIXME</strong> - Should we really be implementing
        61 |      * {@code getBeanInfo()} instead, which returns property descriptors
        62 |      * and a bunch of other stuff?</p>
        63 |      *
        64 |      * @return the set of properties for this DynaClass
        65 |      */

Removed src/main/java/org/apache/commons/beanutils2/PropertyUtilsBean.java (line 722)

       717 | 
       718 | 
       719 |     /**
       720 |      * <p>Return the mapped property descriptors for this bean.</p>
       721 |      *
>>>    722 |      * <p><strong>FIXME</strong> - Does not work with DynaBeans.</p>
       723 |      *
       724 |      * @param bean Bean to be introspected
       725 |      * @return the mapped property descriptors
       726 |      */
       727 |     Map getMappedPropertyDescriptors(final Object bean) {

Removed src/main/java/org/apache/commons/beanutils2/PropertyUtilsBean.java (line 1182)

      1177 | 
      1178 |     /**
      1179 |      * <p>Return an accessible property getter method for this property,
      1180 |      * if there is one; otherwise return <code>null</code>.</p>
      1181 |      *
>>>   1182 |      * <p><strong>FIXME</strong> - Does not work with DynaBeans.</p>
      1183 |      *
      1184 |      * @param clazz The class of the read method will be invoked on
      1185 |      * @param descriptor Property descriptor to return a getter for
      1186 |      * @return The read method
      1187 |      */

Added src/main/java/org/apache/commons/beanutils2/PropertyUtilsBean.java (line 719)

       714 |         return mappedDescriptorsCache.get(beanClass);
       715 | 
       716 |     }
       717 | 
       718 | 
>>>    719 |     /**
       720 |      * <p>Return the mapped property descriptors for this bean.</p>
       721 |      *
       722 |      * <p><strong>FIXME</strong> - Does not work with DynaBeans.</p>
       723 |      *
       724 |      * @param bean Bean to be introspected

Added src/main/java/org/apache/commons/beanutils2/PropertyUtilsBean.java (line 1203)

      1198 |      * @param name Name of the property to be extracted
      1199 |      * @return The property value
      1200 |      *
      1201 |      * @throws IllegalAccessException if the caller does not have
      1202 |      *  access to the property accessor method
>>>   1203 |      * @throws IllegalArgumentException if <code>bean</code> or
      1204 |      *  <code>name</code> is null
      1205 |      * @throws IllegalArgumentException if the property name
      1206 |      *  is nested or indexed
      1207 |      * @throws InvocationTargetException if the property accessor method
      1208 |      *  throws an exception

@gitar-bot

gitar-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 1 resolved / 1 findings

Automates ruling diff comments via a new workflow and optimizes CI performance by disabling specific ruling tests. The removal of temporary test commits and stale resource references resolves the previous demo-only issues.

✅ 1 resolved
Quality: Demo-only @ignore annotations disable ruling tests

📄 its/ruling/src/test/java/org/sonar/java/it/JavaRulingTest.java:193 📄 its/ruling/src/test/java/org/sonar/java/it/JavaRulingTest.java:206 📄 its/ruling/src/test/java/org/sonar/java/it/JavaRulingTest.java:233 📄 its/ruling/src/test/java/org/sonar/java/it/JavaRulingTest.java:353 📄 its/ruling/src/test/java/org/sonar/java/it/JavaRulingTest.java:363 📄 its/ruling/src/test/java/org/sonar/java/it/JavaRulingTest.java:383 📄 its/ruling/src/test/java/org/sonar/java/it/JavaRulingTest.java:395
This commit adds @Ignore("Excluded to speed up CI for PR demo") to seven ruling tests (spring_mall, guava, eclipse_jetty_incremental, sonarqube_server, jboss_ejb3_tutorial, regex_examples, vibebot). Combined with the sibling commit "DO NOT MERGE Introduce some diff for testing" (which modifies java-S1134.json expectations), these are clearly temporary changes to exercise the new ruling-diff-comment workflow. If merged, they would silently disable a large portion of ruling coverage, letting analyzer regressions go undetected. Before this leaves draft state, revert both the @Ignore annotations and the java-S1134.json edits so only the actual CI workflow change ships. The PR is currently a draft, so this is a merge-readiness gate rather than an inherent code defect.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@romainbrenguier romainbrenguier requested a review from rombirli July 10, 2026 12:45
@romainbrenguier romainbrenguier marked this pull request as ready for review July 10, 2026 12:45
@sonarqube-next

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant