-
Notifications
You must be signed in to change notification settings - Fork 58
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
Support java-test-fixtures plugin #223
Conversation
Builds failing on Travis seems to be victims of the unstable build. My PR passes on Travis for Java 11 and 14. Tests pass when I run them locally with Java 8. AppVeyor build with Java 8 passes. |
There was a problem hiding this 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.
The build fails only with Java 8, as PIT 1.1.5 is not treated as Java 11 compatible and:
PitestPluginPitVersionFunctionalSpec.setup and run pitest task with PIT 1.1.5 FAILED
isn't executed in the other builds. As PIT 1.4.0 was the one to required Java 8, feel free to remove any older PIT versions from the verification matrix.
I put some comment/questions/suggestions inline in code.
src/funcTest/groovy/info/solidsoft/gradle/pitest/functional/Junit5FunctionalSpec.groovy
Show resolved
Hide resolved
@@ -21,8 +23,10 @@ class Junit5FunctionalSpec extends AbstractPitestFunctionalSpec { | |||
given: | |||
copyResources("testProjects/junit5kotlin", "") | |||
when: | |||
ExecutionResult result = runTasksSuccessfully('pitest', '-b', 'build.gradle.kts') | |||
ExecutionResult result = runTasks('pitest') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-paste? You probably would like to build using the kotlin build file :).
src/funcTest/groovy/info/solidsoft/gradle/pitest/functional/Junit5FunctionalSpec.groovy
Show resolved
Hide resolved
@CompileDynamic | ||
class TestFixturesFunctionalSpec extends AbstractPitestFunctionalSpec { | ||
|
||
void "should work with kotlin and test fixtures"() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kotlin and test fixtures
? That project seem to have classes written in Java and build file in Groovy :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a copy-paste mistake, I will fix it.
task.additionalClasspath.setFrom({ | ||
List<FileCollection> testRuntimeClasspath = (extension.testSourceSets.get() as Set<SourceSet>)*.runtimeClasspath | ||
FileCollection combinedTaskClasspath = project.objects.fileCollection().from(testRuntimeClasspath) | ||
FileCollection filteredCombinedTaskClasspath = combinedTaskClasspath.filter { File file -> | ||
!extension.fileExtensionsToFilter.getOrElse([]).find { extension -> file.name.endsWith(".$extension") } | ||
} | ||
!extension.fileExtensionsToFilter.getOrElse([]).find { extension -> file.name.endsWith(".$extension") || file == "core.jar" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why core.jar
is special?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to remove it after debugging a local project.
return [new File(project.buildDir, "classes//java//${sourceSetName}"), | ||
new File(project.buildDir, "resources//${sourceSetName}") | ||
return [new File(project.buildDir, "classes/java/${sourceSetName}"), | ||
new File(project.buildDir, "resources/${sourceSetName}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that I had some problems (somewhere) with just a single /
on Windows and //
was fine on both *Unix and Windows. However, the tests passed on AppVeyor, so it doesn't seem to be a problem here...
task.taskArgumentMap()['classPath'] == | ||
( | ||
assembleSourceSetsClasspathByNameAsStringSet("intTest") + | ||
[new File(project.buildDir, "classes/java/main").absolutePath] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder that it started to be needed here. classpath
didn't contain have classes/java/main
before, assuming java
plugin was applied? Or it was a limitation of this "unit" test (and in real execution it was included)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intTest
source set is created from scratch and does not have any relation to main
source set, so intTest
classpath does not include any classes from main
. I suppose previously such a project won't work with real Pitest, because Pitest would fail to find classes to mutate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what confused me.
extension.mainSourceSets.set([javaSourceSets.getByName(SourceSet.MAIN_SOURCE_SET_NAME)])
and:
private Set<File> calculateBaseMutableCodePaths() {
Set<SourceSet> sourceSets = extension.mainSourceSets.get()
return sourceSets*.output.classesDirs.files.flatten() as Set<File>
}
which you call should cover it. However, in the past it was used only for mutableCodePaths
, not for (additional)classpath
, what you fixed making it java-test-fixtures plugin compatible :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, could you, making other changes, refactor assembleSourceSetsClasspathByNameAsStringSet
to generate class only or class+resources classpath to keep new File(project.buildDir, "classes/java/XXX")
in one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you wanted, but I changed the path handling.
Please check and close other discussions, I think I addressed all your comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of :). I will tune it up after merge.
Due to integration test failures with recent Java 8 versions: #223 (review)
Building. Feel free to rebase once it's green. |
Applying 'java-test-fixtures' plugin caused 'No mutations found' from Pitest. The plugin changes test runtime classpath: tested code is included as a JAR instead of as a directory (see gradle/gradle#11696). It seems that Pitest ignores correct directories being passed using --mutableCodePaths and insists on having those directories also passed in --classPath. Also change functional tests: runTasksSuccessfully() fails the test without giving any context, so it should be avoided.
b3ec5df
to
608e731
Compare
Is there something this PR is waiting for to be merged? |
Nope, I accepted it and it's waiting for a longer moment of time to adjust the last commit after merge :). |
Can you release this bugfix and refactor tests later? There is no workaround for this problem. |
Thanks for a reminder. 1.5.2 is just being released on Travis: |
Thank you! |
Applying 'java-test-fixtures' plugin caused 'No mutations found' from Pitest.
The plugin changes test runtime classpath: tested code is included as a JAR instead of as a directory
(see gradle/gradle#11696). It seems that Pitest ignores correct directories being passed using --mutableCodePaths and insists on having those directories also passed in --classPath.
Also change functional tests: runTasksSuccessfully() fails the test without giving any context, so it should be avoided.
The project I work on uses java-test-fixtures plugin, so this issue blocks me from using Pitest.