Skip to content

SONARPY-4541 Provide reusable Python test-file heuristic - #2299

Draft
erwan-leforestier-sonarsource wants to merge 1 commit into
masterfrom
feat/sonarpy-4541-classify-test-files
Draft

SONARPY-4541 Provide reusable Python test-file heuristic#2299
erwan-leforestier-sonarsource wants to merge 1 commit into
masterfrom
feat/sonarpy-4541-classify-test-files

Conversation

@erwan-leforestier-sonarsource

Copy link
Copy Markdown

Summary

Adds a cached, aggressive test-content classifier that Python rules can query from both visitor APIs. The scanner reuses that result when determining rule scope for MAIN-classified files.

Changes

  • Recognize test-oriented paths, filenames, imports, classes, and function ratios from the top-level AST.
  • Expose isLikelyTestFile() through visitor and subscription contexts.
  • Preserve project-relative classification and cached effective file types.

Functional Validation

Artifact: SONARPY-4541-fv.zip

Once the file is attached to the PR description, unzip and run:
./run.sh

Expected output:

******************* MASTER *******************
Analyzing "framework_helper.py"...
Results:
    - Rule "S1226" -> L.4

****** Branch "feat/sonarpy-4541-classify-test-files" ******
Analyzing "framework_helper.py"...
Results:
    (none)

⚠️⚠️ This is not ready for review ⚠️⚠️

Comment on lines +56 to +60
/**
* Reports whether the current file is likely to contain test code.
* @return whether the file is likely test content
*/
boolean isLikelyTestFile();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Quality: New abstract method on public API SubscriptionContext breaks compat

isLikelyTestFile() is added as a non-default abstract method to the public API interface SubscriptionContext (in the org.sonar.plugins.python.api package). Any external implementation of this interface (e.g. test doubles or third-party integrations relying on the published API) will fail to compile / break binary compatibility. Provide a default implementation (e.g. default boolean isLikelyTestFile() { return false; }) to preserve backward compatibility while SubscriptionContextImpl overrides it.

Make the new method a default method so existing implementers of the public API keep compiling.:

/**
 * Reports whether the current file is likely to contain test code.
 * @return whether the file is likely test content
 */
default boolean isLikelyTestFile() {
  return false;
}
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

Comment on lines +275 to +278
public Builder testFilePath(String testFilePath) {
this.testFilePath = Optional.of(testFilePath);
return this;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Bug: Builder.testFilePath(null) throws NPE via Optional.of

testFilePath(String) wraps the argument with Optional.of(testFilePath), which throws NullPointerException if null is passed. The rest of the classifier explicitly supports a null path (the constructor falls back to the PythonFile-based lookup), so this method is inconsistent and brittle for callers other than PythonScanner. Use Optional.ofNullable(testFilePath) to keep the intended null-handling contract.

Avoid NPE and align with the null-tolerant classifier API.:

public Builder testFilePath(String testFilePath) {
  this.testFilePath = Optional.ofNullable(testFilePath);
  return this;
}
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown
Code Review ⚠️ Changes requested 0 resolved / 2 findings

Adds a cached test-content classifier for Python rules and exposes it via visitor APIs, but introduces a breaking change by adding a non-default abstract method to the public SubscriptionContext interface and potential NPE risk in Builder.testFilePath().

⚠️ Quality: New abstract method on public API SubscriptionContext breaks compat

📄 python-frontend/src/main/java/org/sonar/plugins/python/api/SubscriptionContext.java:56-60

isLikelyTestFile() is added as a non-default abstract method to the public API interface SubscriptionContext (in the org.sonar.plugins.python.api package). Any external implementation of this interface (e.g. test doubles or third-party integrations relying on the published API) will fail to compile / break binary compatibility. Provide a default implementation (e.g. default boolean isLikelyTestFile() { return false; }) to preserve backward compatibility while SubscriptionContextImpl overrides it.

Make the new method a default method so existing implementers of the public API keep compiling.
/**
 * Reports whether the current file is likely to contain test code.
 * @return whether the file is likely test content
 */
default boolean isLikelyTestFile() {
  return false;
}
💡 Bug: Builder.testFilePath(null) throws NPE via Optional.of

📄 python-frontend/src/main/java/org/sonar/plugins/python/api/PythonVisitorContext.java:275-278

testFilePath(String) wraps the argument with Optional.of(testFilePath), which throws NullPointerException if null is passed. The rest of the classifier explicitly supports a null path (the constructor falls back to the PythonFile-based lookup), so this method is inconsistent and brittle for callers other than PythonScanner. Use Optional.ofNullable(testFilePath) to keep the intended null-handling contract.

Avoid NPE and align with the null-tolerant classifier API.
public Builder testFilePath(String testFilePath) {
  this.testFilePath = Optional.ofNullable(testFilePath);
  return this;
}
🤖 Prompt for agents
Code Review: Adds a cached test-content classifier for Python rules and exposes it via visitor APIs, but introduces a breaking change by adding a non-default abstract method to the public SubscriptionContext interface and potential NPE risk in Builder.testFilePath().

1. ⚠️ Quality: New abstract method on public API SubscriptionContext breaks compat
   Files: python-frontend/src/main/java/org/sonar/plugins/python/api/SubscriptionContext.java:56-60

   `isLikelyTestFile()` is added as a non-default abstract method to the public API interface `SubscriptionContext` (in the `org.sonar.plugins.python.api` package). Any external implementation of this interface (e.g. test doubles or third-party integrations relying on the published API) will fail to compile / break binary compatibility. Provide a `default` implementation (e.g. `default boolean isLikelyTestFile() { return false; }`) to preserve backward compatibility while `SubscriptionContextImpl` overrides it.

   Fix (Make the new method a default method so existing implementers of the public API keep compiling.):
   /**
    * Reports whether the current file is likely to contain test code.
    * @return whether the file is likely test content
    */
   default boolean isLikelyTestFile() {
     return false;
   }

2. 💡 Bug: Builder.testFilePath(null) throws NPE via Optional.of
   Files: python-frontend/src/main/java/org/sonar/plugins/python/api/PythonVisitorContext.java:275-278

   `testFilePath(String)` wraps the argument with `Optional.of(testFilePath)`, which throws NullPointerException if null is passed. The rest of the classifier explicitly supports a null path (the constructor falls back to the PythonFile-based lookup), so this method is inconsistent and brittle for callers other than PythonScanner. Use `Optional.ofNullable(testFilePath)` to keep the intended null-handling contract.

   Fix (Avoid NPE and align with the null-tolerant classifier API.):
   public Builder testFilePath(String testFilePath) {
     this.testFilePath = Optional.ofNullable(testFilePath);
     return this;
   }

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.
Unblock → Override a blocking verdict and allow merging.

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

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

Was this helpful? React with 👍 / 👎 | Gitar

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