diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java index 35d5ce03ce..a6290d9cbc 100644 --- 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 index a9eb5d0f45..83e696b9fa 100644 --- 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 index f3c85b2d09..4908e10c35 100644 --- 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 spec, SourceSpec... sourceSpecs) for (SourceSpec s : sourceSpecs) { s.customizeExecutionContext.accept(ctx); } - List> 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> validations = new ArrayList<>(); + recipe.validateAll(ctx, validations); + assertThat(validations.stream().filter(Validated::isInvalid)) + .as("Recipe validation must have no failures") + .isEmpty(); + } Map>> sourceSpecsByParser = new HashMap<>(); List 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 index 814d42f597..eb3ae7609d 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 @@ -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 { 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 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 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 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 index 1d8c919474..c8d2f9e7d0 100644 --- 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