Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

report aggregation process implemented for multi-project #243

Merged
merged 4 commits into from Mar 4, 2021

Conversation

MikeSafonov
Copy link
Contributor

Greetings
I have implemented a report aggregation process using pitest-aggregator

Copy link
Owner

@szpak szpak left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

See the separate remarks.

build.gradle Outdated
@@ -34,6 +34,7 @@ sourceSets {

dependencies {
implementation localGroovy()
implementation "org.pitest:pitest-aggregator:1.5.2"
Copy link
Owner

Choose a reason for hiding this comment

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

It is somehow problematic. PIT allows to dynamically change used PIT version and it might be not compatible with agregator's 1.5.2. I have to think if (and how) it could be made up.

Copy link
Owner

Choose a reason for hiding this comment

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

I have some idea, but please make the functional tests green to allow me to test it.

@MikeSafonov
Copy link
Contributor Author

Hello. Any updates?

@szpak
Copy link
Owner

szpak commented Dec 12, 2020

Hello. Any updates?

Unfortunately, I was busy with some other activities. Nevertheless, I will try to handle that "soon".

@szpak
Copy link
Owner

szpak commented Dec 12, 2020

I wanted to work on it, however, it still fails for me locally with commit 140af32 (your master):

info.solidsoft.gradle.pitest.functional.AcceptanceTestsInSeparateSubprojectFunctionalSpec > should aggregate report from subproject FAILED
    java.lang.IllegalArgumentException: Mutable Project State warnings were found (Set the ignoreMutableProjectStateWarnings system property during the test to ignore):
     - The configuration :for-report:testRuntimeClasspath was resolved without accessing the project in a safe manner.  This may happen when a configuration is resolved from a different project. This behaviour has been deprecated and is scheduled to be removed in Gradle 7.0. See https://docs.gradle.org/6.6/userguide/viewing_debugging_dependencies.html#sub:resolving-unsafe-configuration-resolution-errors for more details.
     - The configuration :shared:testRuntimeClasspath was resolved without accessing the project in a safe manner.  This may happen when a configuration is resolved from a different project. This behaviour has been deprecated and is scheduled to be removed in Gradle 7.0. See https://docs.gradle.org/6.6/userguide/viewing_debugging_dependencies.html#sub:resolving-unsafe-configuration-resolution-errors for more details.
        at nebula.test.IntegrationBase$Trait$Helper.checkForMutableProjectState(IntegrationBase.groovy:114)
        at nebula.test.Integration$Trait$Helper.checkForDeprecations(Integration.groovy:171)
        at nebula.test.Integration$Trait$Helper.runTasks(Integration.groovy:167)
        at nebula.test.Integration$Trait$Helper.runTasksSuccessfully(Integration.groovy:148)
        at info.solidsoft.gradle.pitest.functional.AcceptanceTestsInSeparateSubprojectFunctionalSpec.should aggregate report from subproject(AcceptanceTestsInSeparateSubprojectFunctionalSpec.groovy:31)

21 tests completed, 1 failed, 3 skipped

When I ignored the deprecation warnings it started to fail with:

                Caused by:
                java.lang.IllegalStateException: Failed to build: no lineCoverageFiles have been set
                    at org.pitest.aggregate.ReportAggregator$Builder.validateState(ReportAggregator.java:226)
                    at org.pitest.aggregate.ReportAggregator$Builder.build(ReportAggregator.java:214)
                    at info.solidsoft.gradle.pitest.report.PitestReportAggregator.aggregate(PitestReportAggregator.groovy:38)

For some reasons the CI builds pass. I made a dummy commit in your branch to make sure Travis builds from the same commit, but it stuck (most likely due to that).

Does ./gradlew check funcTest work for you locally? What OS and Java version do you use (I checked with Java 15, 11 and 8 on Linux)?

Update. You seem to migrate already to travis.com and that test passed :-/
(the fail on upload is related to my CD mechanism and you may ignore it - I will apply a workaround before the next version is released)

@szpak
Copy link
Owner

szpak commented Dec 12, 2020

I'm not sure why it was happening only on my machine, but both "no lineCoverageFiles" and deprecation warnings were caused by the fact that in functional tests pitestReportAggregate was executed before the regular pitest tasks. In that situation, there were not files with line coverage which was generating the errors. I added mustRunAfter (mysteriously shouldRunAfter was not enough 🤔 ) which fixed the test.

That brought me to the conslusion that AggregateReportTask has no inputs and outputs, it will be always not up-to-date (even there are not changes in previously generated by pitest tasks reports). Woudn't be possible to try to expose them as fields in AggregateReportTask?

Unfortunately, solving that took a lot of time, so I will experiment with avoiding hardcoded versions of runtime dependencies another time :-/.

@MikeSafonov
Copy link
Contributor Author

Greetings. I`ve revorked AggregateReportTask to use Output and Intput

@szpak
Copy link
Owner

szpak commented Jan 20, 2021

My initial approach with compileOnly dependency (and runtime hacks) failed. I asked for other suggestions on their forum.

}

private boolean isRootPitestConfigured() {
return project.plugins.hasPlugin(PitestPlugin.PLUGIN_ID) && project.extensions.findByType(PitestPluginExtension)
Copy link
Owner

@szpak szpak Feb 28, 2021

Choose a reason for hiding this comment

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

I'm back to that topic and it looks quite promising :-). However, adjusting the code, I wonder if there is a reason why you for the root project check for both plugin and its extension, but for subproject you only check for the plugin existence?

Extension should be applied automatically when the plugin is applied, so maybe it is superfluous?

(please do not commit any changes, I'm working on dependencies which I plan to push to that branch once ready)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extension should be applied automatically when the plugin is applied, so maybe it is superfluous?

You are completely right.

@szpak
Copy link
Owner

szpak commented Mar 1, 2021

With a help from @marcphilipp, I was able to retain the PIT version configuration at runtime capability using the Worker API.

@MikeSafonov I had to rework your PR a little bit. Please take a look if it looks ok for you (the last commit). I will write some more tests tomorrow and I plan to merge it soon. Feel free to make new commits if needed, but please do not rebase this branch.

@MikeSafonov
Copy link
Contributor Author

With a help from @marcphilipp, I was able to retain the PIT version configuration at runtime capability using the Worker API.

@MikeSafonov I had to rework your PR a little bit. Please take a look if it looks ok for you (the last commit). I will write some more tests tomorrow and I plan to merge it soon. Feel free to make new commits if needed, but please do not rebase this branch.

Congratulations! I have already looked at your changes and they looks good to me.
Looking forward to the new version of the plugin!

@szpak
Copy link
Owner

szpak commented Mar 1, 2021

I've merged it manually into devel and having some other (unrelated) changes implemented I will release 1.6.0 "soon". Thank you for your contribution!

@szpak szpak added this to the 1.6.0 milestone Mar 3, 2021
@szpak szpak merged commit 746313c into szpak:master Mar 4, 2021
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.

None yet

2 participants