-
Notifications
You must be signed in to change notification settings - Fork 37
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
Capture non Serializable inputs to ValueWrapper as transient field #54
Capture non Serializable inputs to ValueWrapper as transient field #54
Conversation
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.
I think this is a good idea. 👍
@@ -122,6 +124,14 @@ public int getIdentityHashCode() { | |||
return this.identityHashCode; | |||
} | |||
|
|||
/** | |||
* Returns the value supplied to {@link #create(Object)}. if {@code ValueWrapper} | |||
* has been been serialized this will return {@code null} |
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.
Please add a blank line after the first sentence and start the new paragraph with <p>
.
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.
Can do, thanks for the feedback!
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.
@since
is missing
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.
Set to 1.2
as that's the current snapshot version listed in master since last release 👍
Hey @marcphilipp would you be able to give this a second look? |
@@ -158,6 +158,23 @@ public void deserializationOfAssertionFailedErrorWorksForVersion_1_0_0() throws | |||
assertEquals("bar", error.getActual().getValue()); | |||
} | |||
|
|||
@Test | |||
public void ephemeralValueIsOmittedFromSerialization() throws Exception { |
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.
Hm, wouldn't this make more sense if value
itself was Serializable
, maybe add another variant with that?
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.
The original intent was to verify the behavior of ephemeralValue
through serialization, which should return null when given a non-serializable value.
I'm not opposed to adding more test but there are already a handful that exercise AssertionFailedError
and ValueWrapper
with serializable values.
Thanks, @justintuchek! 👍 |
Overview
There seems to have been some discussion about this in the past: #2 #49 and #50
This constraint can be hard to work with if somebody is trying to use those values but has no control over what a user inputs into their assertions.
A concrete example:
JUnit 5 has an extension model which allows for processing of test cases and failure scenarios, such as TestExcecutionExceptionHandler.
During the execution of those extensions we are still in the same runtime. Having access to provided test inputs would be helpful for processing real objects - as compared to analyzing only their string representation
A proposed solution:
Exposing the same instance as a transient field will keep compatibility for Serialized requirements but allow access in test frameworks.
Given the ephemeral object is the same instance it would have a very minimal impact on
ValueWrapper
asidentityHashCode()
,toString()
, andgetType()
would remain unchanged.I hereby agree to the terms of the Open Test Alliance Contributor License Agreement.