Airblader commented on a change in pull request #18333: URL: https://github.com/apache/flink/pull/18333#discussion_r791583482
########## File path: flink-architecture-tests/README.md ########## @@ -1,43 +1,57 @@ # flink-architecture-tests -This module contains architecture tests using [ArchUnit](https://www.archunit.org/). These tests -reside in their own module in order to control the classpath of which modules should be tested. -Running these tests together (rather than individually per module) allows caching the imported -classes for better performance. +This module contains architecture tests using [ArchUnit](https://www.archunit.org/). Considering the +isolation of classpath and rules for architectural test, there are two top level categories: + +- production code architectural test +- test code architectural test Review comment: ```suggestion isolation of classpath and rules for architectural tests, there are two top level categories: - production code architectural tests - test code architectural tests ``` ########## File path: flink-architecture-tests/README.md ########## @@ -1,43 +1,57 @@ # flink-architecture-tests -This module contains architecture tests using [ArchUnit](https://www.archunit.org/). These tests -reside in their own module in order to control the classpath of which modules should be tested. -Running these tests together (rather than individually per module) allows caching the imported -classes for better performance. +This module contains architecture tests using [ArchUnit](https://www.archunit.org/). Considering the +isolation of classpath and rules for architectural test, there are two top level categories: + +- production code architectural test +- test code architectural test + +Since both of them will need some common ArchUnit extensions, There are three submodules: Review comment: ```suggestion Since both of them will need some common ArchUnit extensions, there are three submodules: ``` ########## File path: flink-architecture-tests/README.md ########## @@ -1,43 +1,57 @@ # flink-architecture-tests -This module contains architecture tests using [ArchUnit](https://www.archunit.org/). These tests -reside in their own module in order to control the classpath of which modules should be tested. -Running these tests together (rather than individually per module) allows caching the imported -classes for better performance. +This module contains architecture tests using [ArchUnit](https://www.archunit.org/). Considering the +isolation of classpath and rules for architectural test, there are two top level categories: + +- production code architectural test +- test code architectural test + +Since both of them will need some common ArchUnit extensions, There are three submodules: + +- flink-architecture-tests-base - contains common ArchUnit extensions that will be used for both + production code architectural test and test code architectural test. +- flink-architecture-tests-production - contains all architectural rules and tests centrally for + production code. Please read the [README](flink-architecture-tests-production/README.md) of the + production code architectural test for further information. +- flink-architecture-tests - contains architectural rules centrally for test code. The architectural + test will be built individually in each submodule where the test code has been developed. Please + read the [README](flink-architecture-tests-test/README.md) of test code architectural test for + further information. Review comment: ```suggestion - flink-architecture-tests-base - contains common ArchUnit extensions that will be used for both production code architectural tests and test code architectural tests. - flink-architecture-tests-production - contains all architectural rules and tests centrally for production code. Please read [flink-architecture-tests-production/README](flink-architecture-tests-production/README.md) for further information. - flink-architecture-tests - contains architectural rules centrally for test code. The architectural tests will be built individually in each submodule where the test code has been developed. Please read the [flink-architecture-tests-test/README](flink-architecture-tests-test/README.md) for further information. ``` ########## File path: flink-architecture-tests/flink-architecture-tests-test/README.md ########## @@ -0,0 +1,55 @@ +# flink-architecture-tests-test + +This submodule contains rules defined centrally for test code architectural tests that will be +developed in each submodule individually where the test code has been developed. + +## Why the architecture test infra for production code and for test code are different + +Comparing to central production code architectural tests, test code architectural tests chose this +central rules plus distributed tests for the following reasons: + +- Reduce the classpath complexity. Developing test code architectural tests centrally will need the + classpath with dependencies to be exactly same in different runtime environments, i.e. maven and + IntelliJ IDEA. This turns out that, on one side, every Flink submodule have to create test jar, + and on the other hand, the test jar must contain all test classes. In the current, there are + already some Flink submodules that created test jar with individual exclusion filter logic which + has conflict with the second prerequisite. Creating more test jars increase the complexity of + dependencies and have impact on the project stability. +- Separation of concern. Developing the test code architectural tests within the submodule will keep + the focus on the submodule's test code. Different from production code test, there are less code + sharing for the test between Flink submodules, therefore less performance issue to be considered. +- DYI and SRP. There are commons when testing the architecture of the test code. These commons can + be defined as common rules centrally in this submodule and used by each individual architectural + test in their own submodules. This module will take the responsibility for central rules while + individual test in each submodule will take the responsibility for the local architectural tests. +- Flexibility. For special cases, further rules could be defined locally in their own submodule and + used locally. + +## How do I write a new architectural rule? + +Please refer to [README](../README.md). + +## How do I initialize and develop the first architectural test for a Flink submodule's test code? + +If there already exists any architectural test in the Flink submodule, you can skip this section and Review comment: This stops mid-sentence. Looks like something got cut off? ########## File path: flink-architecture-tests/flink-architecture-tests-test/README.md ########## @@ -0,0 +1,55 @@ +# flink-architecture-tests-test + +This submodule contains rules defined centrally for test code architectural tests that will be +developed in each submodule individually where the test code has been developed. + +## Why the architecture test infra for production code and for test code are different + +Comparing to central production code architectural tests, test code architectural tests chose this +central rules plus distributed tests for the following reasons: + +- Reduce the classpath complexity. Developing test code architectural tests centrally will need the + classpath with dependencies to be exactly same in different runtime environments, i.e. maven and + IntelliJ IDEA. This turns out that, on one side, every Flink submodule have to create test jar, + and on the other hand, the test jar must contain all test classes. In the current, there are + already some Flink submodules that created test jar with individual exclusion filter logic which + has conflict with the second prerequisite. Creating more test jars increase the complexity of + dependencies and have impact on the project stability. +- Separation of concern. Developing the test code architectural tests within the submodule will keep + the focus on the submodule's test code. Different from production code test, there are less code + sharing for the test between Flink submodules, therefore less performance issue to be considered. +- DYI and SRP. There are commons when testing the architecture of the test code. These commons can + be defined as common rules centrally in this submodule and used by each individual architectural + test in their own submodules. This module will take the responsibility for central rules while + individual test in each submodule will take the responsibility for the local architectural tests. +- Flexibility. For special cases, further rules could be defined locally in their own submodule and + used locally. Review comment: ```suggestion Compared to `flink-architecture-tests-production` which implements and executes architectural rules centrally, `flink-architecture-tests-tests` implements the rules centrally, but they are executed individually in each submodule. This is done for the following reasons: - Reduce the classpath complexity. In order to execute the tests centrally, each submodule would have to provide a test-jar. However, this does not work well in IntelliJ which doesn't support all Maven configurations (such as exclusion filters); this would cause the rules to behave differently when executed from Maven versus IntelliJ. Furthermore, creating more test-jars is not desirable and increases the project complexity. - Separation of concerns. Test code should be viewed as internal to a module, and by separating the violation stores for each submodule this separation can be maintained. Unlike the production code tests there is little shared code between test code, and thus the performance benefits do not apply. - Flexibility. Each submodule can import the generic rules defined in `flink-architecture-tests-tests`, but also add further module-specific architectural tests. ``` ########## File path: flink-architecture-tests/flink-architecture-tests-test/src/test/resources/archunit.properties ########## @@ -0,0 +1,31 @@ +# +# 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. +# + +# By default we allow removing existing violations, but fail when new violations are added. +freeze.store.default.allowStoreUpdate=true + +# Enable this if a new (frozen) rule has been added in order to create the initial store and record the existing violations. Review comment: Since this file will be copied for every submodule, I wonder if we should remove all of the commented out stuff + documentation here and simply refer to the `archunit.properties` of `flink-architecture-tests-production` instead? It's not a big deal either way, I think. ########## File path: flink-connectors/flink-connector-files/src/test/java/org/apache/flink/architecture/TestCodeArchitectureTest.java ########## @@ -0,0 +1,40 @@ +/* + * 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; + +import org.apache.flink.architecture.common.ImportOptions; +import org.apache.flink.architecture.rules.ITCaseRules; + +import com.tngtech.archunit.core.importer.ImportOption; +import com.tngtech.archunit.junit.AnalyzeClasses; +import com.tngtech.archunit.junit.ArchTest; +import com.tngtech.archunit.junit.ArchTests; + +/** Architecture tests for test code. */ +@AnalyzeClasses( + packages = "org.apache.flink.connector.file", + importOptions = { + ImportOption.OnlyIncludeTests.class, + ImportOptions.ExcludeScalaImportOption.class, + ImportOptions.ExcludeShadedImportOption.class + }) +public class TestCodeArchitectureTest { + + @ArchTest public static final ArchTests ITCASE = ArchTests.in(ITCaseRules.class); Review comment: What do you think about creating a class in `flink-architecture-tests-tests` so that the individual submodules just extend from it? That would avoid having to duplicate these "rule imports" in every submodule (especially once there are more than just `ITCaseRules`). ########## File path: flink-architecture-tests/README.md ########## @@ -1,43 +1,57 @@ # flink-architecture-tests -This module contains architecture tests using [ArchUnit](https://www.archunit.org/). These tests -reside in their own module in order to control the classpath of which modules should be tested. -Running these tests together (rather than individually per module) allows caching the imported -classes for better performance. +This module contains architecture tests using [ArchUnit](https://www.archunit.org/). Considering the +isolation of classpath and rules for architectural test, there are two top level categories: + +- production code architectural test +- test code architectural test + +Since both of them will need some common ArchUnit extensions, There are three submodules: + +- flink-architecture-tests-base - contains common ArchUnit extensions that will be used for both + production code architectural test and test code architectural test. +- flink-architecture-tests-production - contains all architectural rules and tests centrally for + production code. Please read the [README](flink-architecture-tests-production/README.md) of the + production code architectural test for further information. +- flink-architecture-tests - contains architectural rules centrally for test code. The architectural + test will be built individually in each submodule where the test code has been developed. Please + read the [README](flink-architecture-tests-test/README.md) of test code architectural test for + further information. + +Following descriptions are valid for building and maintaining the architectural test both for the +production code and the test code. Review comment: ```suggestion The following documentation is valid for building and maintaining architectural tests for both the production and test code. ``` ########## File path: flink-architecture-tests/flink-architecture-tests-production/archunit-violations/5b9eed8a-5fb6-4373-98ac-3be2a71941b8 ########## @@ -93,7 +93,6 @@ org.apache.flink.connector.file.src.util.Pool.recycler(): Returned leaf type org org.apache.flink.connector.file.src.util.Utils.forEachRemaining(org.apache.flink.connector.file.src.reader.BulkFormat$Reader, java.util.function.Consumer): Argument leaf type org.apache.flink.connector.file.src.reader.BulkFormat$Reader does not satisfy: reside outside of package 'org.apache.flink..' or annotated with @Public or annotated with @PublicEvolving or annotated with @Deprecated org.apache.flink.connector.jdbc.JdbcExactlyOnceOptions.builder(): Returned leaf type org.apache.flink.connector.jdbc.JdbcExactlyOnceOptions$JDBCExactlyOnceOptionsBuilder does not satisfy: reside outside of package 'org.apache.flink..' or annotated with @Public or annotated with @PublicEvolving or annotated with @Deprecated org.apache.flink.connector.jdbc.JdbcExecutionOptions.builder(): Returned leaf type org.apache.flink.connector.jdbc.JdbcExecutionOptions$Builder does not satisfy: reside outside of package 'org.apache.flink..' or annotated with @Public or annotated with @PublicEvolving or annotated with @Deprecated -org.apache.flink.connector.jdbc.JdbcSink.exactlyOnceSink(java.lang.String, org.apache.flink.connector.jdbc.JdbcStatementBuilder, org.apache.flink.connector.jdbc.JdbcExecutionOptions, org.apache.flink.connector.jdbc.JdbcExactlyOnceOptions, org.apache.flink.util.function.SerializableSupplier): Argument leaf type org.apache.flink.util.function.SerializableSupplier does not satisfy: reside outside of package 'org.apache.flink..' or annotated with @Public or annotated with @PublicEvolving or annotated with @Deprecated Review comment: This should've failed when the violation was fixed, but that is currently broken. I opened a PR (https://github.com/apache/flink/pull/18504) to fix it. It has no impact on this PR. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
