Align estimated time savings between Result and SourcesFileResults

rewrite-core/src/main/java/org/openrewrite/Result.java

@@ -55,6 +55,12 @@ public class Result {
     @Getter
     private final Collection<List<Recipe>> 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<Recipe> 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<UUID, Tree> 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;
         }

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<Recipe> 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 extends SourceFile> S addRecipesThatMadeChanges(List<Recipe> 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;

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<Result> 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<SourcesFileResults.Row> rows, Long expectedEstimatedEffort) {
         assertThat(rows)
           .first()
@@ -166,7 +198,7 @@ public TreeVisitor<?, ExecutionContext> 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<?, ExecutionContext> 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<AtomicBoolean> {