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 passing parameters to a QuarkusTestResource #9082

Merged
merged 1 commit into from
May 12, 2020

Conversation

geoand
Copy link
Contributor

@geoand geoand commented May 5, 2020

Resolves: #9066

@geoand geoand force-pushed the #9066 branch 2 times, most recently from 45cc108 to a87724d Compare May 5, 2020 09:19
@geoand
Copy link
Contributor Author

geoand commented May 5, 2020

This nees to be to ensure that we don't have conflicting args.

Done

gsmet
gsmet previously requested changes May 5, 2020
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Interesting! I added a comment and a question.

@geoand geoand requested a review from gsmet May 5, 2020 10:08
@geoand geoand force-pushed the #9066 branch 2 times, most recently from bc27dcc to e7c1ecc Compare May 5, 2020 10:18
@geoand geoand requested a review from stuartwdouglas May 5, 2020 10:43
@famod
Copy link
Member

famod commented May 5, 2020

What happens if you have:

  • Test 1 with @QuarkusTestResource(value=MyMgr.class, initArgs={"foo=true"})
  • Test 2 with @QuarkusTestResource(value=MyMgr.class, initArgs={"foo=false"})

or something like that?

@geoand
Copy link
Contributor Author

geoand commented May 5, 2020

The PR makes sure that the test fails

@knutwannheden
Copy link
Contributor

The PR makes sure that the test fails

Would an alternative be to instantiate MyMgr twice (and call one with init("foo=true") and the other with init("foo=false"))?

@geoand
Copy link
Contributor Author

geoand commented May 5, 2020

The PR makes sure that the test fails

Would an alternative be to instantiate MyMgr twice (and call one with init("foo=true") and the other with init("foo=false"))?

I think that could lead to surpising results. Best keep with the established Quarkus practice.

@geoand geoand marked this pull request as ready for review May 5, 2020 19:17
@knutwannheden
Copy link
Contributor

I think that could lead to surpising results. Best keep with the established Quarkus practice.

I see. One would have to create some "dummy" classes in these (hopefully rare) situations. I was for instance thinking of the situation where I need two datasources. Then I would have liked to declare two annotations like @QuarkusTestResource(PostgresDatabase.class) (default datasource) and @QuarkusTestResource(PostgresDatabase.class, initArgs="datasource=other"). Possibly there could be some flag in the future to allow this kind of usage.

@geoand geoand force-pushed the #9066 branch 2 times, most recently from a4597ed to 2079b4a Compare May 6, 2020 11:00
@gsmet
Copy link
Member

gsmet commented May 6, 2020

I think that could lead to surpising results. Best keep with the established Quarkus practice.

It might be just me but I think I would:

  • allow two resources with different initArgs
  • disallow having two resources with the exact same initArgs

This way, we have the best of both world: flexibilty and keeping the behavior as is if you don't use initArgs.

WDYT?

@knutwannheden
Copy link
Contributor

@gsmet What you propose would also be my preferred solution.

@geoand
Copy link
Contributor Author

geoand commented May 6, 2020

I don't know... I am not convinced about having different initArgs. I think it would lead to users to believe that they can use different @QuarkusTestResource on different classes and that those would be applied per test.

@famod
Copy link
Member

famod commented May 6, 2020

By the way, what's the status of having individual resources per test class? Anything planned?

The way the annotation can be used and how it is documented (JavaDoc) leaves the impression you can have individual resources per test class.

@geoand
Copy link
Contributor Author

geoand commented May 6, 2020

By the way, what's the status of having individual resources per test class? Anything planned?

Nothing done so far, but hopefully soon :)

The way the annotation can be used and how it is documented (JavaDoc) leaves the impression you can have individual resources per test class.

Would you like to propose a way for it to be written differently that would cause less confusion?

As I mentioned before, I think that the if we allow @QuarkusTestResource with different initArgs, it will only add to the confusion.

@geoand
Copy link
Contributor Author

geoand commented May 8, 2020

So the question becomes, what should we do about this?

@stuartwdouglas
Copy link
Member

I think that different args makes sense, e.g. starting two different databases of the same type is definitely a common use case, however I can see how it could be confusing.

Maybe make this annotation @repeatable, IMHO this would make it clearer that it can be used to create multiple resources.

@geoand
Copy link
Contributor Author

geoand commented May 11, 2020

@QuarkusTestResource is already repeatable, so I just lifted the restriction I had imposed on not allowing different initArgs.
I also added a note to the javadoc of @QuarkusTestResource to make clarify how it's used.

@geoand geoand added this to the 1.5.0 milestone May 11, 2020
@geoand
Copy link
Contributor Author

geoand commented May 11, 2020

CI failure has nothing to do with this PR:

2020-05-11T07:49:51.8096777Z Caused by: java.io.IOException: Could not start process: <EOF>
2020-05-11T07:49:51.8096946Z 	at de.flapdoodle.embed.mongo.AbstractMongoProcess.onAfterProcessStart(AbstractMongoProcess.java:79)
2020-05-11T07:49:51.8097071Z 	at de.flapdoodle.embed.process.runtime.AbstractProcess.<init>(AbstractProcess.java:116)

@geoand
Copy link
Contributor Author

geoand commented May 11, 2020

@stuartwdouglas, @gsmet WDYT now?

@geoand geoand dismissed gsmet’s stale review May 12, 2020 04:36

Comments addressed

@geoand geoand merged commit 579589b into quarkusio:master May 12, 2020
@geoand geoand deleted the #9066 branch May 12, 2020 04:37
@snowdrop-bot snowdrop-bot mentioned this pull request May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arguments for QuarkusTestResource
5 participants