geode icon indicating copy to clipboard operation
geode copied to clipboard

[GEODE-10517] Execution Optimization Enablement via Explicit Cross‑Module Test Result Dependency Modeling

Open JinwooHwang opened this issue 3 months ago • 5 comments

Summary

This change restores and stabilizes Gradle execution optimizations for the aggregated test reporting workflow by explicitly declaring all upstream Test tasks as both inputs and dependencies of the :combineReports TestReport task. It eliminates the prior Gradle validation warning caused by implicit consumption of test result outputs (notably from :geode-old-versions:test) and removes a redundant late graph mutation hook.

Background (Problem)

Gradle emitted a validation warning of the form: Task ':combineReports' uses the output of task ':geode-old-versions:test' without declaring an explicit or implicit dependency. Execution optimizations have been disabled.

Implications

  • Execution optimization for that unit was disabled ( reduced scheduling efficiency / potential loss of incremental behavior)
  • Potential nondeterministic ordering if task scheduling shifted
  • Noise in local and CI build logs

Root Cause

combineReports read binary test result directories generated by multiple subproject Test tasks without declaring a formal task dependency edge and input relationship. Gradle 7+ detects this implicit file coupling and defensively disables optimizations.

What Changed

  1. The combineReports task now:

    • Collects all subproject Test tasks.
    • Ensures explicit inclusion of :geode-old-versions:test.
    • Declares:
      • reportOn allTests (inputs)
      • dependsOn allTests (ordering)
    • Emits a location message after completion.
  2. Removed the redundant trailing gradle.taskGraph.whenReady { ... } block that previously re-applied the same reportOn / dependsOn wiring (no functional value, added noise).

  3. Retained the existing whenReady block.

Outcome

  • Gradle validation warning eliminated.
  • Execution optimizations re-enabled for :combineReports.
  • Report aggregation remains deterministic and complete.
  • Combined HTML report continues to appear at: build/reports/combined.

Verification (Performed / Expected)

  • Run ./gradlew clean build
  • Validate
    • No implicit dependency warning in console output.
    • :geode-old-versions:test executes before :combineReports.
    • All module test results appear in the combined report.
    • No new warnings introduced.

Risk Assessment

  • Low Risk: Change is purely declarative build graph augmentation; no alterations to test logic, filtering, or artifact paths.

Reviewer Checklist

  • [ ] No implicit dependency warnings in build output
  • [ ] combineReports defined only once; no duplicate wiring hook
  • [ ] Report generated at build/reports/combined
  • [ ] Old versions module results included
  • [ ] No unrelated build regressions

For all changes:

  • [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • [x] Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • [x] Is your initial contribution a single, squashed commit?

  • [x] Does gradlew build run cleanly?

  • [ ] Have you written or updated unit tests to verify your changes?

  • [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

JinwooHwang avatar Sep 12 '25 18:09 JinwooHwang

@raboof, all 17 checks passed without issues. Let me know if anything needs clarification or if you'd like to review together. Huge thanks for going through all the PRs. Your effort made a real difference, and we wouldn’t be where we are without your help. Much appreciated!

JinwooHwang avatar Sep 13 '25 11:09 JinwooHwang

Hi @raboof. Please don’t hesitate to reach out if anything looks unclear or if you'd prefer to walk through any part of the PR together. I really appreciate the time and care you’ve put into reviewing everything. Your support has been instrumental in getting us this far. Looking forward to your thoughts whenever you're ready.

JinwooHwang avatar Sep 16 '25 12:09 JinwooHwang

While we wait for @raboof to weigh in, perhaps this is something @wmh1108-sas could also review?

semioticrobotic avatar Oct 21 '25 18:10 semioticrobotic

Sorry, my Gradle knowledge isn't really sufficient to meaningfully review this change. It looks fairly low-risk, though...

raboof avatar Oct 21 '25 23:10 raboof

No worries. Thank you very much @raboof

JinwooHwang avatar Oct 21 '25 23:10 JinwooHwang