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

MTE tests are broken after gestalt v7 migration #4757

Closed
skaldarnar opened this issue Jun 12, 2021 · 10 comments · Fixed by Terasology/ModuleTestingEnvironment#55
Closed

MTE tests are broken after gestalt v7 migration #4757

skaldarnar opened this issue Jun 12, 2021 · 10 comments · Fixed by Terasology/ModuleTestingEnvironment#55
Labels
Category: Test/QA Requests, Issues and Changes targeting tests and quality assurance Type: Bug Issues reporting and PRs fixing problems
Milestone

Comments

@skaldarnar
Copy link
Member

Currently the module integration tests using Module Testing Environment (MTE) are all broken.

Locally, they fail with a VerifyException as the testing environment is unable to find a module for a sepcific component. As far as I can observe, this is not always the same component, but the stacktrace looks quite similar every time.

com.google.common.base.VerifyException: Could not find module for MappedType class org.terasology.unittest.stubs.MappedTypeComponent
	at com.google.common.base.Verify.verify(Verify.java:124)
	at com.google.common.base.Verify.verifyNotNull(Verify.java:500)
	at org.terasology.engine.core.bootstrap.EnvironmentSwitchHandler.registerComponents(EnvironmentSwitchHandler.java:197)
	at org.terasology.engine.core.bootstrap.EnvironmentSwitchHandler.handleSwitchToGameEnvironment(EnvironmentSwitchHandler.java:107)
	at org.terasology.engine.core.TerasologyEngine.initialize(TerasologyEngine.java:227)
	at org.terasology.moduletestingenvironment.ModuleTestingEnvironment.createEngine(ModuleTestingEnvironment.java:409)
	at org.terasology.moduletestingenvironment.ModuleTestingEnvironment.createHeadlessEngine(ModuleTestingEnvironment.java:383)
	at org.terasology.moduletestingenvironment.ModuleTestingEnvironment.createHost(ModuleTestingEnvironment.java:451)
	at org.terasology.moduletestingenvironment.ModuleTestingEnvironment.setup(ModuleTestingEnvironment.java:167)
	at org.terasology.moduletestingenvironment.MTEExtension.beforeAll(MTEExtension.java:93)

In CI, the tests also fail but with a different error message. For instance, in Health build #62 the tests fail with a NoClassDefFoundError for PathManager.

java.lang.NoClassDefFoundError: org/terasology/engine/core/paths/PathManager
	at org.terasology.moduletestingenvironment.ModuleTestingEnvironment.createEngine(ModuleTestingEnvironment.java:401)
	at org.terasology.moduletestingenvironment.ModuleTestingEnvironment.createHeadlessEngine(ModuleTestingEnvironment.java:383)
	at org.terasology.moduletestingenvironment.ModuleTestingEnvironment.createHost(ModuleTestingEnvironment.java:451)
	at org.terasology.moduletestingenvironment.ModuleTestingEnvironment.setup(ModuleTestingEnvironment.java:167)
	at org.terasology.moduletestingenvironment.MTEExtension.beforeAll(MTEExtension.java:93)
@skaldarnar skaldarnar added Type: Bug Issues reporting and PRs fixing problems Category: Test/QA Requests, Issues and Changes targeting tests and quality assurance labels Jun 12, 2021
@keturn
Copy link
Member

keturn commented Jun 18, 2021

In the first case, where EnvironmentSwitchHandler.registerComponents throws "Could not find module for MappedType class o.t.unittest.stubs.MappedTypeComponent," environment.getSubtypesOf(Component) is returning these components in unittest.stubs though the environment does not contain the unittest module.

It looks like that ModuleManager.environment is in its default engine-module-only state, the other modules named in the test class's @Dependencies annotation not yet added to it.

I think the thing to investigate here is to compare to the integration tests in engine-tests, because those aren't failing and I think at least some of them do something like MTE.createEngine, right?

@keturn keturn added this to the v5.1.0 milestone Jun 18, 2021
@keturn
Copy link
Member

keturn commented Jun 19, 2021

Well, I think the reason that engine-test's TerasologyTestingEnvironment and HeadlessEnvironment don't have that same failure in TerasologyEngine.initalize is because they never call TerasologyEngine.initalize!

They've got gobs of initialization code that's essentially a parallel implementation of the stuff that TerasologyEngine.initialize does, but with mocks and whatnot sprinkled in.

But it looks like it should end up at some of the same register-component code, e.g. from HeadlessEnvironment's call of EntitySystemSetupUtil.addEntityManagementRelatedClasses. The difference is that it sets up the ModuleManager differently (including the "unittest" module) before it gets to that point.

@keturn
Copy link
Member

keturn commented Jun 19, 2021

This is some of the engine initialization:

initManagers();
initSubsystems();
changeStatus(TerasologyEngineStatus.INITIALIZING_ASSET_MANAGEMENT);
initAssets();
EnvironmentSwitchHandler environmentSwitcher = new EnvironmentSwitchHandler();
rootContext.put(EnvironmentSwitchHandler.class, environmentSwitcher);
environmentSwitcher.handleSwitchToGameEnvironment(rootContext);
postInitSubsystems();
verifyInitialisation();

here's one idea: make an engine-test subsystem that registers its weird unittest module. Because initSubsystems comes after initManagers but before that handleSwitchToGameEnvironment.

That handleSwitchToGameEnvironment method kinda stands out. I find it dates back to 1860861, when it was named applyModules. The registerComponents part came a bit later in ff1125c as part of #1740.

The thing that's bugging me is that I don't understand why this "switch to game environment" code is being run this early. We haven't selected game modules yet, so I'm not sure why we're doing all this stuff that depends on the module environment. It is called again when we go to actually load a game.

Is it required to load assets that are part of the GUI or something like that?

@jdrueckert
Copy link
Member

I think the thing to investigate here is to compare to the integration tests in engine-tests, because those aren't failing and I think at least some of them do something like MTE.createEngine, right?

Just did a quick grep in engine-tests and I did find neither createEngine nor ModuleTesting nor MTE. Do you recall where you thought you might've seen something like that?

The difference is that it sets up the ModuleManager differently (including the "unittest" module) before it gets to that point.

On MTE or on TTE side? If on TTE, should me maybe just add that to the setup on MTE, too?

@keturn
Copy link
Member

keturn commented Jun 19, 2021

a quick grep in engine-tests and I did find neither

Indeed, I was wrong about that. engine-tests does not use TerasologyEngine for TTE after all.

On MTE or on TTE side?

TTE has customized its setup. MTE can't do so in exactly the same way because MTE doesn't have its own version of all those initialization methods; it lets TerasologyEngine do that.

@keturn keturn modified the milestones: v5.1.0, v5.0.0 Jun 19, 2021
@jdrueckert
Copy link
Member

TTE has customized its setup. MTE can't do so in exactly the same way because MTE doesn't have its own version of all those initialization methods; it lets TerasologyEngine do that.

So TTE does not let TerasologyEngine do it, but does it in a customized way?
Is TerasologyEngine used by any other tests?
Do we know yet in how far the customized way of TTE diverge from what TerasologyEngine does for MTE?
Would it make sense to adjust whatever TerasologyEngine does so we can have both TTE and MTE rely on that and have it parametrizable to support scenarios with and without for instance the "unittest" module?

@keturn
Copy link
Member

keturn commented Jun 19, 2021

Yep. I don't know how far TTE has diverged from TerasologyEngine, but given how much code there is, I assume it has diverged and will only widen as time goes on.

One of the ways that we have of parameterizing Engine is with subsystems. That's one idea I came up with yesterday but I don't know if it's a good idea.

I think it would be sufficient for this particular failure we're having at the moment. Of course, it would be nice if the steps we take for this failure are steps toward fixing the problem of all TTE's potentially-divergent code, and I haven't thought that far ahead.

Another option would be to make the unittest module less of quirky thing. Put it in /modules/, or maybe have another directory /test-modules/ we add to the module path during tests. We'd want it to stay in the same repo so it can't get out of sync with engine-tests.

@jdrueckert
Copy link
Member

One of the ways that we have of parameterizing Engine is with subsystems. That's one idea I came up with yesterday but I don't know if it's a good idea.

I'd like to ping @DarkWeird on that one.

Another option would be to make the unittest module less of quirky thing. Put it in /modules/, or maybe have another directory /test-modules/ we add to the module path during tests. We'd want it to stay in the same repo so it can't get out of sync with engine-tests.

This kind of bring me back to the idea of a "developer tooling" distro we talked about a while ago. Basically a distro where modules for testing such as MTE or modules adding debug tooling or so could live...

@skaldarnar
Copy link
Member Author

All this talk about diverging code makes me wonder

  • a) why do have diverging code for tests? aren't we testing "the real thing"? how many issue just come from having tests run in a different environment than actual game?
  • b) why do we have multiple lines (of diverging code)? can we merge MTE and TTE and only have a single engine variant for testing (in case we need it).

@keturn
Copy link
Member

keturn commented Jun 25, 2021

Yes, that's the subject of #4545.

But for the issue at hand, the TTE tests are the ones that are passing, so removing it for the sake of de-duplication won't get us out of the current mess.

I'm going to try implementing the subsystem thing I suggested in #4757 (comment)
I have some misgivings about it, but at the moment I think it's my best shot at trying something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Test/QA Requests, Issues and Changes targeting tests and quality assurance Type: Bug Issues reporting and PRs fixing problems
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants