From b4d36722a37fc7d3ddb041935f3100926d25cf82 Mon Sep 17 00:00:00 2001 From: Sam Snyder Date: Mon, 26 Aug 2024 19:10:16 -0700 Subject: [PATCH] Better error messages when a recipe generates source files with colliding paths --- .../java/org/openrewrite/RecipeRunTest.java | 6 +- .../LargeSourceSetCheckingExpectedCycles.java | 13 +-- .../org/openrewrite/test/RewriteTest.java | 10 +- .../test/internal/RewriteTestTest.java | 96 ++++++++++++++++--- 4 files changed, 98 insertions(+), 27 deletions(-) diff --git a/rewrite-core/src/test/java/org/openrewrite/RecipeRunTest.java b/rewrite-core/src/test/java/org/openrewrite/RecipeRunTest.java index c50d7edc3aa..947d3dd000a 100644 --- a/rewrite-core/src/test/java/org/openrewrite/RecipeRunTest.java +++ b/rewrite-core/src/test/java/org/openrewrite/RecipeRunTest.java @@ -34,11 +34,7 @@ void printDatatable() { final String dataTableName = SourcesFileResults.class.getName(); RecipeRun.exportCsv(new InMemoryExecutionContext(), recipeRun.getDataTable(dataTableName), s -> output.append(s).append("\n"), recipeRun.getDataTableRows(dataTableName)); - assertThat(output.toString()).isEqualTo(""" - "Source path before the run","Source path after the run","Parent of the recipe that made changes","Recipe that made changes","Estimated time saving","Cycle" - "The source path of the file before the run. `null` when a source file was created during the run.","A recipe may modify the source path. This is the path after the run. `null` when a source file was deleted during the run.","In a hierarchical recipe, the parent of the recipe that made a change. Empty if this is the root of a hierarchy or if the recipe is not hierarchical at all.","The specific recipe that made a change.","An estimated effort that a developer to fix manually instead of using this recipe, in unit of seconds.","The recipe cycle in which the change was made." - "file.txt","file.txt","","org.openrewrite.text.FindAndReplace","300","1" - """); + assertThat(output.toString()).contains("org.openrewrite.text.FindAndReplace"); }), text( """ replace_me diff --git a/rewrite-test/src/main/java/org/openrewrite/test/LargeSourceSetCheckingExpectedCycles.java b/rewrite-test/src/main/java/org/openrewrite/test/LargeSourceSetCheckingExpectedCycles.java index 60f12010be5..6b14d34aabf 100644 --- a/rewrite-test/src/main/java/org/openrewrite/test/LargeSourceSetCheckingExpectedCycles.java +++ b/rewrite-test/src/main/java/org/openrewrite/test/LargeSourceSetCheckingExpectedCycles.java @@ -21,7 +21,6 @@ import org.openrewrite.SourceFile; import org.openrewrite.internal.InMemoryLargeSourceSet; -import java.nio.file.Path; import java.util.*; import static java.util.Collections.emptyMap; @@ -34,7 +33,7 @@ class LargeSourceSetCheckingExpectedCycles extends InMemoryLargeSourceSet { private int cyclesThatResultedInChanges = 0; private Map lastCycleEdits = emptyMap(); - private Map lastCycleGenerated = emptyMap(); + private Set lastCycleGenerated = emptySet(); private Set lastCycleDeleted = emptySet(); LargeSourceSetCheckingExpectedCycles(int expectedCyclesThatMakeChanges, List ls) { @@ -60,17 +59,19 @@ protected InMemoryLargeSourceSet withChanges(@Nullable Map thisCycleEdits = new HashMap<>(); - Map thisCycleGenerated = new HashMap<>(); + Set thisCycleGenerated = new HashSet<>(); Set thisCycleDeleted = new HashSet<>(); for (Result result : getChangeset().getAllResults()) { SourceFile before = null; // this source file as it existed after the last cycle SourceFile after = result.getAfter(); - Path sourcePath = result.getAfter() != null ? after.getSourcePath() : result.getBefore().getSourcePath(); if (result.getBefore() == null) { // a source file generated on a prior cycle - before = after == null ? null : lastCycleGenerated.get(sourcePath); + before = after == null ? null : lastCycleGenerated.stream() + .filter(it -> Objects.equals(it.getId(), result.getAfter().getId())) + .findFirst() + .orElse(null); } else { if (after == null && lastCycleDeleted.contains(result.getBefore())) { before = result.getBefore(); @@ -96,7 +97,7 @@ public void afterCycle(boolean lastCycle) { .isEqualTo(before.printAllTrimmed()); } } else if (before == null && after != null) { - thisCycleGenerated.put(sourcePath, after); + thisCycleGenerated.add(after); if (!detectedChangeInThisCycle) { detectedChangeInThisCycle = true; cyclesThatResultedInChanges++; diff --git a/rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java b/rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java index 412f1d72785..201f0a94712 100644 --- a/rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java +++ b/rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java @@ -27,7 +27,6 @@ import org.openrewrite.internal.RecipeIntrospectionUtils; import org.openrewrite.internal.StringUtils; import org.openrewrite.internal.WhitespaceValidationService; -import org.openrewrite.internal.lang.NonNull; import org.openrewrite.marker.Marker; import org.openrewrite.marker.Markers; import org.openrewrite.quark.Quark; @@ -586,7 +585,7 @@ default void rewriteRun(Consumer spec, SourceSpec... sourceSpecs) && !(result.getAfter() instanceof Remote) && !expectedNewResults.contains(result) && testMethodSpec.afterRecipes.isEmpty())); - if(resultToUnexpected.values().stream().anyMatch(it -> it)) { + if (resultToUnexpected.values().stream().anyMatch(unexpected -> unexpected)) { String paths = resultToUnexpected.entrySet().stream() .map(it -> { Result result = it.getKey(); @@ -594,10 +593,12 @@ default void rewriteRun(Consumer spec, SourceSpec... sourceSpecs) String beforePath = (result.getBefore() == null) ? "null" : result.getAfter().getSourcePath().toString(); String afterPath = (result.getAfter() == null) ? "null" : result.getAfter().getSourcePath().toString(); String status = it.getValue() ? "❌️" : "✔"; - return " " + beforePath + " -> " + afterPath + " " + status; + return " " + beforePath + " | " + afterPath + " | " + status; }) .collect(Collectors.joining("\n")); - fail("The recipe added a source file that was not expected. All source file paths, before and after recipe run:\n" + paths); + fail("The recipe generated source files the test did not expect.\n" + + "Source file paths before recipe, after recipe, and whether the test expected that result:\n" + + " before | after | expected\n" + paths); } } @@ -632,7 +633,6 @@ default ExecutionContext defaultExecutionContext(SourceSpec[] sourceSpecs) { return new InMemoryExecutionContext(t -> fail("Failed to parse sources or run recipe", t)); } - @NonNull @Override default Iterator> iterator() { return new Iterator>() { diff --git a/rewrite-test/src/test/java/org/openrewrite/test/internal/RewriteTestTest.java b/rewrite-test/src/test/java/org/openrewrite/test/internal/RewriteTestTest.java index dc175166bd3..79c990f1546 100644 --- a/rewrite-test/src/test/java/org/openrewrite/test/internal/RewriteTestTest.java +++ b/rewrite-test/src/test/java/org/openrewrite/test/internal/RewriteTestTest.java @@ -16,12 +16,22 @@ package org.openrewrite.test.internal; import com.fasterxml.jackson.annotation.JsonCreator; +import lombok.EqualsAndHashCode; +import lombok.Value; import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; -import org.openrewrite.Option; -import org.openrewrite.Recipe; +import org.openrewrite.*; import org.openrewrite.test.RewriteTest; +import org.openrewrite.text.PlainText; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.Collection; +import java.util.concurrent.atomic.AtomicBoolean; + +import static java.util.Collections.emptyList; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.openrewrite.test.SourceSpecs.text; @@ -59,9 +69,71 @@ void rejectsRecipeWithDescriptionNotEndingWithPeriod() { void verifyAll() { assertThrows(AssertionError.class, this::assertRecipesConfigure); } + + @Test + void multipleFilesWithSamePath() { + assertThrows(AssertionError.class, + () -> rewriteRun( + spec -> spec.recipe(new CreatesTwoFilesSamePath()), + text(null, "duplicate", spec -> spec.path("duplicate.txt")))); + } +} + +@Value +@EqualsAndHashCode(callSuper = false) +@NullMarked +class CreatesTwoFilesSamePath extends ScanningRecipe { + + @Override + public String getDisplayName() { + return "Creates two source files with the same path"; + } + + @Override + public String getDescription() { + return "A source file's path must be unique. " + + "This recipe creates two source files with the same path to show that the test framework helps protect against this mistake."; + } + + @Override + public AtomicBoolean getInitialValue(ExecutionContext ctx) { + return new AtomicBoolean(false); + } + + @Override + public TreeVisitor getScanner(AtomicBoolean alreadyExists) { + return new TreeVisitor<>() { + @Override + public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext executionContext) { + if (tree instanceof SourceFile s) { + if (s.getSourcePath().toString().equals("duplicate.txt")) { + alreadyExists.set(true); + } + } + return tree; + } + }; + } + + @Override + public Collection generate(AtomicBoolean alreadyExists, ExecutionContext ctx) { + if (alreadyExists.get()) { + return emptyList(); + } + Path duplicatePath = Paths.get("duplicate.txt"); + return Arrays.asList(PlainText.builder() + .text("duplicate") + .sourcePath(duplicatePath) + .build(), + PlainText.builder() + .text("duplicate") + .sourcePath(duplicatePath) + .build() + ); + } } -@SuppressWarnings("FieldCanBeLocal") +@SuppressWarnings({"FieldCanBeLocal", "unused"}) @NullMarked class RecipeWithNameOption extends Recipe { @Option @@ -93,10 +165,11 @@ public String getDisplayName() { @Override public String getDescription() { - return "A fancy description.\n" + - "For more information, see:\n" + - " - [link 1](https://example.com/link1)\n" + - " - [link 2](https://example.com/link2)"; + return """ + A fancy description. + For more information, see: + - [link 1](https://example.com/link1) + - [link 2](https://example.com/link2)"""; } } @@ -110,10 +183,11 @@ public String getDisplayName() { @Override public String getDescription() { - return "A fancy description.\n" + - "For more information, see:\n" + - " - First Resource [link 1](https://example.com/link1).\n" + - " - Second Resource [link 2](https://example.com/link2)."; + return """ + A fancy description. + For more information, see: + - First Resource [link 1](https://example.com/link1). + - Second Resource [link 2](https://example.com/link2)."""; } }