Refactor testing #203
Refactor testing #203
Conversation
…t weird newline issues in our integration tests that we were suppressing by adding random newlines.
|
The middle commit (993390d) is the important one - it changes all our integration tests to an (imo) easier-to-read and maintain style. |
|
@lutovich - this affects your recent PR, but since you haven't accepted your committer invite yet I can't add you as reviewer. This look okay to you? |
There was a problem hiding this comment.
Hey @nedtwigg, I've just found the time to give this PR a review, as you requested over at this comment.
FYI, I've focused on reviewing commit 993390d. I'm happy to trust that the changes you made to the locations of the test resources, as you did in 7958dcc, are all happily confirmed to work by Travis. I'm also happy to trust that the apparent newline weirdness you found has been fixed. :)
Overall, I think this PR looks very good! I approve of the change to a fluent API for setting the contents of files, asserting their contents, etc. - improves readability a lot! 👍
I've written three comments below for a few areas which I think could be improved, but two of them regard minor nits, and the other one regards google-java-format and may need its own PR, so I'd be happy for those comments to be addressed in separate issues/PRs if wanted/needed.
| @@ -47,18 +46,14 @@ private void testTarget(boolean useDefaultTarget) throws IOException { | |||
| " licenseHeader('" + HEADER + "', 'plugins')", | |||
| " }", | |||
| "}"); | |||
| setFile("build.gradle").toContent(buildContent); | |||
There was a problem hiding this comment.
I wonder if lines 39-49 could be made more consistent with everything else and simplified slightly if they were made to read as something like this:
setFile("build.gradle").toLines(
"plugins {",
" id 'com.diffplug.gradle.spotless'",
"}",
"spotless {",
" groovyGradle {",
target,
" licenseHeader('" + HEADER + "', 'plugins')",
" }",
"}");WDYT, @nedtwigg?
There was a problem hiding this comment.
The buildContent string is used in the assertions below.
There was a problem hiding this comment.
Ah okay, in that case feel free to completely ignore my earlier comment! 😜
| @@ -33,27 +32,17 @@ public void integration() throws IOException { | |||
| "", | |||
| "spotless {", | |||
| " java {", | |||
| " googleJavaFormat()", | |||
| " googleJavaFormat('1.2')", | |||
There was a problem hiding this comment.
This may have to be addressed in a separate PR, but I think it's worth updating all tests that test against or use googleJavaFormat to use version 1.5 (which is currently the latest version) or as close to version 1.5 as possible.
WDYT @nedtwigg?
There was a problem hiding this comment.
There's a tradeoff: does every default version bump to a FormatterStep require changing all integration tests? If so, it's more work to bump versions. If not, there's some tiny risk of a version bump causing a problem that doesn't get caught until people use it - I can't think of any issues we've had to date where a unit test on the FormatterStep passed but it failed in integration.
If you free time and see some entropy, I'm all for applying one to the other :) But I think keeping Spotless easy to contribute to is more important than keeping its entropy low, so I'm not in favor of a policy like "unit tests must always use latest version" or something like that. People mostly contribute PR's when they have an itch to scratch - so long as their itch doesn't create unbearable itches for us, I'd like to merge them in with a minimum of guidelines.
There was a problem hiding this comment.
Sounds fair! I've just remembered actually that I was looking into updating versions from 1.x to 1.5 on my local clone of Spotless recently, so I'm happy to address this and raise my own PR for it in my own time. :)
| } | ||
|
|
||
| public void hasLines(String... lines) { | ||
| hasContent(Arrays.stream(lines).collect(Collectors.joining("\n"))); |
There was a problem hiding this comment.
Minor nit: I believe that all (or at least most) usages of <array-or-iterable-of-strings-as-stream>.collect(joining("\n")) introduced in this PR can be made shorter with String.join("\n", <array-or-iterable-of-strings>).
There was a problem hiding this comment.
You're a living javadoc for JavaSE8! Good call. I'll fixup with this and merge.
There was a problem hiding this comment.
Haha! I'll take that as a compliment. 😉
|
86a13c3 looks good to me! 👍 |
As mentioned in #190, our testing has gotten a little messy. Most of the test resources we use are copy-pasted multiple places. The originals live in
testlib/src/test/resourcesorlib-extra/src/test/resources, and they get copy-pasted intoplugin-gradleandplugin-mavenfor integration tests.We've also got some weirdness in
ResourceHarness- it sometimes adds newlines. This PR is a breaking change totestlib, and a cleanup of this jumble.In this first commit, all of the test resources have been combined into
testlib/src/test/resources, and the smallest-possible cleanup of the tests themselves to permit this.