Annotation-based dependency definition seems quirky

[Cervator]

Two issues noted on Discord just recently in #architecture relating to something like @Dependencies({"FlexibleMovement", "CoreAssets"})

1. Apparently this is currently needed in child classes if you use a common utility class for some basic setup (see for instance FlexibleMovementTestingEnvironment in https://github.com/Terasology/FlexibleMovement) - the annotation doesn't get inherited by default
2. Transitive dependencies may be honored, but possibly not for assets. This is odd, but in a FlexibleMovement test with the above @Dependencies statement, despite the FM module itself having a dependency tree including CoreAssets the dirt and water block families were not found if CoreAssets wasn't explicitly included in the list

I also sort of wonder why we have to explicitly call out the parent module as a dependency? 🤔 And for that sake - if the parent module's dependencies were to be respected and just loaded normally out of module.txt why would @Dependencies even exist? Is there a case where you'd have a test in a module depend on a different module the test-owning module itself doesn't depend on? Or is it more that we'd want the ability to only activate a subset of the dependency tree? That seems like it would lead to tests differing more than needed from the mainline code.

https://github.com/Terasology/FlexibleMovement/pull/3 was used for some of the testing

[keturn]

I also sort of wonder why we have to explicitly call out the parent module as a dependency?

Yeah, this would clearly be a better developer experience, right?

If a test is using MTE, and the test is defined in a module, that test always wants to include that module as part of the module environment.

(I rarely say “always,” but in this case, I'm having so much trouble thinking of an exception that I bet it would be a long time before we ran in to a use case that required overriding that as a default.)

The hitch is that since Terasology modules have their own categorization independent of Java packages and modules, it's not obvious how a Java Annotation or Jupiter Extension, given a test class, can figure out which Terasology Module it goes with.

[keturn]

This is odd, but in a FlexibleMovement test with the above @Dependencies statement, despite the FM module itself having a dependency tree including CoreAssets the dirt and water block families were not found if CoreAssets wasn't explicitly included in the list.

Oh, I see how it's being used. I think this is arguably Working As Designed.

FlexibleMovement declares no dependency on CoreAssets, optional or otherwise. We require modules to make their dependencies explicit, so their code can't suddenly break if something further up the chain changes to not pull in that dependency anymore.

[Cervator]

Huh - isn't it though? FM depends on Behaviors depends on CoreWorlds depends on CoreAssets

That might just be coincidental though. The FlexibleMovementTestingEnvironment picks dirt and water for :reasons: but could just as well have included assets in its test resources dir so it wouldn't have to depend on something external for simple blocks. So I think that use case for the separate annotation might get the axe as well.

It could just have come about because at the time there were classpath issues and making another annotation was just the easier fix ... ¯\_(ツ)_/¯

[keturn]

FM depends on Behaviors depends on CoreWorlds depends on CoreAssets

Right, so if the Flexible Movement test was failing when some Behaviors method was calling on CoreWorlds, that would be a bug.

But if I understand correctly, the Flexible Movement test is failing when it refers to CoreAssets directly. That seems like it's consistent with a change we made, I think in 2020, requiring modules to state their dependencies explicitly instead of relying on them to be provided transitively.

(If we were relying exclusively on Gradle 6's dependency model, maybe those middle layers would expose those low-levels by including them as API dependencies. But I don't think gestalt-module has that concept.)

[Cervator]

Fair point - and that goes back to

FlexibleMovementTestingEnvironment picks dirt and water for :reasons:

So I guess if FM wants to test directly against CoreAssets it should throw that dependency into module.txt then all will be well

I still thought this would work, but yeah it probably shouldn't. I guess we can keep this issue for point number 1 still.

[DarkWeird]

Main cause failing of FM tests was annotation dependency was at PARENT class of tests. When mark this annotation dependency as @Inherited - all ok. When move this annotation dependency to actual test class - all ok.

dirt and water was there because it simple blocks which have needs property (isPenetrable = false and isLiquid = true) pure engine have only air :)

I agreed with explicit dependencies, when you use them.. ... But this dep is using by test only. if we use this dep in annotation dependencies - we can fail there when module disappears from dependency tree if we add it as optional dependency (like MTE) - it can be understood wrongly.