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

[ARQ-2107][ARQ-2109] improve @EJB and @Resource enrichers #127

Merged

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Apr 28, 2017

@Ladicek Ladicek changed the title [ARQ-2107] ResourceInjectionEnricher uses @Resource(lookup = "...") value [ARQ-2107][ARQ-2109] improve @EJB and @Resource enrichers May 4, 2017
@bartoszmajsak
Copy link
Member

Thanks for these fixes @Ladicek. Can you also check if it would make sense to cover them in the tests we have for both modules?

@Ladicek
Copy link
Contributor Author

Ladicek commented May 8, 2017

@bartoszmajsak thanks for looking into this. I've briefly considered adding tests and concluded I don't know how I woud do that. Specifically:

WDYT?

@bartoszmajsak
Copy link
Member

For @resource, if I were to add a class with @resource(lookup=...), I'd either have to rely on Java 7 (and Arquillian seems to be on Java 5 or 6) or I'd have to bring in a Java EE 6 API dependency and somehow make sure that it will take precedence (which looks brittle, see http://stackoverflow.com/questions/32991207/maven-build-uses-jdk-home-src-zip-javax-annotation-resource-java-not-dependency or http://www.mastertheboss.com/jboss-frameworks/maven-tutorials/jboss-maven/solving-the-resource-lookup-compilation-issue).

Totally agree - that was my thought as well.

For @ejb, I'm just expanding the set of heuristics for one more option. I can only think of testing that the set of heuristics actually used contains some well-known patterns, but I don't think it would add much.

Yeah from the unit testing perspective that won't add much.

Are you testing it somewhere though? e.g. WF Swarm?

@Ladicek
Copy link
Contributor Author

Ladicek commented May 8, 2017

I do have tests for Swarm that I want to submit that excersise these changes. Here's a WIP branch: https://github.com/Ladicek/wildfly-swarm/commits/test-arquillian-enrichers However, I can't submit those tests just yet because there's no Arquillian release that contains them :-) And when trying to run the test locally with a SNAPSHOT build, I found a Swarm issue as well (https://issues.jboss.org/browse/SWARM-1326).

@bartoszmajsak bartoszmajsak merged commit 63d4bf5 into arquillian:master Jul 18, 2017
@bartoszmajsak
Copy link
Member

bartoszmajsak commented Jul 18, 2017

Thanks for your contribution!

@Ladicek Ladicek deleted the resource-injection-enricher-use-lookup branch July 31, 2017 09:25
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.

2 participants