You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/01/13 11:06:07 UTC
[GitHub] [flink] Airblader commented on a change in pull request #18333: [FLINK-25220][test] Write an architectural rule for all IT cases w.r.t. the MiniCluster
Airblader commented on a change in pull request #18333:
URL: https://github.com/apache/flink/pull/18333#discussion_r783821321
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/ArchitectureTest.java
##########
@@ -78,4 +79,36 @@ public boolean includes(Location location) {
return !location.matches(SHADED);
}
}
+
+ /**
+ * Exclude locations that contains test classes.
+ *
+ * <p>Note: classes e.g. within jars under flink-test-utils-parent pom module are located as
+ * production code and should be excluded as test classes. Please see {@link
+ * ImportOption.Predefined#DO_NOT_INCLUDE_TESTS}
+ */
+ static class ExcludeTestJarsImportOption implements ImportOption {
+ private static final Pattern TEST = Pattern.compile(".*-test.*\\.jar.*");
+
+ @Override
+ public boolean includes(Location location) {
+ return !location.matches(TEST);
+ }
+ }
+
+ /**
+ * Include test relevant classes.
+ *
+ * <p>Note: since the test should be work both when maven and Intellij. The pattern will be
+ * improved to be more precise continuously.
+ */
+ static class OnlyTestJarsImportOption implements ImportOption {
Review comment:
Can we just use the inverse of the import option above?
This pattern is a bit "dangerous", because it would easily find anything that contains "test" anywhere in the entire location.
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/common/Predicates.java
##########
@@ -50,5 +83,78 @@
&& field.getRawType().isEquivalentTo(clazz));
}
+ /**
+ * Tests that the given field is {@code public final} and not {@code static} and of the given
+ * type {@code clazz} .
+ */
+ public static DescribedPredicate<JavaField> arePublicFinalOfTyp(Class<?> clazz) {
+ return is(ofType(clazz)).and(isPublic()).and(isFinal()).and(isNotStatic());
+ }
+
+ /**
+ * Tests that the given field is {@code public static final} and of the given type {@code clazz}
+ * .
+ */
+ public static DescribedPredicate<JavaField> arePublicStaticFinalOfTyp(Class<?> clazz) {
Review comment:
Typ → Type
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/common/Predicates.java
##########
@@ -50,5 +83,78 @@
&& field.getRawType().isEquivalentTo(clazz));
}
+ /**
+ * Tests that the given field is {@code public final} and not {@code static} and of the given
+ * type {@code clazz} .
+ */
+ public static DescribedPredicate<JavaField> arePublicFinalOfTyp(Class<?> clazz) {
+ return is(ofType(clazz)).and(isPublic()).and(isFinal()).and(isNotStatic());
+ }
+
+ /**
+ * Tests that the given field is {@code public static final} and of the given type {@code clazz}
+ * .
+ */
+ public static DescribedPredicate<JavaField> arePublicStaticFinalOfTyp(Class<?> clazz) {
+ return arePublicStaticOfType(clazz).and(isFinal());
+ }
+
+ /**
+ * Tests that the given field is {@code public final} and of the given type {@code clazz} with
+ * exactly the given {@code annotationType}.
+ */
+ public static DescribedPredicate<JavaField> arePublicFinalOfTypeWithAnnotation(
+ Class<?> clazz, Class<? extends Annotation> annotationType) {
+ return arePublicFinalOfTyp(clazz).and(annotatedWith(annotationType));
+ }
+
+ /**
+ * Tests that the given field is {@code public static final} and of the given type {@code clazz}
+ * with exactly the given {@code annotationType}.
+ */
+ public static DescribedPredicate<JavaField> arePublicStaticFinalOfTypeWithAnnotation(
+ Class<?> clazz, Class<? extends Annotation> annotationType) {
+ return arePublicStaticFinalOfTyp(clazz).and(annotatedWith(annotationType));
+ }
+
+ /**
+ * Tests that the given field is {@code public static final} and of the given type. It must have
+ * any of the given {@code annotationTypes}
+ */
+ @SafeVarargs
+ public static DescribedPredicate<JavaField> arePublicStaticFinalOfTypeWithOneOfAnnotations(
+ Class<?> clazz, Class<? extends Annotation>... annotationTypes) {
+ return arePublicStaticFinalOfTyp(clazz).and(annotatedWithOneOf(annotationTypes));
+ }
+
private Predicates() {}
+
+ /**
+ * A predicate to determine if a {@link JavaClass} contains one or more {@link JavaField }
Review comment:
```suggestion
* A predicate to determine if a {@link JavaClass} contains one or more {@link JavaField}
```
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/common/JavaFieldPredicates.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.architecture.common;
+
+import com.tngtech.archunit.base.DescribedPredicate;
+import com.tngtech.archunit.core.domain.JavaAnnotation;
+import com.tngtech.archunit.core.domain.JavaField;
+import com.tngtech.archunit.core.domain.JavaModifier;
+
+import java.lang.annotation.Annotation;
+import java.util.Arrays;
+
+/** Fine-grained predicates focus on the JavaField. */
+public class JavaFieldPredicates {
+
+ /**
+ * Match the public modifier of the {@link JavaField}.
+ *
+ * @return A {@link DescribedPredicate} returning true, if and only if the tested {@link
+ * JavaField} has the public modifier.
+ */
+ public static DescribedPredicate<JavaField> isPublic() {
+ return DescribedPredicate.describe(
+ "public", field -> field.getModifiers().contains(JavaModifier.PUBLIC));
+ }
+
+ /**
+ * Match the static modifier of the {@link JavaField}.
+ *
+ * @return A {@link DescribedPredicate} returning true, if and only if the tested {@link
+ * JavaField} has the static modifier.
+ */
+ public static DescribedPredicate<JavaField> isStatic() {
+ return DescribedPredicate.describe(
+ "static", field -> field.getModifiers().contains(JavaModifier.STATIC));
+ }
+
+ /**
+ * Match none static modifier of the {@link JavaField}.
+ *
+ * @return A {@link DescribedPredicate} returning true, if and only if the tested {@link
+ * JavaField} has no static modifier.
+ */
+ public static DescribedPredicate<JavaField> isNotStatic() {
+ return DescribedPredicate.describe(
+ "not static", field -> !field.getModifiers().contains(JavaModifier.STATIC));
+ }
+
+ /**
+ * Match the final modifier of the {@link JavaField}.
+ *
+ * @return A {@link DescribedPredicate} returning true, if and only if the tested {@link
+ * JavaField} has the final modifier.
+ */
+ public static DescribedPredicate<JavaField> isFinal() {
+ return DescribedPredicate.describe(
+ "final", field -> field.getModifiers().contains(JavaModifier.FINAL));
+ }
Review comment:
These methods seem like they add more overhead than they save code. Since they're only called from other constructed methods (like `arePublicStaticFinalOfType`) I don't think we really need them or gain anything, but I don't feel too strongly either way.
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/ArchitectureTest.java
##########
@@ -78,4 +79,36 @@ public boolean includes(Location location) {
return !location.matches(SHADED);
}
}
+
+ /**
+ * Exclude locations that contains test classes.
+ *
+ * <p>Note: classes e.g. within jars under flink-test-utils-parent pom module are located as
+ * production code and should be excluded as test classes. Please see {@link
+ * ImportOption.Predefined#DO_NOT_INCLUDE_TESTS}
+ */
+ static class ExcludeTestJarsImportOption implements ImportOption {
Review comment:
Maybe this is a good time to move all of our import options into a separate `ImportOptions` util class, especially since we have a second class now that references them. What do you think?
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/common/Predicates.java
##########
@@ -50,5 +83,78 @@
&& field.getRawType().isEquivalentTo(clazz));
}
+ /**
+ * Tests that the given field is {@code public final} and not {@code static} and of the given
+ * type {@code clazz} .
+ */
+ public static DescribedPredicate<JavaField> arePublicFinalOfTyp(Class<?> clazz) {
Review comment:
Typ → Type
##########
File path: flink-architecture-tests/src/test/resources/archunit.properties
##########
@@ -19,6 +19,11 @@
# By default we allow removing existing violations, but fail when new violations are added.
freeze.store.default.allowStoreUpdate=true
+# must be set to true to allow the creation of a new violation store
+# default is false
+# set to true when new ArchRule has been developed.
+#freeze.store.default.allowStoreCreation=true.
Review comment:
nit:
```suggestion
# Enable this if a new (frozen) rule has been added in order to create the initial store and record the existing violations.
#freeze.store.default.allowStoreCreation=true.
```
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/common/Predicates.java
##########
@@ -40,7 +51,29 @@
.forSubtype();
}
- /** Tests that the given field is {@code public static} and of the given type. */
+ /**
+ * @return A {@link DescribedPredicate} returning true, if and only if the predicate {@link
+ * JavaField} could be found in the {@link JavaClass}.
+ */
+ public static DescribedPredicate<JavaClass> containAnyFieldsInClassHierarchyThat(
+ DescribedPredicate<? super JavaField> predicate) {
+ return new ContainAnyFieldsThatPredicate<>(
+ "fields",
+ new ChainableFunction<JavaClass, Set<JavaField>>() {
+ @Override
+ public Set<JavaField> apply(JavaClass input) {
+ // need to get all fields with the inheritance hierarchy
+ return input.getAllFields();
+ }
+ },
+ predicate);
+ }
+
+ /**
+ * Tests that the given field is {@code public static} and of the given type {@code clazz} .
+ *
+ * <p>Attention: changing the description will add a rule into the stored.rules.
Review comment:
What's the purpose of this comment? This is true for all predicate functions.
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/common/Predicates.java
##########
@@ -40,7 +51,29 @@
.forSubtype();
}
- /** Tests that the given field is {@code public static} and of the given type. */
+ /**
+ * @return A {@link DescribedPredicate} returning true, if and only if the predicate {@link
+ * JavaField} could be found in the {@link JavaClass}.
+ */
+ public static DescribedPredicate<JavaClass> containAnyFieldsInClassHierarchyThat(
Review comment:
```suggestion
public static DescribedPredicate<JavaClass> containsAnyFieldsInClassHierarchyThat(
```
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/common/Predicates.java
##########
@@ -50,5 +83,78 @@
&& field.getRawType().isEquivalentTo(clazz));
}
+ /**
+ * Tests that the given field is {@code public final} and not {@code static} and of the given
+ * type {@code clazz} .
+ */
+ public static DescribedPredicate<JavaField> arePublicFinalOfTyp(Class<?> clazz) {
+ return is(ofType(clazz)).and(isPublic()).and(isFinal()).and(isNotStatic());
+ }
+
+ /**
+ * Tests that the given field is {@code public static final} and of the given type {@code clazz}
+ * .
+ */
+ public static DescribedPredicate<JavaField> arePublicStaticFinalOfTyp(Class<?> clazz) {
+ return arePublicStaticOfType(clazz).and(isFinal());
+ }
+
+ /**
+ * Tests that the given field is {@code public final} and of the given type {@code clazz} with
+ * exactly the given {@code annotationType}.
+ */
+ public static DescribedPredicate<JavaField> arePublicFinalOfTypeWithAnnotation(
+ Class<?> clazz, Class<? extends Annotation> annotationType) {
+ return arePublicFinalOfTyp(clazz).and(annotatedWith(annotationType));
+ }
+
+ /**
+ * Tests that the given field is {@code public static final} and of the given type {@code clazz}
+ * with exactly the given {@code annotationType}.
+ */
+ public static DescribedPredicate<JavaField> arePublicStaticFinalOfTypeWithAnnotation(
+ Class<?> clazz, Class<? extends Annotation> annotationType) {
+ return arePublicStaticFinalOfTyp(clazz).and(annotatedWith(annotationType));
+ }
+
+ /**
+ * Tests that the given field is {@code public static final} and of the given type. It must have
+ * any of the given {@code annotationTypes}
+ */
+ @SafeVarargs
+ public static DescribedPredicate<JavaField> arePublicStaticFinalOfTypeWithOneOfAnnotations(
+ Class<?> clazz, Class<? extends Annotation>... annotationTypes) {
+ return arePublicStaticFinalOfTyp(clazz).and(annotatedWithOneOf(annotationTypes));
+ }
+
private Predicates() {}
+
+ /**
+ * A predicate to determine if a {@link JavaClass} contains one or more {@link JavaField }
+ * matching the supplied predicate.
+ */
+ private static class ContainAnyFieldsThatPredicate<T extends JavaField>
+ extends DescribedPredicate<JavaClass> {
+ private final Function<JavaClass, Set<T>> getFields;
+ private final DescribedPredicate<? super T> predicate;
+
+ ContainAnyFieldsThatPredicate(
+ String fieldDescription,
+ Function<JavaClass, Set<T>> getFields,
+ DescribedPredicate<? super T> predicate) {
+ super("contain any " + fieldDescription + " that " + predicate.getDescription());
Review comment:
```suggestion
super("contains any " + fieldDescription + " that " + predicate.getDescription());
```
Same for the class name itself
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/ArchitectureTest.java
##########
@@ -78,4 +79,36 @@ public boolean includes(Location location) {
return !location.matches(SHADED);
}
}
+
+ /**
+ * Exclude locations that contains test classes.
+ *
+ * <p>Note: classes e.g. within jars under flink-test-utils-parent pom module are located as
+ * production code and should be excluded as test classes. Please see {@link
+ * ImportOption.Predefined#DO_NOT_INCLUDE_TESTS}
+ */
+ static class ExcludeTestJarsImportOption implements ImportOption {
+ private static final Pattern TEST = Pattern.compile(".*-test.*\\.jar.*");
Review comment:
That extra `.*` after `-test` allows for arbitrarily much text to be there before the `.jar`. I think this could easily allow too much. Can we restrict that? What is that `.*` supposed to match currently?
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/common/Predicates.java
##########
@@ -50,5 +83,78 @@
&& field.getRawType().isEquivalentTo(clazz));
}
+ /**
+ * Tests that the given field is {@code public final} and not {@code static} and of the given
+ * type {@code clazz} .
+ */
+ public static DescribedPredicate<JavaField> arePublicFinalOfTyp(Class<?> clazz) {
+ return is(ofType(clazz)).and(isPublic()).and(isFinal()).and(isNotStatic());
+ }
+
+ /**
+ * Tests that the given field is {@code public static final} and of the given type {@code clazz}
+ * .
+ */
+ public static DescribedPredicate<JavaField> arePublicStaticFinalOfTyp(Class<?> clazz) {
+ return arePublicStaticOfType(clazz).and(isFinal());
+ }
+
+ /**
+ * Tests that the given field is {@code public final} and of the given type {@code clazz} with
+ * exactly the given {@code annotationType}.
+ */
+ public static DescribedPredicate<JavaField> arePublicFinalOfTypeWithAnnotation(
+ Class<?> clazz, Class<? extends Annotation> annotationType) {
+ return arePublicFinalOfTyp(clazz).and(annotatedWith(annotationType));
+ }
+
+ /**
+ * Tests that the given field is {@code public static final} and of the given type {@code clazz}
+ * with exactly the given {@code annotationType}.
+ */
+ public static DescribedPredicate<JavaField> arePublicStaticFinalOfTypeWithAnnotation(
+ Class<?> clazz, Class<? extends Annotation> annotationType) {
+ return arePublicStaticFinalOfTyp(clazz).and(annotatedWith(annotationType));
+ }
+
+ /**
+ * Tests that the given field is {@code public static final} and of the given type. It must have
+ * any of the given {@code annotationTypes}
+ */
+ @SafeVarargs
+ public static DescribedPredicate<JavaField> arePublicStaticFinalOfTypeWithOneOfAnnotations(
Review comment:
This method seems to be unused. And it's the only caller of `annotatedWithOneOf` which is then also unused.
##########
File path: flink-architecture-tests/src/test/java/org/apache/flink/architecture/rules/ITCaseRules.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.architecture.rules;
+
+import org.apache.flink.core.testutils.AllCallbackWrapper;
+import org.apache.flink.runtime.testutils.MiniClusterExtension;
+import org.apache.flink.test.util.AbstractTestBase;
+import org.apache.flink.test.util.MiniClusterWithClientResource;
+
+import com.tngtech.archunit.junit.ArchTest;
+import com.tngtech.archunit.lang.ArchRule;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import static com.tngtech.archunit.core.domain.JavaModifier.ABSTRACT;
+import static com.tngtech.archunit.library.freeze.FreezingArchRule.freeze;
+import static org.apache.flink.architecture.common.Conditions.fulfill;
+import static org.apache.flink.architecture.common.GivenJavaClasses.javaClassesThat;
+import static org.apache.flink.architecture.common.Predicates.arePublicFinalOfTypeWithAnnotation;
+import static org.apache.flink.architecture.common.Predicates.arePublicStaticFinalOfTyp;
+import static org.apache.flink.architecture.common.Predicates.arePublicStaticFinalOfTypeWithAnnotation;
+import static org.apache.flink.architecture.common.Predicates.containAnyFieldsInClassHierarchyThat;
+
+/** Rules for Integration Tests. */
+public class ITCaseRules {
+
+ @ArchTest
+ public static final ArchRule INTEGRATION_TEST_ENDING_WITH_ITCASE =
+ freeze(
+ javaClassesThat()
+ .areAssignableTo(AbstractTestBase.class)
+ .and()
+ .doNotHaveModifier(ABSTRACT)
+ .should()
+ .haveSimpleNameEndingWith("ITCase"));
+
+ @ArchTest
+ public static final ArchRule ITCASE_USE_MINICLUSTER =
+ freeze(
+ javaClassesThat()
+ .haveSimpleNameEndingWith("ITCase")
+ .and()
+ .areTopLevelClasses()
+ .and()
+ .doNotHaveModifier(ABSTRACT)
+ .should(
+ fulfill(
+ // JUnit 5 violation check
+ containAnyFieldsInClassHierarchyThat(
+ arePublicStaticFinalOfTyp(
+ MiniClusterExtension.class))
+ .and(
+ containAnyFieldsInClassHierarchyThat(
+ arePublicStaticFinalOfTypeWithAnnotation(
+ AllCallbackWrapper
+ .class,
+ RegisterExtension
+ .class)))
+ // JUnit 4 violation check, which should be
Review comment:
Is the JUnit migration being tracked on JIRA? It'd be better to create a task there to do this rather than waiting for someone to find this in the future and removing it. :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org