Skip to content
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

Ktlint spotless tasks are always "up-to-date" even when there are changes #144

Closed
JLLeitschuh opened this issue Sep 21, 2017 · 15 comments
Closed
Labels

Comments

@JLLeitschuh
Copy link
Member

I see this issue both with linting Gradle Kotlin DSL files and Kotlin Source files.

Making a change to source code after running spotless and getting a success the task seems to get stuck as always "up-to-date".

A simple example of this is an updated version of the KotlinExtensionTest::testWithHeader that I wrote.
The test as it is in master currently passes 🎉 .

However, changing the test so that it looks like this:

@Test
public void testWithHeader() throws IOException {
	write("build.gradle",
			"plugins {",
			"    id 'nebula.kotlin' version '1.0.6'",
			"    id 'com.diffplug.gradle.spotless'",
			"}",
			"repositories { mavenCentral() }",
			"spotless {",
			"    kotlin {",
			"        licenseHeader('" + HEADER + "')",
			"        ktlint()",
			"    }",
			"}");

	// First run, this will put the task into it's "up-to-date" state.
	runTestWithHeader();
	// Second run should see the changes and the task should not be "up-to-date".
	runTestWithHeader();
}

private void runTestWithHeader() throws IOException {
	final File testFile = write("src/main/kotlin/test.kt", getTestResource("kotlin/licenseheader/KotlinCodeWithoutHeader.test"));
	final String original = read(testFile.toPath());
	gradleRunner().withArguments("spotlessApply").build();
	final String result = read(testFile.toPath());
	Assertions
			.assertThat(result)
			// Make sure the header gets added.
			.startsWith(HEADER)
			// Make sure that the rest of the file is still there with nothing removed.
			.endsWith(original)
			// Make sure that no additional stuff got added to the file.
			.contains(HEADER + '\n' + original);
}

Results in this test failure:

java.lang.AssertionError: 
Expecting:
 <"@file:JvmName("SomeFileName")
package my.test

object AnObject { }

">
to start with:
 <"// License Header">


	at com.diffplug.gradle.spotless.KotlinExtensionTest.runTestWithHeader(KotlinExtensionTest.java:77)
	at com.diffplug.gradle.spotless.KotlinExtensionTest.testWithHeader(KotlinExtensionTest.java:66)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:55)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)

You will notice that the failure stack trace is from the second call to runTestWithHeader meaning that the second run of spotlessApply didn't reformat the source code.

@nedtwigg
Copy link
Member

nedtwigg commented Sep 21, 2017

Try putting a sleep(2000) in between the two calls to runTestWithHeader(). Also, it would be simpler to assert that the content equals something, rather than startswith, endswith, and contains.

@JLLeitschuh
Copy link
Member Author

Regardless of the test failing I see the behaviour in my own builds.

@JLLeitschuh
Copy link
Member Author

Yes, even with the Thread.sleep the tests still fails.

@nedtwigg
Copy link
Member

Well it's great to have a reproducible test case! Adding --debug to the gradle command line will dump to console the reasons why Gradle has determined that the files are up-to-date.

@JLLeitschuh
Copy link
Member Author

Updated test code:

@Test
public void testWithHeader() throws IOException, InterruptedException {
	write("build.gradle",
			"plugins {",
			"    id 'nebula.kotlin' version '1.0.6'",
			"    id 'com.diffplug.gradle.spotless'",
			"}",
			"repositories { mavenCentral() }",
			"spotless {",
			"    kotlin {",
			"        licenseHeader('" + HEADER + "')",
			"        ktlint()",
			"    }",
			"}");

	// First run, this will put the task into it's "up-to-date" state.
	runTestWithHeader();
	Thread.sleep(2000);
	// Second run should see the changes and the task should not be "up-to-date".
	runTestWithHeader();
}

private void runTestWithHeader() throws IOException {
	final File testFile = write("src/main/kotlin/test.kt", getTestResource("kotlin/licenseheader/KotlinCodeWithoutHeader.test"));
	final String original = read(testFile.toPath());
	gradleRunner()
			.withArguments("spotlessApply", "--debug")
			.forwardOutput()
			.build();
	final String result = read(testFile.toPath());
	Assertions
			.assertThat(result)
			// Make sure the header gets added.
			.startsWith(HEADER)
			// Make sure that the rest of the file is still there with nothing removed.
			.endsWith(original)
			// Make sure that no additional stuff got added to the file.
			.isEqualTo(HEADER + '\n' + original);
}

Test results:

TestDebugLog.log.zip

@nedtwigg nedtwigg added the bug label Sep 23, 2017
nedtwigg added a commit that referenced this issue Sep 23, 2017
We had up-to-date checking in BumpThisNumberIfACustomStepChangesTest.  But we missed an important case.  Added UpToDateTest which exposes this.

Evidence for issue #144, many thanks to @JLLeitschuh for the find.
@nedtwigg
Copy link
Member

This was an excellent find. We do have have an up-to-date bug, but it's quite hard to trigger, and this test isolated it perfectly. It's not related to ktlint, it's for all our formatters.

The sequence to trigger the bug is this:

  1. Have a file with bad formatting
  2. Run spotlessApply to fix it
  3. Manually change the file back to exactly its content in step 1
  4. spotlessApply will now falsely claim to be up-to-date

If you never follow that exact sequence, you'll never see it. And to fix it, all you need to do is add a space or something and you'll be back in the proper pipeline. Surely many users (and authors!) have encountered this, but shrugged it off as a brain fart / editor saving weirdness.

I added a commit (just above) which demonstrates this more clearly. Miraculously, I've been working on another gradle plugin with weird up-to-date checking, and I think there's a hack I learned there that can fix this problem. I'll submit a PR with a fix tomorrow..

@jbduncan
Copy link
Member

Surely many users (and authors!) have encountered this, but shrugged it off as a brain fart / editor saving weirdness.

I'm pretty sure I've encountered such behaviour with Spotless myself in the past, not knowing for sure what was causing it. Here's to hoping this resolves that little niggle. 😃

@jbduncan
Copy link
Member

Although, I find it curious we even need a hack to get this working. Something to report to the Gradle team about? 🤔

nedtwigg added a commit that referenced this issue Sep 25, 2017
@nedtwigg
Copy link
Member

The up-to-date API they have is great for .java -> .class. We are modifying our own inputs, which means we're asking for them to have an API for modifying what "up-to-date" means based on the result of the execution. I will definitely put the up-to-date problems (this and my image-grinder one) together to show the Gradle team as a usecase, but it wouldn't be crazy for them to decide that our usecase is too niche.

@nedtwigg
Copy link
Member

This is currently in 3.6.0-SNAPSHOT. It's a deep enough change that I'd like to use it in integration a little before I release.

@nedtwigg
Copy link
Member

Published as 3.6.0.

@JLLeitschuh
Copy link
Member Author

Woot!!! :D Thanks for doing a deep dive on this and figuring out what the cause is. Really awesome support from you all. Awesome tool you have here. I'm always pleased with your professional nature.

@nedtwigg
Copy link
Member

Thanks for the testcase!

@nedtwigg
Copy link
Member

nedtwigg commented Oct 2, 2017

Just FYI, I've posted this and another example to the gradle forum.

@JLLeitschuh
Copy link
Member Author

I would have posted that as a github issue, but sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants