-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
test(TypeHandlerLibrary): RuntimeDelegatingTypeHandler test failure #3992
Conversation
I just hit the "reformat file" button
instead of relying on transitive dep through mockito-junit-jupiter
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. |
<component name="InspectionProjectProfileManager"> | ||
<profile version="1.0"> | ||
<option name="myName" value="Project Default" /> | ||
<inspection_tool class="VarargParameter" enabled="false" level="INFORMATION" enabled_by_default="false" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You sure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# Conflicts: # .idea/dictionaries/kevint.xml
EvilTak provided some comments about this today: https://discord.com/channels/270264625419911192/696830442065625118/762033934925561866 |
# Conflicts: # .idea/dictionaries/kevint.xml # engine-tests/build.gradle # subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/RuntimeDelegatingTypeHandlerTest.java
That was a big merge because it has been a while and all of the TypeHandlerLibrary code moved, but it seems to still apply. |
# Conflicts: # .idea/dictionaries/kevint.xml # subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/RuntimeDelegatingTypeHandlerTest.java
No such luck. It just makes the test fail a little earlier, Lines 131 to 133 in caf9721
|
5cfebbe
to
c17653f
Compare
After stubbing out the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks right.
Works(i love tests)
# Conflicts: # subsystems/TypeHandlerLibrary/build.gradle.kts
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. |
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
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
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.