test(TypeHandlerLibrary): RuntimeDelegatingTypeHandler test failure

[keturn]

When running tests under JDK 11, there's one engine test that fails.

o.t.persistence.typeHandling.coreTypes.RuntimeDelegatingTypeHandlerTest#testDeserializeSub

The other four test cases in RuntimeDelegatingTypeHandlerTest continue to be fine.

There were "warning: raw use of parameterized type" messages all over that test file, in trying to understand the tests, the first thing I did was clean those up.

I haven't fixed the test yet, but I did get one of the other four to fail!

The place I've ended up is here:

Terasology/engine/src/main/java/org/terasology/persistence/typeHandling/coreTypes/RuntimeDelegatingTypeHandler.java

Lines 89 to 94 in 37debd9

	if (!(typeHandler instanceof ObjectFieldMapTypeHandler) &&
	typeHandler.getClass().equals(delegateHandler.getClass())) {
	// Both handlers are of same type and will do the same thing,
	// use delegateHandler which might have more info
	return delegateHandler;
	}

that th.getClass().equals(dh.getClass()) is coming back true for the test's baseTypeHandler and subTypeHandler, so it uses the baseTypeHandler (as that was the one given to RuntimeDelegatingTypeHandler construction), but the test is written specifically to assert the subTypeHandler is used.

So, did I

• uncover a bug in RuntimeDelegatingTypeHandler.serializeNonNull?
• accidentally change (i.e. break) the semantics of the test when I changed things around to not use unparameterized types?
• stumble on to some other type of distinction the test should be making? (e.g. does it need the subTypeHandler to be of a distinctly different class than the baseTypeHandler, not just a different instance with different type parameters).

and why were the test results different between javas 8 and 11?

I'll run some things under java 8 to see if I can find out about that, but I need input on the rest.

[keturn]

okay, now that test suite behaves the same way under JDK 8 and 11, which I appreciate.

The code under test looks like it was introduced in #3535, if that rings any bells.

[DarkWeird]

You sure?

[keturn]

Reports methods taking a variable number of arguments, also known as varargs methods. Such methods are not supported under Java 1.4 or earlier JVMs. The quickfix of this inspection replaces a variable argument parameter with the equivalent array parameter. Relevant arguments in calls to the method are wrapped in an array initializer expression.

We don't have to worry about compat with Java 1.4, so I think the only other reason to have it enabled is so that quickfix-action of replace-variable-argument-parameter-with-array shows up when you have the cursor on varargs.

we can keep it at default if that's useful, but so far I've found it to be an unhelpful suggestion.

[keturn]

EvilTak provided some comments about this today: https://discord.com/channels/270264625419911192/696830442065625118/762033934925561866

[keturn]

That was a big merge because it has been a while and all of the TypeHandlerLibrary code moved, but it seems to still apply.

[keturn]

@eviltak wrote:

If my hunch on how the JUnit Mockito extension works is right, then simply replacing the type of the test class constructor parameter @Mock TypeHandler<Sub> subTypeHandler with @Mock SubHandler subTypeHandler should do the trick.

No such luck. It just makes the test fail a little earlier, deserialize not returning a value at all.

Terasology/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/RuntimeDelegatingTypeHandlerTest.java

Lines 131 to 133 in caf9721

	Optional<Base> result = runtimeDelegatingTypeHandler.deserialize(persistedSub);
	assertTrue(result.isPresent());

[keturn]

After stubbing out the SerializationSandbox so we're not relying on how this test case builds an instance of Reflections, and paying a bit more attention to whether we've stubbed the Type or Class<> variant of methods, I think we have tests that pass in both Java 8 and Java 11 builds. 😌

[keturn]

Java Version 11

[Cervator]

Seems fine here, CI finally passes, huzzah! :-)

Discord chatter seemed pretty confident about this, I'm not at that level but can at least confirm I looked at it 👍

[DarkWeird]

Looks right.
Works(i love tests)

[keturn]

I can't say I'm confident about this, relying as it does on mockito while involving type inspection. But it Works On My Machine, and on CI.