diff --git a/rewrite-core/src/main/java/org/openrewrite/Result.java b/rewrite-core/src/main/java/org/openrewrite/Result.java index 9ab157b667..623afcbac1 100755 --- a/rewrite-core/src/main/java/org/openrewrite/Result.java +++ b/rewrite-core/src/main/java/org/openrewrite/Result.java @@ -55,6 +55,12 @@ public class Result { @Getter private final Collection> recipes; + /** + * Sum of estimated time savings of all recipes that made changes to this source file. + * This is the potential time savings because if the before and after are identical, then no time is actually saved. + * This does not take multiple occurrences of the same change into account and just sums the estimated time savings + * of a single occurrence for each recipe that made a change. + */ private final Duration potentialTimeSavings; private @Nullable Duration timeSavings; @@ -64,12 +70,12 @@ public Result(@Nullable SourceFile before, @Nullable SourceFile after, Collectio this.recipes = recipes; Duration timeSavings = Duration.ZERO; + // for each stack of recipes, take the last recipe in the stack (the leaf) and sum its estimated time savings for (List recipesStack : recipes) { if (recipesStack != null && !recipesStack.isEmpty()) { Duration perOccurrence = recipesStack.get(recipesStack.size() - 1).getEstimatedEffortPerOccurrence(); if (perOccurrence != null) { - timeSavings = perOccurrence; - break; + timeSavings = timeSavings.plus(perOccurrence); } } } @@ -138,6 +144,11 @@ private static boolean subtreeChanged(Tree root, Map beforeTrees) { }.reduce(root, new AtomicBoolean(false)).get(); } + /** + * Sum of estimated time savings of all recipes that made changes to this source file. + * This does not take multiple occurrences of the same change into account and just sums the estimated time savings + * of a single occurrence for each recipe that made a change. + */ public Duration getTimeSavings() { if (timeSavings == null) { if (potentialTimeSavings.isZero() || isLocalAndHasNoChanges(before, after)) { @@ -270,10 +281,10 @@ public String toString() { public static boolean isLocalAndHasNoChanges(@Nullable SourceFile before, @Nullable SourceFile after) { try { return (before == after) || - (before != null && after != null && - // Remote source files are fetched on `printAll`, let's avoid that cost. - !(before instanceof Remote) && !(after instanceof Remote) && - before.printAll().equals(after.printAll())); + (before != null && after != null && + // Remote source files are fetched on `printAll`, let's avoid that cost. + !(before instanceof Remote) && !(after instanceof Remote) && + before.printAll().equals(after.printAll())); } catch (Exception e) { return false; } diff --git a/rewrite-core/src/main/java/org/openrewrite/scheduling/RecipeRunCycle.java b/rewrite-core/src/main/java/org/openrewrite/scheduling/RecipeRunCycle.java index c2468068d3..e975e17e9b 100644 --- a/rewrite-core/src/main/java/org/openrewrite/scheduling/RecipeRunCycle.java +++ b/rewrite-core/src/main/java/org/openrewrite/scheduling/RecipeRunCycle.java @@ -234,7 +234,7 @@ public LSS editSources(LSS sourceSet) { } return after; }, sourceFile); - } + } ); } @@ -258,32 +258,6 @@ private void recordSourceFileResult(@Nullable SourceFile before, @Nullable Sourc recipeName, effortSeconds, cycle)); - if (hierarchical) { - recordSourceFileResult(beforePath, afterPath, recipeStack.subList(0, recipeStack.size() - 1), effortSeconds, ctx); - } - } - - private void recordSourceFileResult(@Nullable String beforePath, @Nullable String afterPath, List recipeStack, Long effortSeconds, ExecutionContext ctx) { - if (recipeStack.size() <= 1) { - // No reason to record the synthetic root recipe which contains the recipe run - return; - } - String parentName; - if (recipeStack.size() == 2) { - // Record the parent name as blank rather than CompositeRecipe when the parent is the synthetic root recipe - parentName = ""; - } else { - parentName = recipeStack.get(recipeStack.size() - 2).getName(); - } - Recipe recipe = recipeStack.get(recipeStack.size() - 1); - sourcesFileResults.insertRow(ctx, new SourcesFileResults.Row( - beforePath, - afterPath, - parentName, - recipe.getName(), - effortSeconds, - cycle)); - recordSourceFileResult(beforePath, afterPath, recipeStack.subList(0, recipeStack.size() - 1), effortSeconds, ctx); } private @Nullable SourceFile handleError(Recipe recipe, SourceFile sourceFile, @Nullable SourceFile after, @@ -319,6 +293,7 @@ private static S addRecipesThatMadeChanges(List r @NonFinal @Nullable transient Boolean isScanningRecipe; + private boolean isScanningRequired() { if (isScanningRecipe == null) { isScanningRecipe = isScanningRequired(recipe); @@ -330,7 +305,7 @@ private static boolean isScanningRequired(Recipe recipe) { if (recipe instanceof ScanningRecipe) { // DeclarativeRecipe is technically a ScanningRecipe, but it only needs the // scanning phase if it or one of its sub-recipes or preconditions is a ScanningRecipe - if(recipe instanceof DeclarativeRecipe) { + if (recipe instanceof DeclarativeRecipe) { for (Recipe precondition : ((DeclarativeRecipe) recipe).getPreconditions()) { if (isScanningRequired(precondition)) { return true; diff --git a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java index cd1fc9d084..1f9ec68874 100644 --- a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java +++ b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java @@ -19,6 +19,9 @@ import lombok.Value; import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.openrewrite.config.CompositeRecipe; import org.openrewrite.marker.AlreadyReplaced; import org.openrewrite.marker.GitProvenance; import org.openrewrite.table.DistinctGitProvenance; @@ -129,6 +132,35 @@ void customEstimatedEffortForRecipeThatGeneratesSourceFiles() { )); } + @ParameterizedTest + @CsvSource({ + "'foo is here', 'bar is here', 10", // Only recipe1 applies + "'baz is here', 'qux is here', 5", // Only recipe2 applies + "'foo and baz are here', 'bar and qux are here', 15" // Both recipes apply + }) + void estimatedTimeSavingsForMultipleRecipes(String beforeContent, String afterContent, int expectedMinutes) { + // Create composite recipe with 2 recipes that have different estimated efforts + Recipe recipe1 = new CustomEstimatedEffortFindReplaceRecipe("foo", "bar", Duration.ofMinutes(10)); + Recipe recipe2 = new CustomEstimatedEffortFindReplaceRecipe("baz", "qux", Duration.ofMinutes(5)); + Recipe compositeRecipe = new CompositeRecipe(List.of(recipe1, recipe2)); + + rewriteRun( + spec -> spec.recipe(compositeRecipe) + .afterRecipe(run -> { + List results = run.getChangeset().getAllResults(); + assertThat(results).hasSize(1); + assertThat(results.getFirst().getTimeSavings()).isEqualTo(Duration.ofMinutes(expectedMinutes)); + }) + .dataTable(SourcesFileResults.Row.class, rows -> { + long totalEstimatedTimeSaving = rows.stream() + .mapToLong(SourcesFileResults.Row::getEstimatedTimeSaving) + .sum(); + assertThat(totalEstimatedTimeSaving).isEqualTo(expectedMinutes * 60L); + }), + text(beforeContent, afterContent) + ); + } + private static void assertEstimatedEffortInFirstRowOfSourceFileResults(List rows, Long expectedEstimatedEffort) { assertThat(rows) .first() @@ -166,7 +198,7 @@ public TreeVisitor getVisitor() { public PlainText visitText(PlainText text, ExecutionContext ctx) { for (AlreadyReplaced alreadyReplaced : text.getMarkers().findAll(AlreadyReplaced.class)) { if (Objects.equals(searchTerm, alreadyReplaced.getFind()) && - Objects.equals(appendText, alreadyReplaced.getReplace())) { + Objects.equals(appendText, alreadyReplaced.getReplace())) { return text; } } @@ -187,6 +219,43 @@ public Duration getEstimatedEffortPerOccurrence() { } } + @EqualsAndHashCode(callSuper = false) + @Value + private static class CustomEstimatedEffortFindReplaceRecipe extends Recipe { + String find; + String replace; + Duration estimatedEffort; + + @Override + public String getDisplayName() { + return "Simple text replace"; + } + + @Override + public String getDescription() { + return "Replaces text in files"; + } + + @Override + public TreeVisitor getVisitor() { + return new PlainTextVisitor<>() { + @Override + public PlainText visitText(PlainText text, ExecutionContext ctx) { + String newText = text.getText().replace(find, replace); + if (!newText.equals(text.getText())) { + return text.withText(newText); + } + return text; + } + }; + } + + @Override + public Duration getEstimatedEffortPerOccurrence() { + return estimatedEffort; + } + } + @EqualsAndHashCode(callSuper = false) @Value private static class CustomEstimatedEffortCreateTextFile extends ScanningRecipe {