Breaking changes to Spotless' internal testing infrastructure testlib (#1443)

Author: nedtwigg

CHANGES.md

@@ -13,6 +13,11 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 ### Changes
 * Bump the dev version of Gradle from `7.5.1` to `7.6` ([#1409](https://github.com/diffplug/spotless/pull/1409))
   * We also removed the no-longer-required dependency `org.codehaus.groovy:groovy-xml`
+* Breaking changes to Spotless' internal testing infrastructure `testlib` ([#1443](https://github.com/diffplug/spotless/pull/1443))
+  * `ResourceHarness` no longer has any duplicated functionality which was also present in `StepHarness`
+  * `StepHarness` now operates on `Formatter` rather than a `FormatterStep`
+  * `StepHarnessWithFile` now takes a `ResourceHarness` in its constructor to handle the file manipulation parts
+  * Standardized that we test exception *messages*, not types, which will ease the transition to linting later on
 
 ## [2.31.1] - 2023-01-02
 ### Fixed

lib-extra/src/test/java/com/diffplug/spotless/extra/GitAttributesTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2022 DiffPlug
+ * Copyright 2016-2023 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -26,24 +26,19 @@
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.junit.jupiter.api.Test;
 
-import com.diffplug.common.base.Errors;
 import com.diffplug.common.base.StringPrinter;
 import com.diffplug.spotless.LineEnding;
 import com.diffplug.spotless.ResourceHarness;
 
 class GitAttributesTest extends ResourceHarness {
 	private List<File> testFiles(String prefix) {
-		try {
-			List<File> result = new ArrayList<>();
-			for (String path : TEST_PATHS) {
-				String prefixedPath = prefix + path;
-				setFile(prefixedPath).toContent("");
-				result.add(newFile(prefixedPath));
-			}
-			return result;
-		} catch (IOException e) {
-			throw Errors.asRuntime(e);
+		List<File> result = new ArrayList<>();
+		for (String path : TEST_PATHS) {
+			String prefixedPath = prefix + path;
+			setFile(prefixedPath).toContent("");
+			result.add(newFile(prefixedPath));
 		}
+		return result;
 	}
 
 	private List<File> testFiles() {
@@ -53,7 +48,7 @@ private List<File> testFiles() {
 	private static final List<String> TEST_PATHS = Arrays.asList("someFile", "subfolder/someFile", "MANIFEST.MF", "subfolder/MANIFEST.MF");
 
 	@Test
-	void cacheTest() throws IOException {
+	void cacheTest() {
 		setFile(".gitattributes").toContent(StringPrinter.buildStringFromLines(
 				"* eol=lf",
 				"*.MF eol=crlf"));
@@ -84,7 +79,7 @@ void cacheTest() throws IOException {
 	}
 
 	@Test
-	void policyTest() throws IOException {
+	void policyTest() {
 		setFile(".gitattributes").toContent(StringPrinter.buildStringFromLines(
 				"* eol=lf",
 				"*.MF eol=crlf"));
@@ -96,7 +91,7 @@ void policyTest() throws IOException {
 	}
 
 	@Test
-	void policyDefaultLineEndingTest() throws GitAPIException, IOException {
+	void policyDefaultLineEndingTest() throws GitAPIException {
 		Git git = Git.init().setDirectory(rootFolder()).call();
 		git.close();
 		setFile(".git/config").toContent(StringPrinter.buildStringFromLines(

testlib/src/main/java/com/diffplug/spotless/ResourceHarness.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2021 DiffPlug
+ * Copyright 2016-2023 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -54,7 +54,7 @@ protected File rootFolder() {
 	}
 
 	/** Returns a new child of the root folder. */
-	protected File newFile(String subpath) throws IOException {
+	protected File newFile(String subpath) {
 		return new File(rootFolder(), subpath);
 	}
 
@@ -85,16 +85,16 @@ protected void replace(String path, String toReplace, String replaceWith) throws
 	}
 
 	/** Returns the contents of the given file from the src/test/resources directory. */
-	protected static String getTestResource(String filename) throws IOException {
+	protected static String getTestResource(String filename) {
 		URL url = ResourceHarness.class.getResource("/" + filename);
 		if (url == null) {
 			throw new IllegalArgumentException("No such resource " + filename);
 		}
-		return Resources.toString(url, StandardCharsets.UTF_8);
+		return ThrowingEx.get(() -> LineEnding.toUnix(Resources.toString(url, StandardCharsets.UTF_8)));
 	}
 
 	/** Returns Files (in a temporary folder) which has the contents of the given file from the src/test/resources directory. */
-	protected List<File> createTestFiles(String... filenames) throws IOException {
+	protected List<File> createTestFiles(String... filenames) {
 		List<File> files = new ArrayList<>(filenames.length);
 		for (String filename : filenames) {
 			files.add(createTestFile(filename));
@@ -103,47 +103,30 @@ protected List<File> createTestFiles(String... filenames) throws IOException {
 	}
 
 	/** Returns a File (in a temporary folder) which has the contents of the given file from the src/test/resources directory. */
-	protected File createTestFile(String filename) throws IOException {
+	protected File createTestFile(String filename) {
 		return createTestFile(filename, UnaryOperator.identity());
 	}
 
 	/**
 	 * Returns a File (in a temporary folder) which has the contents, possibly processed, of the given file from the
 	 * src/test/resources directory.
 	 */
-	protected File createTestFile(String filename, UnaryOperator<String> fileContentsProcessor) throws IOException {
+	protected File createTestFile(String filename, UnaryOperator<String> fileContentsProcessor) {
 		int lastSlash = filename.lastIndexOf('/');
 		String name = lastSlash >= 0 ? filename.substring(lastSlash) : filename;
 		File file = newFile(name);
 		file.getParentFile().mkdirs();
-		Files.write(file.toPath(), fileContentsProcessor.apply(getTestResource(filename)).getBytes(StandardCharsets.UTF_8));
+		ThrowingEx.run(() -> Files.write(file.toPath(), fileContentsProcessor.apply(getTestResource(filename)).getBytes(StandardCharsets.UTF_8)));
 		return file;
 	}
 
-	/** Reads the given resource from "before", applies the step, and makes sure the result is "after". */
-	protected void assertOnResources(FormatterStep step, String unformattedPath, String expectedPath) throws Throwable {
-		assertOnResources(rawUnix -> step.format(rawUnix, new File("")), unformattedPath, expectedPath);
-	}
-
-	/** Reads the given resource from "before", applies the step, and makes sure the result is "after". */
-	protected void assertOnResources(FormatterFunc step, String unformattedPath, String expectedPath) throws Throwable {
-		String unformatted = LineEnding.toUnix(getTestResource(unformattedPath)); // unix-ified input
-		String formatted = step.apply(unformatted);
-		// no windows newlines
-		assertThat(formatted).doesNotContain("\r");
-
-		// unix-ify the test resource output in case git screwed it up
-		String expected = LineEnding.toUnix(getTestResource(expectedPath)); // unix-ified output
-		assertThat(formatted).isEqualTo(expected);
-	}
-
 	@CheckReturnValue
-	protected ReadAsserter assertFile(String path) throws IOException {
+	protected ReadAsserter assertFile(String path) {
 		return new ReadAsserter(newFile(path));
 	}
 
 	@CheckReturnValue
-	protected ReadAsserter assertFile(File file) throws IOException {
+	protected ReadAsserter assertFile(File file) {
 		return new ReadAsserter(file);
 	}
 
@@ -176,7 +159,7 @@ public void matches(Consumer<AbstractCharSequenceAssert<?, String>> conditions)
 		}
 	}
 
-	protected WriteAsserter setFile(String path) throws IOException {
+	protected WriteAsserter setFile(String path) {
 		return new WriteAsserter(newFile(path));
 	}
 
@@ -188,21 +171,25 @@ private WriteAsserter(File file) {
 			this.file = file;
 		}
 
-		public File toLines(String... lines) throws IOException {
+		public File toLines(String... lines) {
 			return toContent(String.join("\n", Arrays.asList(lines)));
 		}
 
-		public File toContent(String content) throws IOException {
+		public File toContent(String content) {
 			return toContent(content, StandardCharsets.UTF_8);
 		}
 
-		public File toContent(String content, Charset charset) throws IOException {
-			Files.write(file.toPath(), content.getBytes(charset));
+		public File toContent(String content, Charset charset) {
+			ThrowingEx.run(() -> {
+				Files.write(file.toPath(), content.getBytes(charset));
+			});
 			return file;
 		}
 
-		public File toResource(String path) throws IOException {
-			Files.write(file.toPath(), getTestResource(path).getBytes(StandardCharsets.UTF_8));
+		public File toResource(String path) {
+			ThrowingEx.run(() -> {
+				Files.write(file.toPath(), getTestResource(path).getBytes(StandardCharsets.UTF_8));
+			});
 			return file;
 		}
 

testlib/src/main/java/com/diffplug/spotless/StepHarness.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2021 DiffPlug
+ * Copyright 2016-2023 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -22,31 +22,21 @@
 import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Objects;
-import java.util.function.Consumer;
 
-import org.assertj.core.api.AbstractThrowableAssert;
+import org.assertj.core.api.AbstractStringAssert;
 import org.assertj.core.api.Assertions;
 
 /** An api for testing a {@code FormatterStep} that doesn't depend on the File path. DO NOT ADD FILE SUPPORT TO THIS, use {@link StepHarnessWithFile} if you need that. */
 public class StepHarness implements AutoCloseable {
-	private final FormatterFunc formatter;
+	private final Formatter formatter;
 
-	private StepHarness(FormatterFunc formatter) {
+	private StepHarness(Formatter formatter) {
 		this.formatter = Objects.requireNonNull(formatter);
 	}
 
 	/** Creates a harness for testing steps which don't depend on the file. */
 	public static StepHarness forStep(FormatterStep step) {
-		// We don't care if an individual FormatterStep is misbehaving on line-endings, because
-		// Formatter fixes that.  No reason to care in tests either.  It's likely to pop up when
-		// running tests on Windows from time-to-time
-		return new StepHarness(FormatterFunc.Closeable.ofDangerous(
-				() -> {
-					if (step instanceof FormatterStepImpl.Standard) {
-						((FormatterStepImpl.Standard<?>) step).cleanupFormatterFunc();
-					}
-				},
-				input -> LineEnding.toUnix(step.format(input, new File("")))));
+		return forSteps(step);
 	}
 
 	/** Creates a harness for testing steps which don't depend on the file. */
@@ -56,60 +46,68 @@ public static StepHarness forSteps(FormatterStep... steps) {
 				.lineEndingsPolicy(LineEnding.UNIX.createPolicy())
 				.encoding(StandardCharsets.UTF_8)
 				.rootDir(Paths.get(""))
+				.exceptionPolicy(new FormatExceptionPolicyStrict())
 				.build());
 	}
 
 	/** Creates a harness for testing a formatter whose steps don't depend on the file. */
 	public static StepHarness forFormatter(Formatter formatter) {
-		return new StepHarness(FormatterFunc.Closeable.ofDangerous(
-				formatter::close,
-				input -> formatter.compute(input, new File(""))));
+		return new StepHarness(formatter);
 	}
 
 	/** Asserts that the given element is transformed as expected, and that the result is idempotent. */
-	public StepHarness test(String before, String after) throws Exception {
-		String actual = formatter.apply(before);
+	public StepHarness test(String before, String after) {
+		String actual = formatter.compute(LineEnding.toUnix(before), new File(""));
 		assertEquals(after, actual, "Step application failed");
 		return testUnaffected(after);
 	}
 
 	/** Asserts that the given element is idempotent w.r.t the step under test. */
-	public StepHarness testUnaffected(String idempotentElement) throws Exception {
-		String actual = formatter.apply(idempotentElement);
+	public StepHarness testUnaffected(String idempotentElement) {
+		String actual = formatter.compute(LineEnding.toUnix(idempotentElement), new File(""));
 		assertEquals(idempotentElement, actual, "Step is not idempotent");
 		return this;
 	}
 
 	/** Asserts that the given elements in  the resources directory are transformed as expected. */
-	public StepHarness testResource(String resourceBefore, String resourceAfter) throws Exception {
+	public StepHarness testResource(String resourceBefore, String resourceAfter) {
 		String before = ResourceHarness.getTestResource(resourceBefore);
 		String after = ResourceHarness.getTestResource(resourceAfter);
 		return test(before, after);
 	}
 
 	/** Asserts that the given elements in the resources directory are transformed as expected. */
-	public StepHarness testResourceUnaffected(String resourceIdempotent) throws Exception {
+	public StepHarness testResourceUnaffected(String resourceIdempotent) {
 		String idempotentElement = ResourceHarness.getTestResource(resourceIdempotent);
 		return testUnaffected(idempotentElement);
 	}
 
-	/** Asserts that the given elements in the resources directory are transformed as expected. */
-	public StepHarness testResourceException(String resourceBefore, Consumer<AbstractThrowableAssert<?, ? extends Throwable>> exceptionAssertion) throws Exception {
-		return testException(ResourceHarness.getTestResource(resourceBefore), exceptionAssertion);
+	public AbstractStringAssert<?> testResourceExceptionMsg(String resourceBefore) {
+		return testExceptionMsg(ResourceHarness.getTestResource(resourceBefore));
 	}
 
-	/** Asserts that the given elements in the resources directory are transformed as expected. */
-	public StepHarness testException(String before, Consumer<AbstractThrowableAssert<?, ? extends Throwable>> exceptionAssertion) throws Exception {
-		Throwable t = assertThrows(Throwable.class, () -> formatter.apply(before));
-		AbstractThrowableAssert<?, ? extends Throwable> abstractAssert = Assertions.assertThat(t);
-		exceptionAssertion.accept(abstractAssert);
-		return this;
+	public AbstractStringAssert<?> testExceptionMsg(String before) {
+		try {
+			formatter.compute(LineEnding.toUnix(before), FormatterStepImpl.SENTINEL);
+			throw new SecurityException("Expected exception");
+		} catch (Throwable e) {
+			if (e instanceof SecurityException) {
+				throw new AssertionError(e.getMessage());
+			} else {
+				Throwable rootCause = e;
+				while (rootCause.getCause() != null) {
+					if (rootCause instanceof IllegalStateException) {
+						break;
+					}
+					rootCause = rootCause.getCause();
+				}
+				return Assertions.assertThat(rootCause.getMessage());
+			}
+		}
 	}
 
 	@Override
 	public void close() {
-		if (formatter instanceof FormatterFunc.Closeable) {
-			((FormatterFunc.Closeable) formatter).close();
-		}
+		formatter.close();
 	}
 }