Junit5 parameter: Introduce serialization alternative to XStream #16302
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
In case this is merged, I'm not sure we should close #15892 just yet or whether we want to wait until that JUnit hook is available. |
|
@mmaggioni-wise I'd very much appreciate it if you could try your record case with this change. |
Failing Jobs
Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/mailer# Tests: 1
+ Success: 0
- Failures: 0
- Errors: 1
! Skipped: 0❌
|
geoand
reviewed
Apr 7, 2021
integration-tests/main/src/test/java/io/quarkus/it/main/ParameterResolverTest.java
Show resolved
Hide resolved
geoand
reviewed
Apr 7, 2021
|
@geoand Two questions:
|
We usually don't backport changes like this, but if there is a very good reason, we could reconsider
Sure, sounds good |
I thought of this because #15892 is a bug, not an enhancement. But maybe it's a bit more of the latter. |
Yeah, it's a bug but not of very high impact. Furthermore the change could theoretically break things and we usually try hard to avoid such breakages. |
|
hey @famod If the param passed is an ArrayList of records (or of a normal class too!) that does not implement Serializable then the test will wait indefinitely (or just a long long time, i stopped the test after a while tbh). My guess is that since the ArrayList implement Serializable it will use your implementation. If I have a complex class/record, with children of different class/record, each of this should implement serializable? |
|
@mmaggioni-wise Thanks for checking!
That's odd! I'll try to reproduce that. My wild guess at this point is that an exception is thrown (but not printed) and the test setup reaches an inconsistent state.
Unfortunately: yes! This serialization alternative is just a band-aid solution until we can use a proper hook in JUnit (or until XStream 1.5 is released). |
|
I think we should fall back if the Serializable solution fails. Lots of classes may appear to be Serializable but simply aren't in practice. Given that the types passed to this method will often be collections I think the current implementation has the potential to fail more often than it succeeds. |
|
Yes, we definitely need a fallback. |
|
I think Serialization first then XStream if it fails. If Serialization works then you can be very confident that the result is correct, its just that it will fail in a lot of cases. |
|
I pushed a rewrite without that That hang problem was actually a mistake on my side: Those piped streams are meant to be used in different threads, not the same one! @mmaggioni-wise So in case you want to give it another try... |
Failing Jobs - Building 945022c
Test Failures⚙️ JVM Tests - JDK 15 #📦 integration-tests/container-image/quarkus-standard-way✖
|
|
@stuartwdouglas can you have another look please? Thanks! |
stuartwdouglas
approved these changes
Apr 12, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relates to #15892 in that it should be able to make records implement
Serializableso that they can be used as parameters.As discussed in that issue, a new hook in JUnit5 is required to drop all this cloning for good but nobody knows when that will be available.