Redefining ArchUnit tests #3552

onepunchmagne · 2022-09-14T16:50:12Z

Fix #2921

Shared kernels cannot depend on bounded contexts
Bounded context can calls each others only using secondary to primary Java adapters communications
Domain cannot use anything but its self-domain, Java vanilla features or some common utils libraries
Application cannot call infrastructure
Primary cannot directly call secondary
Secondary cannot call any application service

pascalgrimaud · 2022-09-14T16:54:44Z

Does it fix #2921 @onepunchmagne ?

onepunchmagne · 2022-09-14T16:55:13Z

Work left:

an issue fixed in ArchUnit 1.0.0-rc1 seems to be still an issue for checking primary Java adapters are called only by secondaries to fix Hexagonal Arch: add new test - secondary can call primary from another context #2921
@DamnClin @pascalgrimaud I have a problem understanding why module is a shared kernel and project a bounded context -> we have an architecture issue on FileSystemJHipsterModulesRepository calling JavaProjects

DamnClin · 2022-09-14T16:59:23Z

I have a problem understanding why module is a shared kernel and project a bounded context

Not sure to understand the question so I can only answer by saying that module is a shared kernel and project is a business context (I don't think we have a bounded context annotation as this would be really wrong and missleading !)

we have an architecture issue on FileSystemJHipsterModulesRepository calling JavaProjects

This is not a bug, JavaProject is a primary adapter, it's totally normal

onepunchmagne · 2022-09-14T19:43:10Z

I don't think we have a bounded context annotation as this would be really wrong and missleading !

Here is the thing that should be discussed, the annotation is called @BusinessContext but the tests say otherwise, here is the behavior implemented in the existing tests (before this PR): "Shared kernels should not have dependencies to bounded contexts"
Moreover in the hexagonal-architecture.md we explain in the same paragraph:

So, first things first: an application is made of multiple "hexagons", one for each Bounded Context. (Yes, sometimes you can have only one but this is an exception). We usually have each Bounded Context as root packages in the application.

my_business_context: root package for the context (naming depends on your technology naming conventions)

I think @pascalgrimaud and @hdurix aggried that JHLite generates bounded contexts from DDD, but this is very confusing in JHLite code itself. As a user of JHLite our project team also understood that @BusinessContext annotation was from "bounded context" (without understanding why it was not named exactly after the DDD strategic patterns). Also the other annotation coming in pair is the @SharedKernel so it made perfectly sense for JHLite users (not contributors). But for contributors point of view, it is indeed misleading, I can't understand our package-info.java descriptors.

If I may, I would suggest we rename to @BoundedContext explicitely to stick to a well-documented notion that would wear a conviction along with @SharedKernel. But that we remove/redefine the existing package-info.java that seems to randomly wear the annotations. (ticket is here #3540)

DamnClin · 2022-09-15T15:26:29Z

Ok, if I'm the only one finding that misleading let's rename it

codecov · 2022-09-22T22:06:42Z

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (814712a) compared to base (40c6e96).
Patch has no changes to coverable lines.

❗ Current head 814712a differs from pull request most recent head 2fcac00. Consider uploading reports for the commit 2fcac00 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##                main     #3552   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
+ Complexity      2093      2074   -19     
===========================================
  Files            545       541    -4     
  Lines           8930      8876   -54     
  Branches         175       174    -1     
===========================================
- Hits            8930      8876   -54

Impacted Files	Coverage Δ
...re/primary/SpringdocOauth2ModuleConfiguration.java	`100.00% <ø> (ø)`
...va/tech/jhipster/lite/common/domain/Memoizers.java	`100.00% <0.00%> (ø)`
...e/module/domain/javadependency/JavaDependency.java	`100.00% <0.00%> (ø)`
...dule/domain/javadependency/JavaDependencyType.java	`100.00% <0.00%> (ø)`
...le/domain/javabuild/command/AddJavaDependency.java	`100.00% <0.00%> (ø)`
...or/client/common/domain/ClientsModulesFactory.java	`100.00% <0.00%> (ø)`
...astructure/secondary/javadependency/MavenType.java	`100.00% <0.00%> (ø)`
.../secondary/javadependency/MavenCommandHandler.java	`100.00% <0.00%> (ø)`
...customjhlite/domain/CustomJHLiteModuleFactory.java	`100.00% <0.00%> (ø)`
...y/FileSystemProjectJavaDependenciesRepository.java	`100.00% <0.00%> (ø)`
... and 25 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

DamnClin · 2022-09-23T07:12:39Z

src/main/java/tech/jhipster/lite/project/package-info.java

@@ -1,2 +1,2 @@
-@tech.jhipster.lite.BusinessContext
+@tech.jhipster.lite.SharedKernel


Project is not a shared kernel, I don't understand this change

If possible I'd like to take also this job but in issue #3540
because sticking to the architecture tests, we have a failure because project is called directly by module which is tagged as a shared kernel.

To be honest I have absolutely no clue currently of which package is a shared kernel and which is a "business" context, but there is something to do in this separate issue I think to realign the dependencies.

This is a mistake :D project is a business context and module is a SharedKernel, what is the link?

Ok, so the whole thing about SharedKernels.shouldNotDependOnBoundedContexts is wrong I suppose, I delete it. A shared kernel can actually call bounded contexts following the same rule as passing through its secondary adapter to call the bounded context primary adapter, is that right?

Thus the shouldBeAnHexagonalArchitecture test should also be applied to shared kernel in the exact same way. Please check new change where I applied this logic

src/main/resources/generator/server/javatool/arch/HexagonalArchTest.java.mustache

DamnClin · 2022-09-23T07:19:21Z

src/main/resources/generator/server/javatool/arch/HexagonalArchTest.java.mustache

      classes()
        .that()
-        .resideInAnyPackage(context + ".domain..")
+        .resideInAPackage(".domain..")


I don't get it, here we are allowing domain to depend on any domain no? No more check for domain dependencies to other domains?

Yes but there were a lot of overlaying tests so I tried defining different separate responsibility tests such as:

BoundedContexts.shouldNotDependOnOtherBoundedContextDomains which will verify our hexagons co-existence and boundaries

Domain.shouldNotDependOnOutside which does not even know it co-exists with other hexagons, it simply check its dependencies to "outside" of a domain package

DamnClin · 2022-09-23T11:54:40Z

This can give some god hints: https://github.com/whiskeysierra/archunit-hexagonal/blob/trunk/src/main/kotlin/io/github/whiskeysierra/archunit/hexagonal/HexagonalArchitecture.kt

onepunchmagne · 2022-09-23T12:22:11Z

This can give some god hints: https://github.com/whiskeysierra/archunit-hexagonal/blob/trunk/src/main/kotlin/io/github/whiskeysierra/archunit/hexagonal/HexagonalArchitecture.kt

Thanks for the correction you're right, I used the hint trying to define some basics rules, I get that it's hard to define "the" hexagonal architecture test but it should probably serve as a good base for people to update according to their definition.

src/main/resources/generator/server/javatool/arch/HexagonalArchTest.java.mustache

* Shared kernels cannot depend on bounded contexts * Bounded context can calls each others only using secondary to primary Java adapters communications * Domain cannot use anything but its self-domain, Java vanilla features or some common utils libraries * Application cannot call infrastructure * Primary cannot directly call secondary * Secondary cannot call any application service Remove empty packages/"todo" contexts to restart from a clean implementation matching expected architecture.

onepunchmagne · 2022-12-18T17:31:01Z

Claimed https://opencollective.com/generator-jhipster/expenses/114216

DanielFran · 2022-12-18T18:00:33Z

@onepunchmagne approved

onepunchmagne changed the title ~~Redefining ArchUnit tests~~ Draft: Redefining ArchUnit tests Sep 14, 2022

hdurix marked this pull request as draft September 14, 2022 16:52

onepunchmagne changed the title ~~Draft: Redefining ArchUnit tests~~ Redefining ArchUnit tests Sep 14, 2022

pascalgrimaud added area: enhancement 🔧 generator: internal theme: tests labels Sep 14, 2022

onepunchmagne mentioned this pull request Sep 15, 2022

Exclude self from violations of onlyAccessedBy(..) by default TNG/ArchUnit#3

Closed

onepunchmagne force-pushed the redefining-archunit-tests branch from 430c60f to 814712a Compare September 22, 2022 21:52

DamnClin requested changes Sep 23, 2022

View reviewed changes

onepunchmagne force-pushed the redefining-archunit-tests branch from 814712a to ffb71fd Compare September 23, 2022 11:50

onepunchmagne force-pushed the redefining-archunit-tests branch from ffb71fd to ba03b58 Compare September 23, 2022 12:18

onepunchmagne force-pushed the redefining-archunit-tests branch from ba03b58 to f6a04b0 Compare September 23, 2022 12:35

onepunchmagne marked this pull request as ready for review September 23, 2022 12:49

onepunchmagne requested a review from DamnClin September 23, 2022 12:50

onepunchmagne force-pushed the redefining-archunit-tests branch from f6a04b0 to d38766e Compare September 23, 2022 16:13

DamnClin previously approved these changes Sep 27, 2022

View reviewed changes

src/main/resources/generator/server/javatool/arch/HexagonalArchTest.java.mustache Outdated Show resolved Hide resolved

onepunchmagne dismissed DamnClin’s stale review via 73cddb0 September 27, 2022 18:08

onepunchmagne force-pushed the redefining-archunit-tests branch from d38766e to 73cddb0 Compare September 27, 2022 18:08

onepunchmagne force-pushed the redefining-archunit-tests branch from 73cddb0 to 2fcac00 Compare September 27, 2022 18:09

DamnClin enabled auto-merge September 27, 2022 18:23

DamnClin merged commit 04fe364 into jhipster:main Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redefining ArchUnit tests #3552

Redefining ArchUnit tests #3552

onepunchmagne commented Sep 14, 2022 •

edited

pascalgrimaud commented Sep 14, 2022

onepunchmagne commented Sep 14, 2022 •

edited

DamnClin commented Sep 14, 2022

onepunchmagne commented Sep 14, 2022 •

edited

DamnClin commented Sep 15, 2022

codecov bot commented Sep 22, 2022 •

edited

DamnClin Sep 23, 2022

onepunchmagne Sep 23, 2022

DamnClin Sep 23, 2022

onepunchmagne Sep 23, 2022

DamnClin Sep 23, 2022

onepunchmagne Sep 23, 2022

DamnClin commented Sep 23, 2022

onepunchmagne commented Sep 23, 2022

onepunchmagne commented Dec 18, 2022

DanielFran commented Dec 18, 2022

		@@ -1,2 +1,2 @@
		@tech.jhipster.lite.BusinessContext
		@tech.jhipster.lite.SharedKernel

Redefining ArchUnit tests #3552

Redefining ArchUnit tests #3552

Conversation

onepunchmagne commented Sep 14, 2022 • edited

pascalgrimaud commented Sep 14, 2022

onepunchmagne commented Sep 14, 2022 • edited

DamnClin commented Sep 14, 2022

onepunchmagne commented Sep 14, 2022 • edited

DamnClin commented Sep 15, 2022

codecov bot commented Sep 22, 2022 • edited

Codecov Report

DamnClin Sep 23, 2022

Choose a reason for hiding this comment

onepunchmagne Sep 23, 2022

Choose a reason for hiding this comment

DamnClin Sep 23, 2022

Choose a reason for hiding this comment

onepunchmagne Sep 23, 2022

Choose a reason for hiding this comment

DamnClin Sep 23, 2022

Choose a reason for hiding this comment

onepunchmagne Sep 23, 2022

Choose a reason for hiding this comment

DamnClin commented Sep 23, 2022

onepunchmagne commented Sep 23, 2022

onepunchmagne commented Dec 18, 2022

DanielFran commented Dec 18, 2022

onepunchmagne commented Sep 14, 2022 •

edited

onepunchmagne commented Sep 14, 2022 •

edited

onepunchmagne commented Sep 14, 2022 •

edited

codecov bot commented Sep 22, 2022 •

edited