Conversation
`graphql-java` is already on Java 11 so we take the liberty of bumping this library to the same version to unlock some nice features.
| assertThat(loadCalls.size(), equalTo(1)); | ||
| assertThat(future1.get(), equalTo(key1)); | ||
| assertThat(future2.get(), equalTo(key1)); | ||
| assertThat(future2.get(), equalTo(key2)); |
There was a problem hiding this comment.
Sorry - why is this test changed for Java 11 ? was it wrong? and if so - why was it passing ?
There was a problem hiding this comment.
A brief inspection indicates the test shouldn't have been working with Java 8. We can see this because the identityLoader doesn't have identical types for its K and V type parameters - if we pass in a JsonObject to a loader generated from idLoader, we should expect to receive that same JsonObject coming out, but that's not what this test was checking.
As to why it was passing at all - I suspect it has something to do with this bug that was fixed with Java 9. Without digging into the javac-generated bytecode, it looks like a class check was missing from bytecode generated with Java 8. As such, when we tried to run with this code with Java 11, we started failing because we tried to cast a JsonObject (the returned value) to an Integer (the V in the DataLoader).
As to why I changed it from key1 to key2 - while key1.equals(key2) is true, it seemed neater to check we got key2 for the future (as it corresponded to future2).
There was a problem hiding this comment.
Yeah I just looked into it - <K,V> are all just Object at runtime and since we never treated a value as a JsonObject - well the test got away with it. Makes sense to fix
|
Wow thanks for upgrading to Java 11 @AlexandreCarlton ! |
3 participants
graphql-javais already on Java 11 so we take the liberty of bumping this library to the same version to unlock some nice features.