Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce more sophisticated ArchUnit architecture test #17520

Closed
codecholeric opened this issue Jan 8, 2022 · 4 comments · Fixed by #17521 or #17548
Closed

Introduce more sophisticated ArchUnit architecture test #17520

codecholeric opened this issue Jan 8, 2022 · 4 comments · Fixed by #17521 or #17548

Comments

@codecholeric
Copy link
Contributor

Overview of the feature request

I was wondering if it would make sense to replace the predefined ArchUnit test with one that is a little bit more sophisticated to give a better demo of what it can do. One thing that comes to mind would be to define a layered architecture on the technical components (as there is not too much domain to go on in a completely generic application). At the moment this could e.g. be such a test:

@AnalyzeClasses(packagesOf = ${root_class}.class)
class TechnicalStructureTest {

    @ArchTest
    static final ArchRule respects_technical_architecture_layers = layeredArchitecture()
        .layer("Config").definedBy("..config..")
        .layer("Web").definedBy("..web..")
        .layer("Security").definedBy("..security..")
        .layer("Service").definedBy("..service..")
        .layer("Persistence").definedBy("..repository..")
        .layer("Domain").definedBy("..domain..")

        .whereLayer("Web").mayNotBeAccessedByAnyLayer()
        .whereLayer("Service").mayOnlyBeAccessedByLayers("Web", "Config")
        .whereLayer("Persistence").mayOnlyBeAccessedByLayers("Service", "Security", "Web", "Config")
        .whereLayer("Domain").mayOnlyBeAccessedByLayers("Persistence", "Service", "Security", "Web", "Config");
}

(I just looked which allowed dependencies the test needs to be green, so I suppose this is the target architecture 😉)

Motivation for or Use Case

While forbidding dependencies repository -> web and service -> web obviously makes sense it still leaves a lot of holes. For example it would not complain if we create root_package.SomeUtils and then call repository -> SomeUtils -> web. Thus, I think in general it's a better approach to white-list what should be allowed instead of blacklist a specific sort of dependency.

Also, if we have the dependency archunit-junit5 on the classpath it makes sense to use the JUnit 5 support (@AnalyzeClasses, @ArchTest, ...) instead of importing the classes manually with new ClassFileImporter(). Because, for the current way of using ArchUnit the core dependency archunit would already be good enough.

If you think this makes sense I would be happy to create some PR, I just didn't want to go for it without discussion.

@pascalgrimaud
Copy link
Member

For your information, we have a complete Arch Unit Test for JHipster Lite project

See https://github.com/jhipster/jhipster-lite/blob/main/src/test/java/tech/jhipster/lite/HexagonalArchTest.java

These tests are used to respect the Hexagonal Architecture, so thanks so much for this wonderful project.

Then, I'm sure we can improve the one, generated by generator-jhipster

@codecholeric
Copy link
Contributor Author

I can just create a PR with the above suggestion then to check if it passes on all environments or if there are technical components I overlooked (after all the combination of all possible generation options is pretty complex meanwhile as I found out when I tried to refactor the test infrastructure in the past 😂)

In the end for any custom app it usually makes sense to extend these tests once the application grows anyway, but I think a layered architecture is a nice starter for a new project 🤷‍♂️ (would be kind of cool to be able to select layered architecture vs hexagonal architecture on creation 😂 But I guess that would blow up the complexity, as it would also require the classes to be generated in a different way to invert some dependencies, etc.)

@mshima
Copy link
Member

mshima commented Jan 8, 2022

This would be a great start for v8.

@pascalgrimaud
Copy link
Member

@codecholeric : I tried to do it in main generator-jhipster last summer but I was stuck by the complexity, so I gave up, and start from scratch with JHipster Lite.

That's the goal of JHipster Lite, to generate projects with Hexagonal Architecture. And the cool result is our coverage is at 💯% 😎

codecholeric added a commit to codecholeric/generator-jhipster that referenced this issue Jan 8, 2022
Since the generated application is modelled after a simple layered architecture we can provide a generated ArchUnit test that uses `Architectures.layeredArchitecture()` as a nice example of a higher abstraction architecture rule. This will assert even more dependencies for correctness than before (for example no dependencies from repositories to services). Furthermore, by specifying an exact list of which layer is allowed to access which other layer, instead of forbidding specific dependencies from one layer to another, we remove one potential source of accidental violation. E.g., by forbidding `service -> web` we can still have accesses like `services -> some_utils_no_one_cares_about -> web`. If we specify that no other layer may access web (because all accesses are via REST, etc., and not from other Java code), we can make sure that there are no accidental dependencies through some unexpected packages.

 I've also replaced the manual import via `new ClassFileImporter()...importPackages(..)` by ArchUnit's JUnit 5 support. The dependency was already correct anyway, and it allows to write the test a little more concisely and automatically brings caching between multiple rules that import the same classes.

Resolves: jhipster#17520
codecholeric added a commit to codecholeric/generator-jhipster that referenced this issue Jan 8, 2022
Since the generated application is modelled after a simple layered architecture we can provide a generated ArchUnit test that uses `Architectures.layeredArchitecture()` as a nice example of a higher abstraction architecture rule. This will assert even more dependencies for correctness than before (for example no dependencies from repositories to services). Furthermore, by specifying an exact list of which layer is allowed to access which other layer, instead of forbidding specific dependencies from one layer to another, we remove one potential source of accidental violation. E.g., by forbidding `service -> web` we can still have accesses like `services -> some_utils_no_one_cares_about -> web`. If we specify that no other layer may access web (because all accesses are via REST, etc., and not from other Java code), we can make sure that there are no accidental dependencies through some unexpected packages.

 I've also replaced the manual import via `new ClassFileImporter()...importPackages(..)` by ArchUnit's JUnit 5 support. The dependency was already correct anyway, and it allows to write the test a little more concisely and automatically brings caching between multiple rules that import the same classes.

Resolves: jhipster#17520
codecholeric added a commit to codecholeric/generator-jhipster that referenced this issue Jan 9, 2022
Since the generated application is modelled after a simple layered architecture we can provide a generated ArchUnit test that uses `Architectures.layeredArchitecture()` as a nice example of a higher abstraction architecture rule. This will assert even more dependencies for correctness than before (for example no dependencies from repositories to services). Furthermore, by specifying an exact list of which layer is allowed to access which other layer, instead of forbidding specific dependencies from one layer to another, we remove one potential source of accidental violation. E.g., by forbidding `service -> web` we can still have accesses like `services -> some_utils_no_one_cares_about -> web`. If we specify that no other layer may access web (because all accesses are via REST, etc., and not from other Java code), we can make sure that there are no accidental dependencies through some unexpected packages.

 I've also replaced the manual import via `new ClassFileImporter()...importPackages(..)` by ArchUnit's JUnit 5 support. The dependency was already correct anyway, and it allows to write the test a little more concisely and automatically brings caching between multiple rules that import the same classes.

Resolves: jhipster#17520
codecholeric added a commit to codecholeric/generator-jhipster that referenced this issue Jan 9, 2022
Since the generated application is modelled after a simple layered architecture we can provide a generated ArchUnit test that uses `Architectures.layeredArchitecture()` as a nice example of a higher abstraction architecture rule. This will assert even more dependencies for correctness than before (for example no dependencies from repositories to services). Furthermore, by specifying an exact list of which layer is allowed to access which other layer, instead of forbidding specific dependencies from one layer to another, we remove one potential source of accidental violation. E.g., by forbidding `service -> web` we can still have accesses like `services -> some_utils_no_one_cares_about -> web`. If we specify that no other layer may access web (because all accesses are via REST, etc., and not from other Java code), we can make sure that there are no accidental dependencies through some unexpected packages.

 I've also replaced the manual import via `new ClassFileImporter()...importPackages(..)` by ArchUnit's JUnit 5 support. The dependency was already correct anyway, and it allows to write the test a little more concisely and automatically brings caching between multiple rules that import the same classes.

Resolves: jhipster#17520
mshima pushed a commit that referenced this issue Jan 12, 2022
Since the generated application is modelled after a simple layered architecture we can provide a generated ArchUnit test that uses `Architectures.layeredArchitecture()` as a nice example of a higher abstraction architecture rule. This will assert even more dependencies for correctness than before (for example no dependencies from repositories to services). Furthermore, by specifying an exact list of which layer is allowed to access which other layer, instead of forbidding specific dependencies from one layer to another, we remove one potential source of accidental violation. E.g., by forbidding `service -> web` we can still have accesses like `services -> some_utils_no_one_cares_about -> web`. If we specify that no other layer may access web (because all accesses are via REST, etc., and not from other Java code), we can make sure that there are no accidental dependencies through some unexpected packages.

 I've also replaced the manual import via `new ClassFileImporter()...importPackages(..)` by ArchUnit's JUnit 5 support. The dependency was already correct anyway, and it allows to write the test a little more concisely and automatically brings caching between multiple rules that import the same classes.

Resolves: #17520
@pascalgrimaud pascalgrimaud added this to the 7.6.0 milestone Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment