From adf6644ca5e52ce07e79b873fa2cd5d78c02da25 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Wed, 10 Sep 2025 11:41:44 +0200 Subject: [PATCH 1/9] Align estimated time savings between `Result` and `SourcesFileResults` This PR solves 2 issues: 1. Result only returns the estimated time savings from the first recipe that made a change to a file. This change sums up the estimated time of all changes to a file (however, this still does not take occurrences into account). 2. `SourcesFileResults` has rows for all recipes in the hierarchy for tracability. Because each of these rows has an estimated time savings (of the actual change) summing the column gives the wrong values. By only setting the value to the leaf recipe this column becomes sum-able. --- .../src/main/java/org/openrewrite/Result.java | 12 ++++++++++-- .../org/openrewrite/scheduling/RecipeRunCycle.java | 13 +++++++------ .../org/openrewrite/table/SourcesFileResults.java | 1 + 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/rewrite-core/src/main/java/org/openrewrite/Result.java b/rewrite-core/src/main/java/org/openrewrite/Result.java index 9ab157b667..5bfbc03008 100755 --- a/rewrite-core/src/main/java/org/openrewrite/Result.java +++ b/rewrite-core/src/main/java/org/openrewrite/Result.java @@ -55,6 +55,11 @@ 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; @@ -68,8 +73,7 @@ public Result(@Nullable SourceFile before, @Nullable SourceFile after, Collectio 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 +142,10 @@ 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)) { 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..624501c876 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); - } + } ); } @@ -259,11 +259,11 @@ private void recordSourceFileResult(@Nullable SourceFile before, @Nullable Sourc effortSeconds, cycle)); if (hierarchical) { - recordSourceFileResult(beforePath, afterPath, recipeStack.subList(0, recipeStack.size() - 1), effortSeconds, ctx); + recordSourceFileResult(beforePath, afterPath, recipeStack.subList(0, recipeStack.size() - 1), ctx); } } - private void recordSourceFileResult(@Nullable String beforePath, @Nullable String afterPath, List recipeStack, Long effortSeconds, ExecutionContext ctx) { + private void recordSourceFileResult(@Nullable String beforePath, @Nullable String afterPath, List recipeStack, ExecutionContext ctx) { if (recipeStack.size() <= 1) { // No reason to record the synthetic root recipe which contains the recipe run return; @@ -281,9 +281,9 @@ private void recordSourceFileResult(@Nullable String beforePath, @Nullable Strin afterPath, parentName, recipe.getName(), - effortSeconds, + null, // estimated time savings is only recorded for the recipe that made the change cycle)); - recordSourceFileResult(beforePath, afterPath, recipeStack.subList(0, recipeStack.size() - 1), effortSeconds, ctx); + recordSourceFileResult(beforePath, afterPath, recipeStack.subList(0, recipeStack.size() - 1), ctx); } private @Nullable SourceFile handleError(Recipe recipe, SourceFile sourceFile, @Nullable SourceFile after, @@ -319,6 +319,7 @@ private static S addRecipesThatMadeChanges(List r @NonFinal @Nullable transient Boolean isScanningRecipe; + private boolean isScanningRequired() { if (isScanningRecipe == null) { isScanningRecipe = isScanningRequired(recipe); @@ -330,7 +331,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/main/java/org/openrewrite/table/SourcesFileResults.java b/rewrite-core/src/main/java/org/openrewrite/table/SourcesFileResults.java index ff2ca20ae6..dc1aa332dd 100644 --- a/rewrite-core/src/main/java/org/openrewrite/table/SourcesFileResults.java +++ b/rewrite-core/src/main/java/org/openrewrite/table/SourcesFileResults.java @@ -53,6 +53,7 @@ public static class Row { @Column(displayName = "Estimated time saving", description = "An estimated effort that a developer to fix manually instead of using this recipe," + " in unit of seconds.") + @Nullable Long estimatedTimeSaving; @Column(displayName = "Cycle", From 82122aeb6380ded450a1f0af051ffbf1167dd0fe Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Wed, 10 Sep 2025 12:08:51 +0200 Subject: [PATCH 2/9] add test --- .../RecipeEstimatedEffortTest.java | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java index cd1fc9d084..0c99fb9705 100644 --- a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java +++ b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java @@ -19,6 +19,7 @@ import lombok.Value; import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; +import org.openrewrite.config.CompositeRecipe; import org.openrewrite.marker.AlreadyReplaced; import org.openrewrite.marker.GitProvenance; import org.openrewrite.table.DistinctGitProvenance; @@ -30,6 +31,7 @@ import java.nio.file.Path; import java.time.Duration; import java.util.Collection; +import java.util.Comparator; import java.util.List; import java.util.Objects; import java.util.concurrent.atomic.AtomicBoolean; @@ -129,6 +131,99 @@ void customEstimatedEffortForRecipeThatGeneratesSourceFiles() { )); } + @Test + void summedEstimatedEffortForMultipleChangesToSameFile() { + // Create a composite recipe with 2 recipes that each 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( + recipeSpec -> recipeSpec.recipe(compositeRecipe) + .afterRecipe(run -> { + List results = run.getChangeset().getAllResults(); + // File 1 has no changes, so it won't appear in the results + assertThat(results).hasSize(3); + + // Sort results by path for consistent ordering + results.sort(Comparator.comparing(r -> r.getAfter() != null ? + r.getAfter().getSourcePath().toString() : "")); + + // File 2: Only first change (10 minutes) + Result file2Result = results.get(0); + assertThat(file2Result.getAfter()).isNotNull(); + assertThat(file2Result.getAfter().getSourcePath().toString()).endsWith("file2.txt"); + assertThat(file2Result.getTimeSavings()).isEqualTo(Duration.ofMinutes(10)); + + // File 3: Only second change (5 minutes) + Result file3Result = results.get(1); + assertThat(file3Result.getAfter()).isNotNull(); + assertThat(file3Result.getAfter().getSourcePath().toString()).endsWith("file3.txt"); + assertThat(file3Result.getTimeSavings()).isEqualTo(Duration.ofMinutes(5)); + + // File 4: Both changes (10 + 5 = 15 minutes) + Result file4Result = results.get(2); + assertThat(file4Result.getAfter()).isNotNull(); + assertThat(file4Result.getAfter().getSourcePath().toString()).endsWith("file4.txt"); + assertThat(file4Result.getTimeSavings()).isEqualTo(Duration.ofMinutes(15)); + }) + .dataTable(SourcesFileResults.Row.class, rows -> { + // The data table will have multiple rows per file (one for each recipe that made changes) + // File 1: no rows (no changes) + // File 2: 1 row for recipe1 + // File 3: 1 row for recipe2 + // File 4: 2 rows (one for recipe1, one for recipe2) + // Total: 4 rows + assertThat(rows).hasSize(4); + + // Check that each row has the correct estimated time saving for its recipe + long totalEstimatedTimeSaving = 0; + for (SourcesFileResults.Row row : rows) { + if (row.getRecipe().contains("SimpleTextReplaceRecipe")) { + // First recipe has 10 minutes = 600 seconds + if (row.getRecipe().contains("foo")) { + assertThat(row.getEstimatedTimeSaving()).isEqualTo(600L); + } + // Second recipe has 5 minutes = 300 seconds + else if (row.getRecipe().contains("baz")) { + assertThat(row.getEstimatedTimeSaving()).isEqualTo(300L); + } + } + if (row.getEstimatedTimeSaving() != null) { + totalEstimatedTimeSaving += row.getEstimatedTimeSaving(); + } + } + + // Verify the sum of all rows' estimated time savings + // File 2: 1 recipe1 (600s) + // File 3: 1 recipe2 (300s) + // File 4: 1 recipe1 (600s) + 1 recipe2 (300s) + // Total: 600 + 300 + 600 + 300 = 1800 seconds (30 minutes) + assertThat(totalEstimatedTimeSaving).isEqualTo(1800L); + }), + text( + "nothing here", + spec -> spec.path("file1.txt") + ), + text( + "foo is here twice, look: foo", + "bar is here twice, look: bar", + spec -> spec.path("file2.txt") + ), + text( + "baz is here", + "qux is here", + spec -> spec.path("file3.txt") + ), + text( + "foo and baz are here", + "bar and qux are here", + spec -> spec.path("file4.txt") + ) + ); + } + private static void assertEstimatedEffortInFirstRowOfSourceFileResults(List rows, Long expectedEstimatedEffort) { assertThat(rows) .first() @@ -187,6 +282,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 { From dc8459a5ab3cb2e0b5a8b0d7578eabb6a0fc98d4 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 10 Sep 2025 12:49:48 +0200 Subject: [PATCH 3/9] Clear out empty lines --- .../RecipeEstimatedEffortTest.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java index 0c99fb9705..4d6ef4b721 100644 --- a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java +++ b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java @@ -136,32 +136,32 @@ void summedEstimatedEffortForMultipleChangesToSameFile() { // Create a composite recipe with 2 recipes that each 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( recipeSpec -> recipeSpec.recipe(compositeRecipe) .afterRecipe(run -> { List results = run.getChangeset().getAllResults(); // File 1 has no changes, so it won't appear in the results assertThat(results).hasSize(3); - + // Sort results by path for consistent ordering - results.sort(Comparator.comparing(r -> r.getAfter() != null ? - r.getAfter().getSourcePath().toString() : "")); - + results.sort(Comparator.comparing(r -> r.getAfter() != null ? + r.getAfter().getSourcePath().toString() : "")); + // File 2: Only first change (10 minutes) Result file2Result = results.get(0); assertThat(file2Result.getAfter()).isNotNull(); assertThat(file2Result.getAfter().getSourcePath().toString()).endsWith("file2.txt"); assertThat(file2Result.getTimeSavings()).isEqualTo(Duration.ofMinutes(10)); - + // File 3: Only second change (5 minutes) Result file3Result = results.get(1); assertThat(file3Result.getAfter()).isNotNull(); assertThat(file3Result.getAfter().getSourcePath().toString()).endsWith("file3.txt"); assertThat(file3Result.getTimeSavings()).isEqualTo(Duration.ofMinutes(5)); - + // File 4: Both changes (10 + 5 = 15 minutes) Result file4Result = results.get(2); assertThat(file4Result.getAfter()).isNotNull(); @@ -176,7 +176,7 @@ void summedEstimatedEffortForMultipleChangesToSameFile() { // File 4: 2 rows (one for recipe1, one for recipe2) // Total: 4 rows assertThat(rows).hasSize(4); - + // Check that each row has the correct estimated time saving for its recipe long totalEstimatedTimeSaving = 0; for (SourcesFileResults.Row row : rows) { @@ -194,7 +194,7 @@ else if (row.getRecipe().contains("baz")) { totalEstimatedTimeSaving += row.getEstimatedTimeSaving(); } } - + // Verify the sum of all rows' estimated time savings // File 2: 1 recipe1 (600s) // File 3: 1 recipe2 (300s) From 4f190f20253ceb3037cebdd22c418c1df47a44b5 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Thu, 11 Sep 2025 12:49:21 +0200 Subject: [PATCH 4/9] Simplify test --- .../RecipeEstimatedEffortTest.java | 99 +++---------------- 1 file changed, 15 insertions(+), 84 deletions(-) diff --git a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java index 4d6ef4b721..124f0d7a36 100644 --- a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java +++ b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java @@ -19,6 +19,8 @@ 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; @@ -31,7 +33,6 @@ import java.nio.file.Path; import java.time.Duration; import java.util.Collection; -import java.util.Comparator; import java.util.List; import java.util.Objects; import java.util.concurrent.atomic.AtomicBoolean; @@ -131,96 +132,26 @@ void customEstimatedEffortForRecipeThatGeneratesSourceFiles() { )); } - @Test - void summedEstimatedEffortForMultipleChangesToSameFile() { - // Create a composite recipe with 2 recipes that each have different estimated efforts + @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( - recipeSpec -> recipeSpec.recipe(compositeRecipe) + spec -> spec.recipe(compositeRecipe) .afterRecipe(run -> { List results = run.getChangeset().getAllResults(); - // File 1 has no changes, so it won't appear in the results - assertThat(results).hasSize(3); - - // Sort results by path for consistent ordering - results.sort(Comparator.comparing(r -> r.getAfter() != null ? - r.getAfter().getSourcePath().toString() : "")); - - // File 2: Only first change (10 minutes) - Result file2Result = results.get(0); - assertThat(file2Result.getAfter()).isNotNull(); - assertThat(file2Result.getAfter().getSourcePath().toString()).endsWith("file2.txt"); - assertThat(file2Result.getTimeSavings()).isEqualTo(Duration.ofMinutes(10)); - - // File 3: Only second change (5 minutes) - Result file3Result = results.get(1); - assertThat(file3Result.getAfter()).isNotNull(); - assertThat(file3Result.getAfter().getSourcePath().toString()).endsWith("file3.txt"); - assertThat(file3Result.getTimeSavings()).isEqualTo(Duration.ofMinutes(5)); - - // File 4: Both changes (10 + 5 = 15 minutes) - Result file4Result = results.get(2); - assertThat(file4Result.getAfter()).isNotNull(); - assertThat(file4Result.getAfter().getSourcePath().toString()).endsWith("file4.txt"); - assertThat(file4Result.getTimeSavings()).isEqualTo(Duration.ofMinutes(15)); - }) - .dataTable(SourcesFileResults.Row.class, rows -> { - // The data table will have multiple rows per file (one for each recipe that made changes) - // File 1: no rows (no changes) - // File 2: 1 row for recipe1 - // File 3: 1 row for recipe2 - // File 4: 2 rows (one for recipe1, one for recipe2) - // Total: 4 rows - assertThat(rows).hasSize(4); - - // Check that each row has the correct estimated time saving for its recipe - long totalEstimatedTimeSaving = 0; - for (SourcesFileResults.Row row : rows) { - if (row.getRecipe().contains("SimpleTextReplaceRecipe")) { - // First recipe has 10 minutes = 600 seconds - if (row.getRecipe().contains("foo")) { - assertThat(row.getEstimatedTimeSaving()).isEqualTo(600L); - } - // Second recipe has 5 minutes = 300 seconds - else if (row.getRecipe().contains("baz")) { - assertThat(row.getEstimatedTimeSaving()).isEqualTo(300L); - } - } - if (row.getEstimatedTimeSaving() != null) { - totalEstimatedTimeSaving += row.getEstimatedTimeSaving(); - } - } - - // Verify the sum of all rows' estimated time savings - // File 2: 1 recipe1 (600s) - // File 3: 1 recipe2 (300s) - // File 4: 1 recipe1 (600s) + 1 recipe2 (300s) - // Total: 600 + 300 + 600 + 300 = 1800 seconds (30 minutes) - assertThat(totalEstimatedTimeSaving).isEqualTo(1800L); + assertThat(results).hasSize(1); + assertThat(results.getFirst().getTimeSavings()).isEqualTo(Duration.ofMinutes(expectedMinutes)); }), - text( - "nothing here", - spec -> spec.path("file1.txt") - ), - text( - "foo is here twice, look: foo", - "bar is here twice, look: bar", - spec -> spec.path("file2.txt") - ), - text( - "baz is here", - "qux is here", - spec -> spec.path("file3.txt") - ), - text( - "foo and baz are here", - "bar and qux are here", - spec -> spec.path("file4.txt") - ) + text(beforeContent, afterContent) ); } @@ -261,7 +192,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; } } From f215f5ca70f103f34bff7704984c38ee6ea49efd Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Thu, 11 Sep 2025 12:51:19 +0200 Subject: [PATCH 5/9] Simplify test --- .../java/org/openrewrite/RecipeEstimatedEffortTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java index 124f0d7a36..897340e1b7 100644 --- a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java +++ b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java @@ -150,6 +150,12 @@ void estimatedTimeSavingsForMultipleRecipes(String beforeContent, String afterCo 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(row -> row.getEstimatedTimeSaving() != null ? row.getEstimatedTimeSaving() : 0L) + .sum(); + assertThat(totalEstimatedTimeSaving).isEqualTo(expectedMinutes * 60L); }), text(beforeContent, afterContent) ); From c2503877aaba48c2e674e6dcbfdc20c2685f7146 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Thu, 11 Sep 2025 12:51:51 +0200 Subject: [PATCH 6/9] formatting --- .../src/main/java/org/openrewrite/Result.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/rewrite-core/src/main/java/org/openrewrite/Result.java b/rewrite-core/src/main/java/org/openrewrite/Result.java index 5bfbc03008..3617f62e3f 100755 --- a/rewrite-core/src/main/java/org/openrewrite/Result.java +++ b/rewrite-core/src/main/java/org/openrewrite/Result.java @@ -55,7 +55,8 @@ public class Result { @Getter private final Collection> recipes; - /** Sum of estimated time savings of all recipes that made changes to this source file. + /** + * 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. @@ -142,7 +143,8 @@ 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. + /** + * 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. */ @@ -278,10 +280,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; } From d2180e46c243b864121698e724dcc6afb0e79e22 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Mon, 15 Sep 2025 16:04:14 +0200 Subject: [PATCH 7/9] Remove the entire hierarchy in `SourcesFileResults` and add a comment to the logic in `Result.java` --- .../src/main/java/org/openrewrite/Result.java | 1 + .../scheduling/RecipeRunCycle.java | 26 ------------------- 2 files changed, 1 insertion(+), 26 deletions(-) diff --git a/rewrite-core/src/main/java/org/openrewrite/Result.java b/rewrite-core/src/main/java/org/openrewrite/Result.java index 3617f62e3f..623afcbac1 100755 --- a/rewrite-core/src/main/java/org/openrewrite/Result.java +++ b/rewrite-core/src/main/java/org/openrewrite/Result.java @@ -70,6 +70,7 @@ 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(); 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 624501c876..e975e17e9b 100644 --- a/rewrite-core/src/main/java/org/openrewrite/scheduling/RecipeRunCycle.java +++ b/rewrite-core/src/main/java/org/openrewrite/scheduling/RecipeRunCycle.java @@ -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), ctx); - } - } - - private void recordSourceFileResult(@Nullable String beforePath, @Nullable String afterPath, List recipeStack, 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(), - null, // estimated time savings is only recorded for the recipe that made the change - cycle)); - recordSourceFileResult(beforePath, afterPath, recipeStack.subList(0, recipeStack.size() - 1), ctx); } private @Nullable SourceFile handleError(Recipe recipe, SourceFile sourceFile, @Nullable SourceFile after, From b2a01445e745b3b769a23e04c28f09f20982f3f1 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Mon, 15 Sep 2025 16:05:51 +0200 Subject: [PATCH 8/9] revert nullable --- .../src/main/java/org/openrewrite/table/SourcesFileResults.java | 1 - 1 file changed, 1 deletion(-) diff --git a/rewrite-core/src/main/java/org/openrewrite/table/SourcesFileResults.java b/rewrite-core/src/main/java/org/openrewrite/table/SourcesFileResults.java index dc1aa332dd..ff2ca20ae6 100644 --- a/rewrite-core/src/main/java/org/openrewrite/table/SourcesFileResults.java +++ b/rewrite-core/src/main/java/org/openrewrite/table/SourcesFileResults.java @@ -53,7 +53,6 @@ public static class Row { @Column(displayName = "Estimated time saving", description = "An estimated effort that a developer to fix manually instead of using this recipe," + " in unit of seconds.") - @Nullable Long estimatedTimeSaving; @Column(displayName = "Cycle", From fb4ff7e0fdd340281ebee576c9f581ee95860fde Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Mon, 15 Sep 2025 16:18:43 +0200 Subject: [PATCH 9/9] simplify --- .../test/java/org/openrewrite/RecipeEstimatedEffortTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java index 897340e1b7..1f9ec68874 100644 --- a/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java +++ b/rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java @@ -153,7 +153,7 @@ void estimatedTimeSavingsForMultipleRecipes(String beforeContent, String afterCo }) .dataTable(SourcesFileResults.Row.class, rows -> { long totalEstimatedTimeSaving = rows.stream() - .mapToLong(row -> row.getEstimatedTimeSaving() != null ? row.getEstimatedTimeSaving() : 0L) + .mapToLong(SourcesFileResults.Row::getEstimatedTimeSaving) .sum(); assertThat(totalEstimatedTimeSaving).isEqualTo(expectedMinutes * 60L); }),