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

Support for hierarchical context hierarchies #817

Merged
merged 1 commit into from
Apr 28, 2014
Merged

Support for hierarchical context hierarchies #817

merged 1 commit into from
Apr 28, 2014

Conversation

bechte
Copy link
Contributor

@bechte bechte commented Feb 9, 2014

This is a pull-request for issue #816.
#816: Support for hierarchical context hierarchies - RunWith annotation will be resolved along the hierarchy of non-static member classes.

Discussions on this topic should happen in #816. I provided this pull-request mostly to show the changes more graphically.

currentTestClass = getEnclosingClassForNonStaticMemberClass(currentTestClass)) {
RunWith annotation = currentTestClass.getAnnotation(RunWith.class);
if (annotation != null) {
return buildRunner(annotation.value(), currentTestClass);
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you don't want to pass testClass instead of currentTestClass here? Passing the outermost class with @RunWith doesn't appear to solve #816

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this. Of course, testClass needs to be passed with buildRunner. I changed it and updated the pull request.

@kcooney
Copy link
Member

kcooney commented Feb 21, 2014

Could you add a test?

@bechte
Copy link
Contributor Author

bechte commented Feb 22, 2014

Yes, of course. I couldn't find any tests for this class so I also added tests for the default case. Hope that's fine. Let me know if it needs any changes on those.

@kcooney
Copy link
Member

kcooney commented Feb 23, 2014

LGTM

@dsaff any objections?

@bechte
Copy link
Contributor Author

bechte commented Feb 24, 2014

With the last commit, I didn't change any more line of production code, just added two more test cases that show that annotation on inner member classes are also handled while going upwards to the top level class.

Still, this has no effect on the JUnit default runner, because it does not allow inner member (non-static) classes.

@kcooney
Copy link
Member

kcooney commented Feb 24, 2014

@bechte Yes, I saw that. Just making sure @dsaff doesn't have any objections to this code having a special case for inner classes.

@dsaff
Copy link
Member

dsaff commented Feb 24, 2014

I'm OK with the change. But I do think there may need to be some documentation that devs can use in terms of the expected search path, especially about the fact that, if I'm reading it right, a superclass of the inner class will be used before the outer class. (This is understandable behavior, but should be doc'ed.)

@bechte
Copy link
Contributor Author

bechte commented Feb 24, 2014

Sounds Good. I will provide docs soon. Prob. at the weekend.

@stefanbirkner stefanbirkner added this to the 4.12 milestone Feb 26, 2014
@bechte
Copy link
Contributor Author

bechte commented Mar 1, 2014

I added the documentation.

@kcooney
Copy link
Member

kcooney commented Mar 8, 2014

LGTM. I'm assigning this to @dsaff since he requested the documentation.

@dsaff
Copy link
Member

dsaff commented Mar 12, 2014

Unfortunately, the new docs still seem to leave unresolved the question of priority. Given:

@RunWith(R1.class)
public class Outer {
public class Inner extends Super {
}
}

@RunWith(R2.class)
public class Super {}

Does Inner run with R1 or R2?

@kcooney
Copy link
Member

kcooney commented Mar 12, 2014

@dsaff RunWith is inherited, so inner will run with R2

@dsaff
Copy link
Member

dsaff commented Mar 12, 2014

@kcooney, right. I think this is worth calling out explicitly in the documentation, because it's consistent behavior, but also potentially surprising.

@kcooney
Copy link
Member

kcooney commented Apr 18, 2014

@bechte can you update the documentation to clarify the issue that @dsaff brought up? Thanks.

@bechte
Copy link
Contributor Author

bechte commented Apr 18, 2014

I'm sorry it took so long to reply. We had tremendous troubles in the last 5 weeks on work so I had hardly time for anything else. I modified the documentation. Thanks for reviewing.

@kcooney
Copy link
Member

kcooney commented Apr 20, 2014

@dsaff do those updates resolve your concerns?

@bechte after David reviews the recent changes, do you think you want to squash some of those commits?

@dsaff
Copy link
Member

dsaff commented Apr 21, 2014

Looks good to me, thanks!

…ion will be resolved along the hierarchy of non-static member classes.
@bechte
Copy link
Contributor Author

bechte commented Apr 27, 2014

Sure, makes totally sence. I squashed all commits into one. Thanks.

kcooney added a commit that referenced this pull request Apr 28, 2014
Support for hierarchical context hierarchies
@kcooney kcooney merged commit cf507ab into junit-team:master Apr 28, 2014
@bechte
Copy link
Contributor Author

bechte commented Apr 29, 2014

Thanks.

@kcooney
Copy link
Member

kcooney commented Apr 29, 2014

@bechte no problem. Glad to help. Could you please update the release notes?

@bechte
Copy link
Contributor Author

bechte commented Apr 30, 2014

Sure, done.

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.

None yet

4 participants