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 @WeldSetup annotated fields in test superclasses #32

Closed
qmunke opened this issue Feb 6, 2018 · 8 comments
Closed

Support for @WeldSetup annotated fields in test superclasses #32

qmunke opened this issue Feb 6, 2018 · 8 comments
Milestone

Comments

@qmunke
Copy link

qmunke commented Feb 6, 2018

Currently if a test class inherits from a parent containing a field annotated with @WeldSetup, the WeldJUnit5Extension doesn't pick it up and creates a default instance - due to the call to Class#getDeclaredFields as opposed to Class#getFields. This leads to confusing errors depending on how you're trying to inject into the test (either getting "Container not started" if you reference the super's WeldInitiator from the subclass, or exceptions about unsatisfied dependencies if you try and @Inject a class that isn't picked up by the default instance)

See https://github.com/qmunke/weld-junit/commit/b77d16f10b4d75bd96a686a5c144fc0999c5e684 for a potential fix.

@manovotn
Copy link
Collaborator

manovotn commented Feb 7, 2018

Hello,

yea, you are right about this bug. Thanks for the effort with debugging/nailing it down.
Since you went this far already, would you mind creating a PR from your potential fix?

@manovotn manovotn added this to the 1.2.2 milestone Feb 7, 2018
@mkouba
Copy link
Member

mkouba commented Feb 7, 2018

Note that getFields() can only be used for public fields. We don't have this requirement ATM for junit5 extensions (for junit4 test rules must be declared public).

@qmunke
Copy link
Author

qmunke commented Feb 7, 2018

The documentation doesn't indicate this:

WeldInitiator should be a public field annotated with @org.jboss.weld.junit5.WeldSetup. From there you can use static methods:

@mkouba
Copy link
Member

mkouba commented Feb 7, 2018

@qmunke Well, you're right the docs say should be. I wonder if we should change this to must be or simply discover the class hierarchy and use getDeclaredFields(). @manovotn WDYT?

@manovotn
Copy link
Collaborator

manovotn commented Feb 7, 2018

The extension was designed to be able to access even non-public fields.
E.g. it uses setAccessible

I would prefer to retain this ability but the requirement to grab this field from superclass should definitely be supported. Maybe we should inspect the hierarchy but still allow non-public fields? The current class' field would be prioritized, but if there is none, we could inspect the hierarchy and search for any fitting field. Does that sound sane?

@mkouba
Copy link
Member

mkouba commented Feb 7, 2018

Maybe we should inspect the hierarchy but still allow non-public fields?

+1

@qmunke something like this should work:

for (Class<?> clazz = testInstance.getClass(); clazz != null; clazz = clazz.getSuperclass()) {
  // Find @WeldSetup field using getDeclaredFields()...
}

@rakcheru
Copy link

rakcheru commented Mar 26, 2018

@mkouba @manovotn : waiting for this fix to be available as part of weld-junit release. Any plans to release 1.2.2 soon ?

Note : technically, applying the fix on top of 1.2.1 is not a solution for us.

@mkouba
Copy link
Member

mkouba commented Mar 27, 2018

@rakcheru Yes, we're going to release 1.2.2 later today or tomorrow morning (CET).

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

No branches or pull requests

4 participants