Skip recipe validation for imperative recipes in assertRecipesConfigure() (#6841)

MBoegers · web-flow · commit bf2ca3c8d916 · 2026-03-04T16:48:40.000+01:00
diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java
@@ -19,11 +19,11 @@
 import org.junit.jupiter.api.Test;
 import org.openrewrite.DocumentExample;
 import org.openrewrite.Issue;
+import org.openrewrite.Validated;
 import org.openrewrite.test.RewriteTest;
 import org.openrewrite.test.SourceSpec;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.openrewrite.java.Assertions.mavenProject;
 import static org.openrewrite.maven.Assertions.pomXml;
 
@@ -765,33 +765,17 @@ void changeProfileManagedDependencyGroupIdAndArtifactId() {
     @Issue("https://github.com/openrewrite/rewrite-java-dependencies/issues/55")
     @Test
     void requireNewGroupIdOrNewArtifactId() {
-        assertThatExceptionOfType(AssertionError.class)
-          .isThrownBy(() -> rewriteRun(
-            spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId(
-              "javax.activation",
-              "javax.activation-api",
-              null,
-              null,
-              null,
-              null
-            ))
-          )).withMessageContaining("newGroupId OR newArtifactId must be different from before");
+       ChangeDependencyGroupIdAndArtifactId recipe = new ChangeDependencyGroupIdAndArtifactId("javax.activation", "javax.activation-api", null, null, null, null);;
+        assertThat(recipe.validate().failures()).extracting(Validated.Invalid::getMessage)
+            .contains("newGroupId OR newArtifactId must be different from before");
     }
 
     @Issue("https://github.com/openrewrite/rewrite-java-dependencies/issues/55")
     @Test
     void requireNewGroupIdOrNewArtifactIdToBeDifferentFromBefore() {
-        assertThatExceptionOfType(AssertionError.class)
-          .isThrownBy(() -> rewriteRun(
-            spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId(
-              "javax.activation",
-              "javax.activation-api",
-              "javax.activation",
-              null,
-              null,
-              null
-            ))
-          )).withMessageContaining("newGroupId OR newArtifactId must be different from before");
+        ChangeDependencyGroupIdAndArtifactId recipe = new ChangeDependencyGroupIdAndArtifactId("javax.activation", "javax.activation-api", "javax.activation", null, null, null);
+        assertThat(recipe.validate().failures()).extracting(Validated.Invalid::getMessage)
+            .contains("newGroupId OR newArtifactId must be different from before");
     }
 
     @Test
diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactIdTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactIdTest.java
@@ -18,10 +18,10 @@
 import org.junit.jupiter.api.Test;
 import org.openrewrite.DocumentExample;
 import org.openrewrite.Issue;
+import org.openrewrite.Validated;
 import org.openrewrite.test.RewriteTest;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.openrewrite.maven.Assertions.pomXml;
 
 class ChangeManagedDependencyGroupIdAndArtifactIdTest implements RewriteTest {
@@ -79,16 +79,9 @@ void changeManagedDependencyGroupIdAndArtifactId() {
     @Issue("https://github.com/openrewrite/rewrite-java-dependencies/issues/55")
     @Test
     void requireNewGroupIdOrNewArtifactIdToBeDifferentFromBefore() {
-        assertThatExceptionOfType(AssertionError.class)
-          .isThrownBy(() -> rewriteRun(
-            spec -> spec.recipe(new ChangeManagedDependencyGroupIdAndArtifactId(
-              "javax.activation",
-              "javax.activation-api",
-              "javax.activation",
-              "javax.activation-api",
-              null
-            ))
-          )).withMessageContaining("newGroupId OR newArtifactId must be different from before");
+        ChangeManagedDependencyGroupIdAndArtifactId recipe = new ChangeManagedDependencyGroupIdAndArtifactId("javax.activation", "javax.activation-api", "javax.activation", "javax.activation-api", null);
+        assertThat(recipe.validate().failures()).extracting(Validated.Invalid::getMessage)
+            .contains("newGroupId OR newArtifactId must be different from before");
     }
 
     @Test
diff --git a/rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java b/rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java
@@ -19,6 +19,7 @@
 import org.jspecify.annotations.Nullable;
 import org.openrewrite.*;
 import org.openrewrite.config.CompositeRecipe;
+import org.openrewrite.config.DeclarativeRecipe;
 import org.openrewrite.config.Environment;
 import org.openrewrite.config.OptionDescriptor;
 import org.openrewrite.internal.*;
@@ -226,11 +227,18 @@ default void rewriteRun(Consumer<RecipeSpec> spec, SourceSpec<?>... sourceSpecs)
         for (SourceSpec<?> s : sourceSpecs) {
             s.customizeExecutionContext.accept(ctx);
         }
-        List<Validated<Object>> validations = new ArrayList<>();
-        recipe.validateAll(ctx, validations);
-        assertThat(validations.stream().filter(Validated::isInvalid))
-                .as("Recipe validation must have no failures")
-                .isEmpty();
+
+        // Imperative recipes are loaded with no user-provided arguments, so all optional parameters are null.
+        // Skip recipe validation for these since custom validate() methods (e.g. requiring at least one of several
+        // optional parameters) would fail on an unconfigured instance.
+        if (recipe instanceof DeclarativeRecipe
+                || recipe.getDescriptor().getOptions().stream().allMatch(OptionDescriptor::isRequired)) {
+            List<Validated<Object>> validations = new ArrayList<>();
+            recipe.validateAll(ctx, validations);
+            assertThat(validations.stream().filter(Validated::isInvalid))
+                    .as("Recipe validation must have no failures")
+                    .isEmpty();
+        }
 
         Map<Parser.Builder, List<SourceSpec<?>>> sourceSpecsByParser = new HashMap<>();
         List<Parser.Builder> methodSpecParsers = testMethodSpec.parsers;
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
@@ -21,9 +21,11 @@
 import lombok.Value;
 import org.jspecify.annotations.NullMarked;
 import org.jspecify.annotations.Nullable;
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.openrewrite.*;
 import org.openrewrite.test.RewriteTest;
+import org.openrewrite.test.SourceSpecs;
 import org.openrewrite.test.TypeValidation;
 import org.openrewrite.text.PlainText;
 import org.openrewrite.text.PlainTextVisitor;
@@ -35,6 +37,7 @@
 
 import static java.util.Collections.emptyList;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.openrewrite.test.SourceSpecs.text;
 
@@ -121,7 +124,7 @@ void rejectRecipeValidationFailure() {
               description: Deliberately has a non-existent recipe in its recipe list to trigger a validation failure.
               recipeList:
                 - org.openrewrite.DoesNotExist
-
+              
               """, "org.openrewrite.RefersToNonExistentRecipe")
           ));
     }
@@ -152,6 +155,59 @@ void allowScannerEdit() {
           text("foo")
         );
     }
+
+    @Nested
+    class RecipeValidation {
+
+        @Test
+        void validateRecipeWithNoOptions() {
+            rewriteRun(
+              spec -> spec.recipe(new RecipeWithNoOptions()),
+              new SourceSpecs[0]
+            );
+        }
+
+        @Test
+        void validateRecipeWithOnlyRequiredOptionsPositive() {
+            rewriteRun(
+              spec -> spec.recipe(new RecipeWithRequiredOptionValidateNoBlank("Test")),
+              new SourceSpecs[0]
+            );
+        }
+
+        @Test
+        void validateRecipeWithOnlyRequiredOptionsNegative() {
+            assertThatThrownBy(() ->
+              rewriteRun(
+                spec -> spec.recipe(new RecipeWithRequiredOptionValidateNoBlank("")),
+                new SourceSpecs[0]
+              )
+            );
+        }
+
+        @Test
+        void skipValidationForRecipeWithOptionalOptions() {
+            rewriteRun(
+              spec -> spec.recipe(new RecipeWithOptionalOrValidation(null, null)),
+              new SourceSpecs[0]
+            );
+        }
+
+        @Test
+        void validateDeclarativeRecipe() {
+            assertThrows(AssertionError.class, () ->
+              rewriteRun(
+                spec -> spec.recipeFromYaml("""
+                  type: specs.openrewrite.org/v1beta/recipe
+                  name: org.openrewrite.test.internal.StillValidated
+                  displayName: Still validated
+                  description: Declarative recipe with a non-existent sub-recipe should still fail validation.
+                  recipeList:
+                    - org.openrewrite.DoesNotExist
+                  """, "org.openrewrite.test.internal.StillValidated")
+              ));
+        }
+    }
 }
 
 @EqualsAndHashCode(callSuper = false)
@@ -230,7 +286,7 @@ class CreatesTwoFilesSamePath extends ScanningRecipe<AtomicBoolean> {
     String displayName = "Creates two source files with the same path";
 
     String description = "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.";
+      "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) {
@@ -296,10 +352,10 @@ class RecipeWithDescriptionListOfLinks extends Recipe {
 
     @Getter
     final String description = """
-          A fancy description.
-          For more information, see:
-            - [link 1](https://example.com/link1)
-            - [link 2](https://example.com/link2)""";
+      A fancy description.
+      For more information, see:
+        - [link 1](https://example.com/link1)
+        - [link 2](https://example.com/link2)""";
 }
 
 @NullMarked
@@ -310,10 +366,10 @@ class RecipeWithDescriptionListOfDescribedLinks extends Recipe {
 
     @Getter
     final String description = """
-          A fancy description.
-          For more information, see:
-            - First Resource [link 1](https://example.com/link1).
-            - Second Resource [link 2](https://example.com/link2).""";
+      A fancy description.
+      For more information, see:
+        - First Resource [link 1](https://example.com/link1).
+        - Second Resource [link 2](https://example.com/link2).""";
 }
 
 @NullMarked
@@ -353,3 +409,57 @@ public Collection<? extends SourceFile> generate(AtomicBoolean acc, ExecutionCon
           .build());
     }
 }
+
+@Value
+@EqualsAndHashCode(callSuper = false)
+class RecipeWithNoOptions extends Recipe {
+    String displayName = "Recipe with no options";
+    String description = "Has no configurable options at all.";
+}
+
+@Value
+@NullMarked
+@EqualsAndHashCode(callSuper = false)
+class RecipeWithRequiredOptionValidateNoBlank extends Recipe {
+    String displayName = "Recipe with required option";
+    String description = "Has a single required parameter.";
+
+    @Option(displayName = "Required param",
+      description = "A required parameter.")
+    String requiredParam;
+
+    @Override
+    public Validated<Object> validate() {
+        return super.validate()
+          .and(Validated.notBlank("Required param", requiredParam));
+    }
+}
+
+@Value
+@NullMarked
+@EqualsAndHashCode(callSuper = false)
+class RecipeWithOptionalOrValidation extends Recipe {
+    String displayName = "Recipe with optional OR validation";
+    String description = "Has two optional parameters where at least one must be set.";
+
+    @Option(displayName = "Option A",
+      description = "First optional parameter.",
+      example = "valueA",
+      required = false)
+    @Nullable
+    String optionA;
+
+    @Option(displayName = "Option B",
+      description = "Second optional parameter.",
+      example = "valueB",
+      required = false)
+    @Nullable
+    String optionB;
+
+    @Override
+    public Validated<Object> validate() {
+        return super.validate().and(
+          Validated.required("optionA", optionA)
+            .or(Validated.required("optionB", optionB)));
+    }
+}
diff --git a/rewrite-yaml/src/test/java/org/openrewrite/yaml/MergeYamlTest.java b/rewrite-yaml/src/test/java/org/openrewrite/yaml/MergeYamlTest.java
@@ -18,9 +18,10 @@
 import org.junit.jupiter.api.Test;
 import org.openrewrite.DocumentExample;
 import org.openrewrite.Issue;
+import org.openrewrite.Validated;
 import org.openrewrite.test.RewriteTest;
 
-import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.openrewrite.yaml.Assertions.yaml;
 import static org.openrewrite.yaml.MergeYaml.InsertMode.After;
 import static org.openrewrite.yaml.MergeYaml.InsertMode.Before;
@@ -3059,22 +3060,21 @@ void preventKeysToBeAppendedToPreviousCommentIfManyLineBreaks() {
 
     @Test
     void invalidYaml() {
-        assertThrows(AssertionError.class, () -> rewriteRun(
-          spec -> spec
-            .recipe(new MergeYaml(
-              "$.some.object",
-              //language=text
-              """
-                script: |-ParseError
-                """,
-              false,
-              "name",
-              null,
-              null,
-              null,
-              null
-            ))
-        ));
+        MergeYaml recipe = new MergeYaml(
+          "$.some.object",
+          //language=text
+          """
+            script: |-ParseError
+            """,
+          false,
+          "name",
+          null,
+          null,
+          null,
+          null
+        );
+        assertThat(recipe.validate().failures()).extracting(Validated.Invalid::getMessage)
+            .contains("Must be valid YAML");
     }
 
     @Test
@@ -3144,20 +3144,18 @@ void createNewKeysFalse() {
     @SuppressWarnings("DataFlowIssue")
     @Test
     void sourceNull() {
-        assertThrows(AssertionError.class, () ->
-            rewriteRun(
-              spec -> spec
-                .recipe(new MergeYaml(
-                  "$.some.object",
-                  null,
-                  false,
-                  "name",
-                  null,
-                  null,
-                  null,
-                  null
-                ))
-            ));
+        MergeYaml recipe = new MergeYaml(
+          "$.some.object",
+          null,
+          false,
+          "name",
+          null,
+          null,
+          null,
+          null
+        );
+        assertThat(recipe.validate().failures()).extracting(Validated.Invalid::getMessage)
+            .contains("Must be valid YAML");
     }
 
     @Test
