Add Tests

[t-rad679]

fixes #529

Changes

Add tests for the following cases:

• Empty pattern passed to spotlessFiles
• A pattern that matches no files passed to spotlessFiles
• Invalid regexp passed to spotlessFiles
• Spotless is configured with a Kotlin build script

Also a little reorganization, and a change required to support Kotlin
build script was added to GradleIntegrationTest.java so that other tests
can make use of it.

I'd be more than willing to also fix the IntelliJ warnings (unused method, unnecessary throws, and access modifiers that can be more restrictive) in GradleIntegrationTest.java if you'd like. As a rule of thumb, I generally do incremental style fixes when making other changes to a file (in a separate commit, but the same PR. Happy to break this convention if you prefer).

Suggestion

Perhaps consider renaming GradleIntegrationTest. Maybe something like GradleIntegrationTestHarness would be a better name. In my experience, it's best practice for the names only of test classes to end in Test, rather than also doing so for testing infrastructure. For example, I sometimes will search the test source directory with the intent of only looking at testing infrastructure, rather than tests. I actually did so in writing this change. Sometimes people get around this by putting their test classes in a separate directory from testing infrastructure, though this does not appear to be the case for Spotless. This makes proper naming of testing infrastructure especially important. Even in other cases, I still see it as the cleaner naming convention. I'd be happy to make this change, if you'd like.

Question

When an empty pattern is passed to spotlessFiles, what should be the behavior? The test I've written currently assumes that the correct behavior is for Spotless not to format any files. Note that this is not the way Spotless currently behaves: It currently acts as though spotlessFiles is unspecified and formats all files matched by target. I have added an @Ignore annotation to that test case.

[t-rad679]

I do not see a -SNAPSHOT section in any of the CHANGES.md files. Should I create one?

[nedtwigg]

I do not see a -SNAPSHOT section

We use [Unreleased] as per keepachangelog.

This all looks good to me, except for the changes to GradleIntegrationTest. This is the only test which needs absolute path functionality, so I would rather it be put in the only place where it is being used.

I like Kotlin, I'm all for people using kotlin build scripts, but I don't think we need to use it in all/any of our tests. I'm fine with you using it in this test, but I don't want to push it into the infrastructure.

When an empty pattern is passed to spotlessFiles, what should be the behavior?

I think probably it should format nothing. But most of all, how are you using this feature? What are you connecting to? My vote is to fix what is broken for your usecase. I care a lot about Spotless being clean and correct, and I think this particular feature is deeply, irrevocably dirty. I let it in, years ago, to help someone solve their problem. I'm happy to let you merge whatever you need to solve your problem. But I do not want to spend a lot of time engineering every corner-case of this escape hatch.

[t-rad679]

This all looks good to me, except for the changes to GradleIntegrationTest. This is the only test which needs absolute path functionality, so I would rather it be put in the only place where it is being used.

The GradleIntegrationTest change has nothing to do with absolute paths. It's strictly for Kotlin support.

I like Kotlin, I'm all for people using kotlin build scripts, but I don't think we need to use it in all/any of our tests. I'm fine with you using it in this test, but I don't want to push it into the infrastructure.

Fair enough. In an ideal world, Gradle's support for plugins should be agnostic to the language in which the build is configured. I know it's a rule of unit testing to only cover the system under test, and Gradle's Kotlin build script support is not part of the system under test. In practice, though, I found a number of differences between the two, and, even though it did not actually end up affecting my use case, I could see those differences causing bugs. I just think it would be nice to be aware of these potential problems before they happen

I think probably it should format nothing. But most of all, how are you using this feature?

I'd like Spotless to format no files in the case that my git diff command returns nothing, like you mentioned somewhere in one of our conversations. I'm setting filePatterns to the results from the git diff command in the build script and I think it would be a huge surprise to our engineers to accidentally format all of their files when they run Spotless and for whatever reason git didn't see any changes to files they might have made. This is an edge case, though, and I already have an easy workaround in mind. The workaround is probably easier than fixing the bug.

I do want to say that it seems odd to me that applying Spotless only to changed files has been considered a corner case up until this point. I know you have an issue open to add more proper support for this, but that has always been a very important use case for me. Incremental style compliance is extremely useful when changing the style rules or applying new ones to an existing system. Those huge PRs that touch all of your files are really scary to me and impossible to review, so I greatly prefer to fix the style issues as I make other changes to files.

Anyway, I'll make the requested changes in the morning. Thanks!

[nedtwigg]

The GradleIntegrationTest change has nothing to do with absolute paths.

Apologies, my mistake. Moving the Kotlin-specific change out of GradleIntegrationTest is the only change to make in that case.

Incremental style compliance is extremely useful when changing the style rules or applying new ones to an existing system.

Agreed! Just hasn't made it to the top of anyone's todo list. Probably because the cost of swallowing one "format the world" commit has always been less than implementing this feature.

[t-rad679]

the cost of swallowing one "format the world" commit has always been less than implementing this feature.

Ah yeah, that makes sense.

Anyway, I've uploaded the requested changes. PTAL

[t-rad679]

Thanks for all the support, Ned!

[nedtwigg]

Thanks for making the test better 🥂

[nedtwigg]

FYI, -PspotlessFiles has been deprecated and will be removed. Migration path available here: #602