test: Move ModuleTestingEnvironment to engine-tests

[keturn]

for #4545
and Terasology/ModuleTestingEnvironment#39

from ModuleTestingEnvironment commit adf9b6df0407a62c15092a82960d8711e28bb7b7

Module Changes

No module changes are required at this time. The old ModuleTestingEnvironment code is still there and will keep doing what it does.

But as we make changes to better integrate that code with the other code in engine-tests, we're likely to make some API change somewhere so old MTE won't work with new engines past that point.

Migrating module tests should be straightforward: change your import paths to the ones based in org.terasology.engine.integrationenvironment, and you even get to delete ModuleTestingEnvironment from your module.txt dependencies.

I have pushed test/moveMTE branches to some modules with these changes already.

For Reviewers

The new files added under engine-tests/src/main/org/terasology/engine/integrationenvironment are almost direct copies of the https://github.com/Terasology/ModuleTestingEnvironment sources. I changed a few package paths and names here and there, but the logic is untouched; I recommend you treat them as old code and don't feel like you need to audit all 2600 lines.

There are a few changed files in engine. Most of those are various null checks for things I bumped in to while testing this out. Most of those could go in to a separate PR if you wanted them to, but I think they're small enough that I don't expect that to be what makes or breaks whether this PR gets reviewed.

The most significant engine change is the one to core.TerasologyEngine, detailed in this post from the 9th and the one following it.

How to test

Run engine tests (unitTest and integrationTest), run your module tests.

You could run the game just for funsies but since this is almost entirely changes in engine-tests, there shouldn't be any differences there. Unless I got one of the null checks backwards.

Outstanding before merging

• fix(test): set jacoco excludes for engine-test's unitTest and integrationTest tasks #5013
• fix(ChunkProcessingPipeline): base thread pool size on available processors #5014

[keturn]

discord thread: #Architecture/integration tests

[keturn]

The Bug

Bug: The world generators in engine-tests/src/main/org/terasology/unittest/worlds/ were registered as engine:stub,
not unittest:stub.

But only under MTE. There is another test that shows it as unittest:stub as expected.

The Cause

Part I: The Engine Module

When we were straightening out the upgrade to gestalt v7, we had to revise the idea of the engine module: the set of classes and resources that are not in their own gestalt Module, but that need to be registered with something in the ModuleEnvironment so they can be properly addressed. Without it, gestalt-asset can't load assets from them, and gestalt-module cannot categorize their Components.

One of the factors was TeraNUI. Terasology had recently switched to using the independently packaged version of TeraNUI. It is not distributed as a gestalt module, but it defines asset types and has other classes that must be part of the API accessible to a Terasology ModuleEnvironment.

So I cobbled together this function ModuleManager::loadAndConfigureEngineModule, which rougly defines the contents of the "engine module" as being things in the org.terasology.engine java package namespace, and also anything loaded alongside a certain set of other classes.

// the normal Package Module inclusion test
packageModule.getClassPredicate().test(clazz)
// OR anything loaded from the same classpath entry as that other stuff!
|| config.getUrls().contains(ClasspathHelper.forClass(clazz))

We had also been working on splitting subsystems out from the Engine, and that "list of other classes" grew to include all of the Engine's active subsystems:

 // add all subsystems as engine module parts. (needed for ECS classes loaded from external subsystems)
 allSubsystems.stream().map(Object::getClass).forEach(this::addToClassesOnClasspathsToAddToEngine);

Part II: The Unittest Module

You can't write tests for a module system without letting your tests define at least a few things outside the engine module.

We put those things in a org.terasology.unittest package, but didn't seperate it out any more than that. We still wanted to be able to access those classes from our test cases— the JUnit test runner doesn't allow for loading test cases inside of another ClassLoader like gestalt-module's, which means they've still got to be on the classpath— so it didn't seem worth the bother.

Despite that being neither in the engine module nor anywhere else you'd look for a module by default, some ambitious auto-discovery code would find those classes and then throw a fit about not being able to associate them with a module.

I figured the way to handle that would be for engine-tests to use some extension point to define its unittest module. It had to be something that ran late enough to access the ModuleManager but before that cranky auto-discovery code. What did I find I could run at that time? Subsystem initialization code!

Okay, Until It Wasn't

Somehow that worked okay for a while. The tests that cared about which module the org.terasology.unittest classes got assigned to didn't run with that subsystem, and the tests that ran with that subsystem were content as long as all the discovered classes were assigned to something.

That lasted until I tried to move the ModuleTestingEnvironment classes from the module where they had been to engine-tests. The setup for those tests does use the WithUnittestModule subsystem, and they do expect to be able to reference their WorldGenerator classes by a name that's not "engine".

When those tests run, WithUnittestModule is loaded from the build output of the engine-tests project.

The org.terasology.unittest classes are also loaded from the build output of the engine-tests project.

Suddenly we have a situation where our Frankenstein engine module looks at those classes and says "hey, those come from the same place as a Subsystem, they must be mine!"

The unittest module registered by the WithUnittestModule subsystem would claim them too, but the engine module is at the root of the dependency tree so it gets first dibs.

[keturn]

Proposed Fix

Fix: Do not add all EngineSubsystem class paths to the engine-module.

Because I don't want to get sidetracked by reorganizing DiscordRPCSubsystem, I've named it as a special case:

Terasology/engine/src/main/java/org/terasology/engine/core/TerasologyEngine.java

Lines 108 to 115 in 1176bb1

	/**
	* Subsystem classes that automatically make their classpath part of the engine module.
	* <p>
	* You don't want to add to this! If you need a module, make a module!
	*/
	private static final Set<String> LEGACY_ENGINE_MODULE_POLLUTERS = Set.of(
	"org.terasology.subsystem.discordrpc.DiscordRPCSubSystem"
	);

Alternate Proposals

• Move DiscordRPCSubsystem out of /subsystems and back in to /engine, if it's sure it wants to be in the engine module.
• Move org.terasology.unittest out of /engine-tests to a new gradle subproject.
  This is not a bad idea (as long as you're not allergic to Yet Another Subproject). But it only fixes things for the unittest module. In comparison, reducing the number of ways to automatically trigger changes to the engine module makes things more stable for all additions to modules and subsystems.

[keturn]

This PR is down to one failing test on CI, where it seems to be failing consistently.

I've tried a number of things to reproduce locally, and I've been entirely successful—that is, It Works On My Machine every time.

Help on getting a reproduction environment would be much appreciated.

[keturn]

After some experimentation, I think #5013 and #5014 will improve things enough for these tests to pass.

[keturn]

Move MTE classes to engine-tests

[keturn]

I think I found out what had been breaking things for some modules!

One was an extra comma: MovingBlocks/gestalt#133

And the other was the module under test being in a weird state because it had been picked up from the classpath but its module.txt was not read for other reasons: MovingBlocks/Terasology@3cf39f5

I've pushed test/moveMTE branches for more modules now.

[skaldarnar]

In the current setup MTE is separate from ˋengine-testsˋ and an explicit dependency of modules.

After this PR, MTE is a part of ˋengine-testsˋ and becomes an implicit dependency for every module.

[keturn]

an implicit dependency for every module

Implicit in the sense that it does not appear in module.txt, correct.

More specifically, engine-tests is a test dependency when building a module, as declared here:

Terasology/build-logic/src/main/kotlin/terasology-module.gradle.kts

Lines 53 to 55 in 6508667

	dependencies {
	implementation(group = "org.terasology.engine", name = "engine", version = moduleMetadata.engineVersion())
	testImplementation(group = "org.terasology.engine", name = "engine-tests", version = moduleMetadata.engineVersion())

[keturn]

One more detail on that: Most of what was in MTE wasn't really usable from module runtime (as Terasology/ModuleTestingEnvironment#39 pointed out); it didn't provide any assets or components that had a URI you could look up in a ModuleEnvironment.

There are a few exceptions, the most commonl of which are the WorldGenerators used in test scenarios. Those got moved to a module named unittest with sources in engine-tests/src/main/org/terasology/unittest.

For example: If you had a reference to one of those such as

@UseWorldGenerator("ModuleTestingEnvironment:dummy")

it is now

@UseWorldGenerator("unittest:dummy")

[keturn]

That does mean the unittest module has to be in the ModuleEnvironment your test runs in. The environment @MTEExtension constructs for you does add that implicitly, so you don't need to add "unittest" to your test suite's @Dependencies.

Terasology/engine-tests/src/main/java/org/terasology/engine/integrationenvironment/TestingStateHeadlessSetup.java

Lines 45 to 46 in 4a301c3

	// Include the MTE module to provide world generators and suchlike.
	dependencyNames.add(MTE_MODULE_NAME);