Spotless: Problems with SpecificFilesTest

[t-rad679]

If you are submitting a bug, please include the following:

• [ ] summary of problem

Three things:

• On Java 13, SpecificFilesTest fails with IllegalArgumentException. See logs
• The test passes with Java 8, yet, when I copy the config shown in the test verbatim I have to fix a few things for the test:
  
  
  
  • I have to add the version number to the plugin import to make it run at all
  
  
  • I have to add the target field to the Spotless config to make it lint any files
  
  

• The test passes with Java 8, but adding -PspotlessFiles=Test.java does not lint Test.java. See the output listed in this comment on another issue. This was attempted both on Java 8 and Java 13. Both produce the same result.

• [ ] gradle or maven version

$ gradle --version

------------------------------------------------------------
Gradle 6.2
------------------------------------------------------------

Build time:   2020-02-17 08:32:01 UTC
Revision:     61d3320259a1a0d31519bf208eb13741679a742f

Kotlin:       1.3.61
Groovy:       2.5.8
Ant:          Apache Ant(TM) version 1.10.7 compiled on September 1 2019
JVM:          13.0.1 (Oracle Corporation 13.0.1+9)
OS:           Mac OS X 10.15.3 x86_64

• [ ] spotless version
  3.27.1
• [ ] operating system and version
  MacOSX 10.15.3
• [ ] copy-paste your full Spotless configuration block(s), and a link to a public git repo that reproduces the problem if possible

buildscript { repositories { mavenCentral() } }
plugins {
    id 'com.diffplug.gradle.spotless' version "3.27.1"
}
apply plugin: 'java'
spotless {
    java {
        target '**/*.java'
        googleJavaFormat('1.2')
    }
}

• [ ] copy-paste the full content of any console errors emitted by gradlew spotless[Apply/Check] --stacktrace

No errors, which is unexpected. A lint error is expected see this comment

[t-rad679]

Note that all of this was initially tried on Gradle 6.0.1. It didn't work then either.

[nedtwigg]

On Java 13, SpecificFilesTest fails with IllegalArgumentException

The stacktrace indicates that Gradle itself does not support Java 13. It's different to support compiling code for 13, vs running the build on 13.

Caused by: java.lang.IllegalArgumentException: Could not determine java version from '13.0.1'.
    at org.gradle.api.JavaVersion.toVersion(JavaVersion.java:68)
    at org.gradle.api.JavaVersion.current(JavaVersion.java:78)

when I copy the config shown in the test verbatim I have to fix a few things for the test:
I have to add the version number to the plugin import to make it run at all

This is expected. The Gradle test infrastructure uses the code under test, which is why you can leave out the version number within the test.

I have to add the target field to the Spotless config to make it lint any files

Spotless uses the default java source set, which is src/main/java etc. So you shouldn't need to add a target if it's in a default location, but it is expected that you would need to have a target if apply plugin: java wouldn't pickup the file you are testing.

https://github.com/diffplug/spotless/blob/c007dbac060d5dd413b252a6fcf98ab2784f71de/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JavaExtension.java#L178-L188

Summary

It looks like you made a lone test project to replicate SpecificFilesTest, and somehow it's not doing in the project what it looks to be doing in the test, correct?

One way to make your test more similar is to put the file at src/main/java/test0.java, and remove the custom target.

https://github.com/diffplug/spotless/blob/c007dbac060d5dd413b252a6fcf98ab2784f71de/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/SpecificFilesTest.java#L24

[t-rad679]

Thanks for the explanation on those. I'll file a ticket with Gradle to properly support Java 13, though it works in most other cases.

It looks like you made a lone test project to replicate SpecificFilesTest, and somehow it's not doing in the project what it looks to be doing in the test, correct?

No. Well, sort of. For the last bullet, I used the same project we talked about in the other thread, but I replaced the build.gradle with a replica of the one in the SpecificFilesTest...so I guess it amounts to a dummy project.

It seems like the test only passing when the test input file is in a particular location is a bug in the test, but I'll play around with it. I don't think Spotless should assume the files it's linting are in a particular place when using -PspotlessFiles, especially given that this isn't the case for general usage.

[nedtwigg]

I don't know what Gradle's policy is, but 13 is not an LTS release, and will reach EOL this March (in weeks!) when Java 14 comes out. In this new fast-JDK cadence, I would expect that most infrastructure projects will only invest in LTS releases. As you note, the inbetween releases mostly work, which is the point of releasing them so fast, but the tradeoff is that the entire ecosystem is probably not going to work on non-LTS releases.

Happy to merge expanded test coverage for SpecificFilesTest :)

[t-rad679]

I'm working on some changes to the tests, but I still think the last issue I mentioned is a bug. I understand that this isn't a core feature so the core maintainers might not fix it, but it does seem like the problem I encountered in the other issue (the last bullet under "summary of problem" above) is a bug with Spotless. Even if it's because my files are in unexpected places, that's definitely a case that -PspotlessFiles should handle. Perhaps it's some other environmental issue, but I can just resolve this as "Works As Intended" if that turns out to be the case. I think this should still be labeled bug.

When I'm finished, the PR I make for the tests will go to https://github.com/diffplug/spotless/issues/529.

[t-rad679]

Thanks! I'll keep this ticket updated if I figure anything out!

[t-rad679]

See comments https://github.com/diffplug/spotless/issues/529#issuecomment-588421298 to https://github.com/diffplug/spotless/issues/529#issuecomment-588431410 in https://github.com/diffplug/spotless/issues/529

TL;DR--
The issue is that spotlessFiles only works when absolute paths are specified.

[t-rad679]

I have confirmed that using an absolute path in spotlessFiles works in the super simplified case that matches the test. I pretty distinctly remember trying this out before on my more complicated config, but maybe my config was wrong or I made a mistake in specifying the path somehow.

If @nedtwigg and I can come up with an acceptable solution for checking relative and absolute paths, I will implement it. So I'd like to leave this ticket open.

[nedtwigg]

How hard is it to turn your relative-path inputs into absolute paths? Is it easier than changing this project, its tests, and documentation?

[t-rad679]

Ah! I wrote a response for this but apparently never hit the comment button!

Yeah, that's a good point. It would definitely be easier to use absolute paths, it just seems to me like supporting both options would be the ideal behavior; being restricted to just absolute paths is unintuitive and it seems unlikely that I'm the only person this will trip up. You're right that it's not worth the time to implement this feature, though perhaps it would make sense be clearer in the documentation that this is the case. I do think spotlessFiles will become significantly less useful when ratchetFrom is complete.

It is strange to me that only running spotless on changed files/a small subset of files appears to be such an uncommon use case. It seems like a formatter that only has the option to run on all source files at once would be unusable in almost all cases of introducing style conventions to an existing system. In my experience, incremental style compliance is the way to go in such a case...the only alternative is a massive CL/PR that touches every single file in the code base and you just hope that the formatter got every edge case correct.

Anyway, unless you feel it is going to clutter your GitHub, I'd like to leave this ticket open, label it as an enhancement, and maybe rename it to something like "Relative Paths for spotlessFiles". Or we could create a new one if it seems more appropriate. It'd be nice to do in case someone ever decides they want to tackle it (possibly myself), though I understand that allowing such cases frequently might make your issues page become harder to parse. Just let me know what you think!

[nedtwigg]

I'm gonna dupe this out in favor of #529 which I'll leave open for you, and for which I've changed to an enhancement for relative paths.

[t-rad679]

Sounds good. Thanks!

[t-rad679]

I realized that the ticket you linked is the tests ticket, which is not specifically about relative paths, after I responded. Can we reopen this ticket and redo the naming?

[nedtwigg]

FYI, -PspotlessFiles has been deprecated and will be removed. Migration path available here: https://github.com/diffplug/spotless/pull/602