junit 4.12 is incompatible with cucumber-junit 1.2.0

[henrik242]

PublicClassValidator throws an Exception with message:
"The class cucumber.runtime.junit.ExamplesRunner is not public."

junit 4.12 is incompatible with cucumber-junit 1.2.0, see http://stackoverflow.com/a/25540276/13365 and https://groups.google.com/d/msg/cukes/w1QhqqqvJ8M/F9Pjoxe6Xb0J

[henrik242]

Solved in cucumber-junit 1.2.2, see cucumber/cucumber-jvm#836

[kcooney]

I think we should consider fixing this in a 4.12.x release (and adding the validation check back in 5.0)

[marcphilipp]

For reference, the commit that added this validation was 1d97da7.

@kcooney Were test classes ever allowed/supposed to be non-public or was this working more or less accidentally in 4.11 and before?

[marcphilipp]

For reference, the pull requests that introduced this check was #866.

[kcooney]

Looking at the stacktrace, I think it is the runner that wasn't public in Cucumber. i don't know why Cucumber worked before.

[marcphilipp]

So, can we close this issue again?

[kcooney]

While they fixed it on their end, we did commit a change that broke existing code in a minor release. It's possible some people can't upgrade cucumber, so they are forced to downgrade JUnit. So I think we should consider a bug fix release

[marcphilipp]

IMHO we do not need to ship a fix for code that shouldn't have worked in the first place.

@junit-team/junit-committers What are your opinions on this?

[dsaff]

So was Cucumber using reflection-based visibility hoisting to get at private classes, and we weren't blocking them before, but now we are?

If that's an accurate summary, then we're in a bit of a quandry; I've always publicly maintained that JUnit needs reflectively-accessed members to be public (because we don't want to require tests to be run under a security manager that's OK with visibility hoisting). On the other hand, I don't know that that expectation was in the documentation with enough clarity that we can just say "cucumber's fault". Thoughts?

[kcooney]

Not sure what Cucumber was doing; someone should check. Without that knowledge, it is hard to say whether they had code that shouldn't have worked.

Assuming David is right, I feel that just because the classes in JUnit require annotated fields and classes to be public doesn't mean that third-party extensions should be required to do the same. Also, in this case, the affected users didn't write the problematic code. So I am slightly leaning toward fixing this in a 4.12.x release, not having a 4.13 release, and breaking it in 5.0.

[dsaff]

What would a fix look like? Cucumber is extending the JUnit Suite runner. There was a bug (IMHO) where the Suite runner wasn't consistently checking to be sure it was attached to a public class. That bug was fixed. Would a fix consist of going back and allowing Suite to be attached to private classes?

[kcooney]

I am suggesting that we would roll back the part of the change that added this particular validation. We would do that on a new branch that we don't merge to master (unless we do decide to release a 4.13 release)

[dsaff]

@kcooney, OK, SGTM.

[marcphilipp]

If we want to do this, we should do it soon, shouldn't we? @kcooney Do you have time to take care of it?

[kcooney]

@marcphilipp I can look at this in about a week. I'd also like to fix this issue if I have the time: http://stackoverflow.com/q/27420675/95725

[marcphilipp]

Eclipse was also broken by this change:
http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d4f9c3a05c94647f801324f4fab5898aa5c9f921

[marcphilipp]

@kcooney Any news on this issue?

[kcooney]

@marcphilipp sorry, haven't found time to follow through on this

[kcooney]

This was fixed in a58d459