Skip to content
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

Merged

Conversation

justintuchek
Copy link
Contributor

@justintuchek justintuchek commented Feb 8, 2019

Overview

  • Caputre non Serializable inputs to ValueWrapper as transient field

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 as identityHashCode(), toString(), and getType() would remain unchanged.


I hereby agree to the terms of the Open Test Alliance Contributor License Agreement.

@justintuchek justintuchek changed the title Allow non Serializable inputs to ValueWrapper as transient field Capture non Serializable inputs to ValueWrapper as transient field Feb 8, 2019
Copy link
Member

@marcphilipp marcphilipp left a 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}
Copy link
Member

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>.

Copy link
Contributor Author

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@since is missing

Copy link
Contributor Author

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 👍

@justintuchek
Copy link
Contributor Author

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@marcphilipp marcphilipp added this to the 1.2.0 milestone Apr 6, 2019
@marcphilipp marcphilipp merged commit 42e530d into ota4j-team:master Apr 6, 2019
@marcphilipp
Copy link
Member

Thanks, @justintuchek! 👍

@justintuchek justintuchek deleted the value-wrapper-ephemeral-value branch April 6, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants