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

Remove broken XStream2 test of legacy custom ConcurrentHashMapSerialization #2071

Merged
merged 3 commits into from
Mar 7, 2016

Conversation

svanoort
Copy link
Member

This quirk causes failure of the the Jenkins test suite when run under JDK8.

This one requires some explanation: there is a bug with testing deserialization of the custom ConcurrentHashMap serialization that existed before 2010 (see eb51d46). Given the age, we can assume that testing conversion is no longer a high priority.

Root cause appears to be a subtle change in the XStream attribute access behavior when run under JDK8 only. This appears to be the only case that triggers it. I've left in as much as possible of the test for coverage, while removing the offending piece.

…ation

This one requires some explanation: there is a bug
As custom serialization of ConcurrentHashMaps was removed circa 2010
(see eb51d46)

We can assume that testing conversion is no longer a high priority.
Root cause appears to be a subtle change in the XStream attribute access behavior when run under JDK8 only.
This appears to be the only case that triggers it.
@svanoort
Copy link
Member Author

@reviewbybees specifically @jglick and @daniel-beck who had requested this be included.

@@ -296,11 +296,6 @@ public void concurrentHashMapSerialization() throws Exception {
} finally {
v.delete();
}

// should be able to read in old data just fine
Copy link
Member

Choose a reason for hiding this comment

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

The Javadoc for the test is now incorrect—it is no longer testing that we

still can deserialize to [from?] older, verbose format

Since it is no longer testing Jenkins code at all, I would rather suggest just deleting the whole test case—the part you left behind was only a kind of control case for the real test which is now deleted.

@jglick
Copy link
Member

jglick commented Feb 29, 2016

I would propose just deleting ConcurrentHashMapConverter. Its only purpose is to do the conversion which we know does not work any more. (Well, for Java 7 users, but we recommend 8, and at some point will require it.) Whatever user settings were being saved by this hack no longer are, and anyway it is unclear how long ago such an old format was being created: no later than 2010, apparently.

@ghost
Copy link

ghost commented Feb 29, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@svanoort
Copy link
Member Author

svanoort commented Mar 2, 2016

@jglick Removed them both, as separate commits in case any issues arise from the ConcurrentHashMapConverter

Edit: specifically Java 7 users upgrading from ancient installations.

@jglick
Copy link
Member

jglick commented Mar 2, 2016

🐝 (maybe @kohsuke can explain what this was about to begin with)

@lordofthejars
Copy link

🐝

@andresrc
Copy link
Contributor

andresrc commented Mar 4, 2016

🐝

I was hit be this today in junit test suite. If the test suite has LocalData prepared with that ancient versions...

@svanoort
Copy link
Member Author

svanoort commented Mar 4, 2016

@reviewbybees done

@ghost
Copy link

ghost commented Mar 4, 2016

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

@jglick
Copy link
Member

jglick commented Mar 5, 2016

PR build does not seem to have worked.

@jglick jglick closed this Mar 5, 2016
@jglick jglick reopened this Mar 5, 2016
@svanoort
Copy link
Member Author

svanoort commented Mar 5, 2016

Oh for goodness sakes. Interrupted errors and bizarre socket quirks. Once this one finishes it might need yet another build sigh.

@jglick
Copy link
Member

jglick commented Mar 7, 2016

No word from @kohsuke so I am going ahead with a merge.

jglick added a commit that referenced this pull request Mar 7, 2016
Remove broken XStream2 test of legacy custom ConcurrentHashMapSerialization
@jglick jglick merged commit 785811a into jenkinsci:master Mar 7, 2016
@svanoort svanoort deleted the remove-broken-xstream-test branch March 8, 2016 15:05
kohsuke pushed a commit that referenced this pull request May 11, 2016
…s it prevents use of JDK 8 to run core tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants