Repository navigation
Fix test tasks that pass without running any tests - #13274
PerfectSlayer wants to merge 2 commits into
Conversation
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
a109a8d to
d75ed4f
Compare
There was a problem hiding this comment.
More details
The empty-task exceptions are consistent with the repository’s forked-test and abstract-base conventions. Whether the revived tasks execute successfully remains unverified because Gradle tests were not run.
🤖 Bits Code Review · Commit a109a8d
jandro996
left a comment
There was a problem hiding this comment.
It seems that the testRuntimeActivation tasks of springboot-security and datastreams/kafkaschemaregistry still run nothing after this change. Neither module has a src/test source set, so Gradle skips both tasks as NO-SOURCE (@SkipWhenEmpty on the candidate class files) before failOnNoDiscoveredTests is ever evaluated, and they keep passing silently. The description lists them among the revived tasks, and APMLP-2186 has "the two smoke apps without tests either get tests or are removed" in its done criteria. APMLP-2199 covers springboot-security (item 4), but I could not find anything tracking kafkaschemaregistry, whose task does not even set an AppSec mode and looks like a copy of the regular test task. Would it make sense to drop those two tasks here, or at least adjust the description and track kafkaschemaregistry in a follow-up? I might be missing something, what do you think?
jandro996
left a comment
There was a problem hiding this comment.
LGTM! Just left some comments to track with APMLP-2199
I'm currently investigating if it can be dropped or not (as covered by instrumentation tests). |
Run every Test task on the JUnit Platform, not only test suites: tasks registered with tasks.register(..., Test) fell back to JUnit 4 since the Gradle 8.1 upgrade and discovered none of the Spock 2 or JUnit 5 tests. Fail a test task that discovers no tests again, except for the non-forked peer of a suite whose concrete tests are all *ForkedTest classes, and for three test source sets that only hold shared abstract bases. Skip the springboot-graphql detection feature in testRuntimeActivation, which loads AppSec without ever activating it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d75ed4f to
221053e
Compare
The testRuntimeActivation task of spring-tomcat7 was copied from springboot's build file. Its spec is IAST-only and never applies the inactive AppSec flag, so the task reran the test task unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
221053e to
c6071d7
Compare
What Does This Do
Runs every Gradle
Testtask on the JUnit Platform and makes a test task fail when it discovers no tests. A misconfigured test task can no longer pass silently.This brings back test tasks that had never run a test since their creation:
scala-2.10.7(34 tests each);testRuntimeActivationtask ofspringboot-graphql. Its attack-detection feature is skipped for now, because nothing activates AppSec in that mode (APMLP-2199).It also removes
spring-tomcat7'stestRuntimeActivationtask, which was a copy ofspringboot's that only reran thetesttask.Motivation
Since the Gradle 8.1 upgrade (April 2023), only test suites get the JUnit Platform. A task registered with
tasks.register(..., Test)falls back to JUnit 4. It then discovers none of the Spock 2 or JUnit 5 tests and still passes. The Gradle 9.5.1 bump would have surfaced this, but it addedfailOnNoDiscoveredTests = false, which hid it. CI Visibility reports zero executions for these tasks on master.Additional Notes
*ForkedTestclasses is allowed to be empty, because it only hosts abstract bases. The check reads the compiled classes at execution time.testsource set only holds shared abstract bases opt out explicitly.-PrunFlakyTests) and--testsruns are exempt on Gradle's side.testRuntimeActivation: onlyspringboot's task was designed for runtime activation (2022). Since Upgrade libddwaf java to 14.0.0 #8654, AppSec only loads its rules on a real activation through remote config, so the app-side reflective activation no longer works. The other modules copied the task without any activation:springboot-graphqlkeeps its task. Its detection feature is skipped in that mode until the AppSec team adds a real activation through remote config (APMLP-2199).spring-tomcat7's spec is IAST-only and never applies the inactive-AppSec flag, so its task rerantestunchanged. It is removed.springboot-security) and APMLP-2205 (kafkaschemaregistry).@FlakyJUnit tests would now fail under-PskipFlakyTests, sinceFlakyJUnitFilterremoves them during discovery. No task is in that situation today.Jira ticket: APMLP-2186
🤖 Generated with Claude Code