Additional Tests for SpecificFilesTest

[t-rad679]

SpecificFilesTest is missing a few test cases:

• Kotlin config
• Config split between two files. In other words, a common use case is to write the spotless configuration in a separate file and import into the build.gradle with apply from: spotless_config_file.gradle. This should be tested.
• Empty input
• Invalid input. i.e., [asdf\\.)? or another invalid pattern.

[t-rad679]

I wasn't aware that Spotless, when target is unspecified, tests only within the java plugin's default sourceSet. As such, the case where target and spotlessFiles are both specified should be tested as well.

[t-rad679]

And another one: The tests only cover the case of absolute path specified in spotlessFiles. We should also test relative paths.

[nedtwigg]

Spotless always looks exclusively at the field target. If you don't specify target manually, then Spotless uses the host build system's defaults for the format in question. Happy to take PR's for whatever test improvements, but I don't think combinatoric testing is necessary. There are N ways to set target. There are M actions that take target as an input. The exclusive interface between these is target. I'm fine with whatever you want to test, but I don't think you need an NxM test matrix here.

I would focus more tightly on this:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java

Lines 181 to 255 in c007dba

	@TaskAction
	public void performAction(IncrementalTaskInputs inputs) throws Exception {
	if (target == null) {
	throw new GradleException("You must specify 'Iterable<File> target'");
	}
	if (!check && !apply) {
	throw new GradleException("Don't call " + getName() + " directly, call " + getName() + SpotlessExtension.CHECK + " or " + getName() + SpotlessExtension.APPLY);
	}
	Predicate<File> shouldInclude;
	if (this.filePatterns.isEmpty()) {
	shouldInclude = file -> true;
	} else {
	// a list of files has been passed in via project property
	final String[] includePatterns = this.filePatterns.split(",");
	final List<Pattern> compiledIncludePatterns = Arrays.stream(includePatterns)
	.map(Pattern::compile)
	.collect(Collectors.toList());
	shouldInclude = file -> compiledIncludePatterns
	.stream()
	.anyMatch(filePattern -> filePattern.matcher(file.getAbsolutePath())
	.matches());
	}
	// find the outOfDate files
	List<File> outOfDate = new ArrayList<>();
	inputs.outOfDate(inputDetails -> {
	File file = inputDetails.getFile();
	if (shouldInclude.test(file) && file.isFile() && !file.equals(getCacheFile())) {
	outOfDate.add(file);
	}
	});
	// load the files that were changed by the last run
	// because it's possible the user changed them back to their
	// unformatted form, so we need to treat them as dirty
	// (see bug #144)
	if (getCacheFile().exists()) {
	LastApply lastApply = SerializableMisc.fromFile(LastApply.class, getCacheFile());
	for (File file : lastApply.changedFiles) {
	if (shouldInclude.test(file) && !outOfDate.contains(file) && file.exists() && Iterables.contains(target, file)) {
	outOfDate.add(file);
	}
	}
	}
	if (outOfDate.isEmpty()) {
	// no work to do
	return;
	}
	// create the formatter
	try (Formatter formatter = Formatter.builder()
	.lineEndingsPolicy(lineEndingsPolicy)
	.encoding(Charset.forName(encoding))
	.rootDir(getProject().getRootDir().toPath())
	.steps(steps)
	.exceptionPolicy(exceptionPolicy)
	.build()) {
	if (apply) {
	List<File> changedFiles = applyAnyChanged(formatter, outOfDate);
	if (!changedFiles.isEmpty()) {
	// If any file changed, we need to mark the task as dirty
	// next time to avoid bug #144.
	LastApply lastApply = new LastApply();
	lastApply.timestamp = System.currentTimeMillis();
	lastApply.changedFiles = changedFiles;
	SerializableMisc.toFile(lastApply, getCacheFile());
	}
	}
	if (check) {
	check(formatter, outOfDate);
	}
	}
	}

[nedtwigg]

The tests only cover the case of absolute path specified in spotlessFiles. We should also test relative paths.

I think we found the bug / our error! Look at this line:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java

Lines 201 to 202 in c007dba

	.anyMatch(filePattern -> filePattern.matcher(file.getAbsolutePath())
	.matches());

So the path gets compiled as a regex (so . does surprising things) and it has to match(), not just find(), but match(), on the whole absolute path.

This matches the documentation

The patterns are matched using String#matches(String) against the absolute file path.

I think that's probably the whole bug. I agree this is not obvious, but it looks like throughout this whole ordeal neither I nor you really looked at the code or its documentation very carefully. Which is why I put such an emphasis on good defaults, and why I'm hesitant to let too many escape hatches for one-off behaviors into the code.

[nedtwigg]

I'm open to any changes to documentation to make it less likely for others to bump into this. Probably the examples in the documentation are misleading, since they seem to be relative path. If you want a behavior change, that's tricky, because current users are counting on this behavior.

[t-rad679]

#529 (comment):

I didn't want to do NxM, I just wanted to have one test case that makes sure that cases similar to the one I'm having trouble with are covered, though I've since discovered that making that change in my dummy project didn't solve the issue. i.e., I moved my Test.java to the default sourceSet and it didn't cause spotlessFiles to work properly. I'm mostly done with a test for one such case, though, just to make sure...so I might as well finish it up.

I guess what I'm really testing is that, if the files specified in spotlessFiles are outside the default sourceSet, they are still respected if they are within target.

Rather than writing tests based on the code, I prefer to write tests based on expected behavior. Tests based on code are more likely to be brittle and less likely to test the thing you really want to verify.

#529 (comment):

Hmm, I swear I tried using absolute paths before, but I'll try again. I know I haven't tried them since I simplified my build script.

If you want a behavior change, that's tricky, because current users are counting on this behavior.

Why would it be a problem to match absolute or relative paths? As long as absolute continues working, would there be a problem? I could be mistaken, but my initial thought is that it shouldn't be hard at all to match either the absolute or relative path.

Thank you for finding this!

[nedtwigg]

Why would it be a problem to match absolute or relative paths?

Because of common prefixes. /path/to/file should not match /path/to/filesuffix, and with .match() it does not, while with .find() it does.

[t-rad679]

Nah, I get that. I was thinking something more along the lines of:

shouldInclude = file -> compiledIncludePatterns
		.stream()
		.anyMatch(filePattern -> 
                        filePattern.matcher(file.getAbsolutePath()).matches() ||
                        filePattern.matcher(file.getRelativePath()).matches());

I feel like these particular comments probably would make more sense in #528

[nedtwigg]

What is getRelativePath() relative to? When I get sucked into a support blackhole, it's so often because of an underspecified feature. This is a rare case where the feature is specified very specifically, and I'm inclined to keep it that way. The next feature request will be "-PspotlessFilesRelativeTo". I guess I'm okay with that, but only if I get to @ you on all relevant issues and PRs :)

In a different issue you mentioned that your experience with this makes you wary of customStep, which is too bad, because that's a core part that's really well tested and fairly widely used. That's the hidden cost of complicated features - when someone hits them it decreases their estimate of the quality of the whole thing, even if they're using the lone wart that was let in.

[t-rad679]

I think it's pretty fair to make the assumption that a relative path is relative to the working directory from which the command is run. In IDEs this might not be as clear, but most other tools allow it and people who want to use relative paths from their IDE are aware of this and configure their IDE appropriately. If not, they can just specify absolute paths when in their IDE and use relative paths only on the command line. Not only are relative paths easier to specify, they're also easier to read in cases like this one. The only possible exception is those odd cases where someone sets up their Gradle to run from a non-root directory, but I personally think this should be viewed as misuse of the tool and, in most cases, should not be designed for. Even so, those people could just prefix their relative paths with ../ if they really want to do that.

In a different issue you mentioned that your experience with this makes you wary of customStep, which is too bad, because that's a core part that's really well tested and fairly widely used.

I remember in #527 saying I was was wary of piggybacking the CLI off of Gradle, but I definitely didn't mean that supporting custom formatter steps was a problem. Custom formatter steps is absolutely a great feature. Was it the following quote that gave you that impression?

It feels like Bazel calling Gradle with extra steps.

"That just sounds like ______ with extra steps" is a Rick and Morty reference :P. It seems wrong to build a CLI that calls a build system directly in order to do its actual work. Part of the motivation for the CLI is to provide the option for independence from build systems. I'm not sure I can fully articulate why I feel this way, but it just seems cleaner and more resilient to build an actual CLI than to configure your CLI in Gradle then run it independently. At that point, why doesn't the user just install Gradle? They're going to have to learn all about it anyway. If you avoid this problem by generating a Gradle config from another configuration like YAML, then why not just run Spotless directly?

Anyway, if/when I end up adding stuff that you feel comfortable with, absolutely feel free to include me on any issues with or changes to that functionality. I think a notion of fine-grained code owners is great, allowing much better support from true SMEs on that part of the code. After all this, I wouldn't mind helping support the spotlessFiles feature, too.

I'm going to link to this comment in #527 because it seems relevant.

[t-rad679]

Question: What is the expected behavior when an invalid pattern is passed to spotlessFiles?

[nedtwigg]

The patterns are matched using String#matches(String) against the absolute file path.

That is the comprehensive, exhaustive documentation for the feature currently. One of the things I like about it is that I think it answers your question.

If you want to add validation for the argument, or add relative paths, I'm okay with it, but make sure the documentation explains what the paths are relative to, and make sure that it doesn't have the common-prefix problem. e.g. relative to the root project (e.g. git add subproject/Test.java) or relative to the subproject (e.g. subproject/build.gradle: file('Test.java'). Does it matter if I'm using build daemon? Does it matter if I use gdub? Are the paths always /, or are they \ on windows?

If the documentation can answer those questions in a fairly concise way, then I'll merge whatever you want. My recommendation is to write the docs first, get those signed off, and then proceed with the implementation. My opinionated recommendation is to pass absolute paths, because it takes all these questions off the table. But I'm out of time on this - if your docs are good, I'll merge what you want.

[t-rad679]

So, this ticket is actually not really about adding relative path support; or specifically related to the relative paths question. This is legitimately about tests. The empty string case, the invalid pattern case, and the Kotlin case, at least. I think I would be satisfied with those three, and I'm almost done implementing them. I just need to know how you would like it to behave with an invalid pattern and I need to implement the Kotlin test. If you assume that Kotlin Gradle functions identically to Groovy Gradle, this is trivial, but, in my experience this has not been the case. Then again, I guess one could make the argument that adding such a test here is actually testing functionality within Gradle, on some level; which is inappropriate. I would argue otherwise.

One of the things I like about it is that I think it answers your question.

The question of invalid pattern behavior? I'm still not sure what that should look like. My thoughts are that it should either fail in error or not match any files. I am assuming based on one of your previous comments that empty string should cause Spotless to target 0 files.

If left up to me, I would make the invalid pattern fail in error, so that it is easier to debug.

[nedtwigg]

From this comment onward the discussion has been about ways to implement relative path matching, and the pros and cons of it. If you want to organize the issues differently, you have my blessing to close any issues you want and open/reopen whatever other issues you want. You have my blessing copy-paste any discussion you feel is relevant.

A pattern that matches no files is not necessarily an error. For example, you could pipe git diff to a string, and pass that to spotless. If git diff returns no changes, it is not an error for Spotless to not format anything.

If you add things to a test, I'll merge it 99%+ of the time, whether there's an issue or not.

If you want to change or add functionality, I strongly recommend starting with the documentation.

[t-rad679]

Yeah, there's been a lot of back and forth on different tickets about different things. I think since the initial description of this one is about tests, it would be best to leave this as the one about tests. For now, I'm just going to update the tests.

A pattern that matches no files is not necessarily an error.

Yeah, I guessed this, and that makes sense if you pass an empty string. The part I'm not as sure of is when an invalid regex pattern is passed to spotlessFiles. For example, [?\!) will cause an error in the regex parser. I have confirmed that the current behavior of Spotless in this case is to return in error:

tomeofatsecrets:styleguide trevorradcliffe$ gradle spotlessCheck -PspotlessFiles=\[\?\!\)

> Task :spotlessJava FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':spotlessJava'.
> Unclosed character class near index 3
  [?!)
     ^

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 816ms
1 actionable task: 1 executed

Is this the behavior you want here? I think it makes sense to return in error, but it shouldn't be hard to change.

[nedtwigg]

I think it makes sense to return in error

I agree.

[t-rad679]

Should the empty pattern case format no files? This is not the current behavior; Spotless runs against all files when spotlessFiles is specified without a value (i.e., it acts as though spotlessFiles is not specified at all). If this is not the expected behavior, I would like to add the test with an @Ignore tag denoting the current broken behavior and the expected behavior.

Otherwise, I'm done with the test additions. I know it's minor, but I think it's worth it. I'll put up the PR shortly.

[nedtwigg]

Should the empty pattern case format no files?

I think so. How are you using this feature? My assumption has been that anyone using this feature was connecting it to some other tooling, and probably that tooling might sometimes want to apply to nothing.

[nedtwigg]

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