Skip to content

refactor(pitest-aggregator): replace cross-project access with variant-aware producer/consumer configurations#395

Open
luisgomez29 wants to merge 6 commits into
szpak:masterfrom
luisgomez29:fix-pitest-report-aggregate
Open

refactor(pitest-aggregator): replace cross-project access with variant-aware producer/consumer configurations#395
luisgomez29 wants to merge 6 commits into
szpak:masterfrom
luisgomez29:fix-pitest-report-aggregate

Conversation

@luisgomez29
Copy link
Copy Markdown
Contributor

@luisgomez29 luisgomez29 commented Jan 29, 2026

This PR refactors the pitest-aggregator plugin to support Gradle 9 and Configuration Cache requirements. It replaces the legacy cross-project model access (which broke project isolation) with Gradle's recommended Variant-Aware Dependency Management.

Changes

1. Configuration Cache Compliance

2. Variant-Aware Architecture (Producer-Consumer)

  • Producer (PitestPlugin): Now exposes three distinct outgoing configurations with specific attributes:

    • pitestReportElements: Exposes mutations.xml and linecoverage.xml.
    • pitestSourcesElements: Exposes source code directories.
    • pitestClassesElements: Exposes compiled class directories.
  • Consumer (PitestAggregatorPlugin): Uses ArtifactView for artifact consumption with variant selection:

    • Filters project components using ProjectComponentIdentifier.
    • Defines selection attributes: Category.VERIFICATION, Usage.pitest, and specific artifact types.
    • Lazily resolves mutation files, line coverage files, sources, and classes.
    • Aggregates artifacts from the root project and all subprojects with both pitest and java plugins applied.
  • Shared Constants: Introduced PitestAttributes to manage shared attribute keys and values, ensuring type safety between plugins.

3. Report Generation Fixes

  • Added org.pitest:pitest-html-report as a dependency in the aggregator. This fixes an issue where aggregated reports in multi-module projects were missing CSS styles and HTML resources.
  • Cleaned up the AggregateReportTask correctly using Provider conventions for lazy property evaluation.

I’ve tested the changes, and they resolve the configuration‑cache serialization error. Builds now work correctly with the configuration cache enabled.

@szpak @Vampire please review the proposed changes.

@luisgomez29 luisgomez29 marked this pull request as ready for review January 29, 2026 21:37
@Vampire
Copy link
Copy Markdown
Contributor

Vampire commented Jan 30, 2026

I have no idea.
The aggregate part is a big pile of highly discouraged bad practice that is quire fragile.
Latest with isolated projects it will not work anymore at all.
If you ask me, the whole thing should be rewritten to do the aggregation properly without using evil cross-project model access but proper aggregation logic like for example the testreport aggregation plugin and jacoco aggregation plugin do, then you also don't need spurious cross-project must-run-afters and similar hacks.

@luisgomez29 luisgomez29 changed the title fix: improve Pitest report classpath aggregation refactor(pitest-aggregator): replace cross-project access with variant-aware producer/consumer configurations Feb 2, 2026
@luisgomez29
Copy link
Copy Markdown
Contributor Author

I have no idea. The aggregate part is a big pile of highly discouraged bad practice that is quire fragile. Latest with isolated projects it will not work anymore at all. If you ask me, the whole thing should be rewritten to do the aggregation properly without using evil cross-project model access but proper aggregation logic like for example the testreport aggregation plugin and jacoco aggregation plugin do, then you also don't need spurious cross-project must-run-afters and similar hacks.

Thanks @Vampire! I’ve updated the aggregator to remove the cross‑project model access and rewrote it using proper variant‑aware aggregation, similar to what the test-report and Jacoco aggregation plugins do. Please review the latest changes.

@Vampire
Copy link
Copy Markdown
Contributor

Vampire commented Feb 2, 2026

I’ve updated the aggregator to remove the cross‑project model access and rewrote it using proper variant‑aware aggregation, similar to what the test-report and Jacoco aggregation plugins do.

Nice

Please review the latest changes.

I'm sorry, but I don't currently have the capacity to review such a change in a project that I do not maintain and in a functionality that I do not use.

Just one note as I specifically searched for it.
You most probably do not want a lenient artifact view.
Artifact views are "lenient" by default in the sense that it ignores the dependencies that do not provide a matching variant (directly or through artifact transforms).
Making an artifact view lenient means that almost any problems are ignored, including download problems or what-not and is seldomly what you want.

@luisgomez29
Copy link
Copy Markdown
Contributor Author

Hi @szpak, I have already applied the changes based on the comments provided. Please review the changes.

@Vampire
Copy link
Copy Markdown
Contributor

Vampire commented Feb 2, 2026

Not really, you added a rectifying comment instead of making it non-lenient.
The problem is, that you create three configurations each with the attributes and this fails if the respective variant is not found.
Better do it like the examples I told you.
There should be one configuration where you define the dependencies that should be aggregated, and from that you create three artifact views that request the respective set of attributes, also requesting variant reselection, and most probably you should also add a componentFilter that only looks at project dependencies, not at external dependencies.
Then there should be no need for lenient which is just wrong.

You should probably also not just fixedly depend on all subprojects, but have that at most as defaultDependencies in the one configuration you create.
Then a user can define other projects to aggregate instead.
For example if he only wants to aggregate a subset of the projects, or if he has a sibling project for the aggregation instead of the root project.
Also, if someone has pitest in the root project and in subprojects, currently you would only aggregate the subprojects and leave out the rootproject's.

@luisgomez29
Copy link
Copy Markdown
Contributor Author

Hi @Vampire, I have already applied the changes based on the comments provided. Please review the changes.

@Vampire
Copy link
Copy Markdown
Contributor

Vampire commented Feb 15, 2026

Still, I don't have time for a full review.

But regarding the aggregation logic I had another cursory look, you still don't really do what I suggested.
In the latest code, you check project.plugins.hasPlugin which is bad in multiple ways.

  • you should not use project.plugins (look at its JavaDoc)
  • you should not check in any way whether some other plugin is applied, as that then requires that you apply plugins in a certain order which is bad, use pluginsManager.withPlugin instead if you want to react to plugins being applied
  • you still (or again) reach into the model of subprojects by checking what plugins are applied there, which is bad because of the two points I just mentioned, and additionally because you reach into the subproject model which you must not do, there even the withPlugin would be bad as it again would be cross-project configuration. As you use artifact views now, this should also not be necessary, as long as the variant declared in the actual configuration on which you do the artifact views can be resolved properly. And if this is not the case, the user can any time configure the proper dependencies now that you use default dependencies.

Also findPluginExtension().ifPresent seems to be a bad idea, also that requires a certain plugin application order.
If you always need that extension added by that plugin, apply the plugin before you use the extension.
If you want to react to the plugin being applied also, use pluginManager.withPlugin then you know the plugin was applied and thus the extension will be present.

Btw. you should not create configurations but register them like for any other domain object collection, especially as Gradle made the configuration largely treated lazily recently.

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.

2 participants