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

ArrayComparisonFailure serialization incompatibility fix: #1315

Merged
merged 2 commits into from
Jul 17, 2016

Conversation

aishahalim
Copy link
Contributor

Add back field fCause, initialize and use in the constructor (via initCause()) to avoid npe and unused field warnings, respectively
Issue #1178, #976

@marcphilipp
Copy link
Member

marcphilipp commented Jun 1, 2016

Thanks for the pull request! On second thought I think we need to do a little more.

As you have verified in #1178 (comment) this will ensure that pre 4.12 versions can consume post 4.12 versions' ArrayComparisonFailures. However, this does not hold the other way around. We should override getCause() similar to this:

public synchronized Throwable getCause() {
    return super.getCause() == null ? fCause : super.getCause();
}

I agree that this is hard to test. However, I think the following should be fairly easy doable (@kcooney has done the same for Result in f6149a9):

  1. Create a ArrayComparisonFailure using 4.11 and serialize it into a file.
  2. Put the file into src/test/resources and add a test that reads it, deserializes it and checks the resulting ArrayComparisonFailure

Do the same for 4.12.

@aishahalim Please let me know if you're okay with doing that.

@aishahalim
Copy link
Contributor Author

However, this does not hold the other way around.

Yikes, yes I see it now.

Thanks for pointing out how to construct that test! On it!

Add back field fCause, initialize and use in the constructor (via initCause()) to avoid npe and unused field warnings, respectively
Issue junit-team#1178, junit-team#976
Override getCause() to allow fallback to the deprecated fCause field.
Run tests around possible forward incompatibility of the class from r4.11, 4.12.
@aishahalim
Copy link
Contributor Author

aishahalim commented Jun 13, 2016

Hey Marc!

I've update this pullrequest with another commit. Your test strategy description, when implemented in the test, very apparently caught the forward incompatibility problem with getCause() not falling back to fCause.

I threw in a description in the test to explain what it's interacting with (since super wordy variablenames don't fill in the blanks around all the "serializing to a file setup") and why.

What do you think about it so far?

I didn't mull over it long enough, but I don't see how I could test the other way, where I have a current version of an exception deserializing with an older version of the class. Is this implemented in f6149a9? I threw a breakpoint in when running ResultTest and still didn't see anything that tried to serialize to a special version of a class, unless you were referring to the fixed runtime sub class -- i.e. subclassing ArrayComparisonFailure to behave like the previous versions', then testing compatibility with the current version's instance of the failure?

Apologies for all these questions.

@marcphilipp
Copy link
Member

Looks good to me, you did exactly what I had in mind! 👍

@junit-team/junit-committers What do you think, is this good to merge?

@PeterWippermann
Copy link
Contributor

Any updates on this?

@aishahalim
Copy link
Contributor Author

^ It might be a bit of a hefty changeset for a not-at-all-critical bug, so, understandably, second review might take longer.

Let me know if I should split out any functional bits in separate commits for reviewing succinctness or whatnot.

@marcphilipp
Copy link
Member

@junit-team/junit-committers I'd like a second opinion on this. Can one of you guys please have a look?

@kcooney kcooney added the 4.13 label Jul 17, 2016
@kcooney
Copy link
Member

kcooney commented Jul 17, 2016

LGTM. @marcphilipp was there something specific you were concerned about?

@marcphilipp marcphilipp added this to the 4.13 milestone Jul 17, 2016
@marcphilipp
Copy link
Member

was there something specific you were concerned about?

@kcooney Basically I just wanted to have another pair of eyes look at it to decrease the probability that we've overlooked something. :-)

@marcphilipp marcphilipp merged commit e6a2004 into junit-team:master Jul 17, 2016
@marcphilipp
Copy link
Member

@aishahalim Thanks again! 👍 Please add a description of this fix to the draft of the release notes.

@PeterWippermann
Copy link
Contributor

Well done, @aishahalim! 👍

@kcooney kcooney modified the milestone: 4.13 Aug 6, 2017
@kcooney kcooney removed the 4.13 label Aug 6, 2017
sebasjm pushed a commit to sebasjm/junit4 that referenced this pull request Mar 11, 2018
…1315)

* Add back field fCause, initialize and use it in the constructor (via initCause()) to avoid NPE and unused field warnings, respectively.
* Override getCause() to allow fallback to the deprecated fCause field.
* Run tests around possible forward incompatibility of the class from r4.11, 4.12.

Fixes junit-team#1178.
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.

4 participants