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

Avoid using collection as bean type in ManagedBeanTypesTest #477

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

manovotn
Copy link
Contributor

Fixes #476

IIUIC, this should fix the problem while retaining the test goal (which is bean types for nested generics).
Is that right @arjantijms ?

@manovotn manovotn requested a review from Ladicek July 12, 2023 06:04
@Ladicek
Copy link
Contributor

Ladicek commented Jul 12, 2023

I would avoid Iterable too, even if it seems fairly safe.

But yes, this is a proper way to fix this test.

@arjantijms
Copy link

Looks good, same remark about Iterable.

@manovotn
Copy link
Contributor Author

I would avoid Iterable too, even if it seems fairly safe.

Looks good, same remark about Iterable.

Alright, alright, my laziness got the better of me :)
I've updated the PR to only use custom interface structure.

@manovotn manovotn requested a review from Ladicek July 12, 2023 10:17
@Ladicek
Copy link
Contributor

Ladicek commented Jul 12, 2023

Do we treat this (or rather #476) as a challenge?

EDIT: I mean, I don't have a problem with that, and I think we probably should, just asking for opinions. If we don't, this PR should wait for 4.1.

@arjantijms
Copy link

It probably can't be a challenge, at least not the way it's worded right now. JDK 21 is obviously not a requirement for the EE 10 TCK.

But just like an implementation detail was updated for the Authentication TCK that didn't effect the test goal itself (which is just testing generics in this case), it could be a general update. Thoughts?

@manovotn
Copy link
Contributor Author

But just like an implementation detail was updated for the Authentication TCK that didn't effect the test goal itself (which is just testing generics in this case), it could be a general update. Thoughts?

I am not sure we are allowed to put anything but necessary changes and challenge resolutions to 4.0.x releases now.
It's probably safer to leave this for 4.1 - that being said, we should consider just moving main branch to that and creating a separate 4.0 branch for any further maintenance releases. Most work going into CDI now will be related to 4.1 anyway

@Ladicek Ladicek added this to the CDI 4.1 milestone Jul 12, 2023
@Ladicek
Copy link
Contributor

Ladicek commented Jul 12, 2023

I added the 4.1 milestone.

Good point about branching @manovotn though. I want to submit the PR for TCK tests for InvocationContext.getInterceptorBindings(), and there are some other outstanding PRs for 4.1 already. So I agree with moving master to 4.1.

@manovotn manovotn merged commit 4089e59 into jakartaee:master Sep 21, 2023
2 checks passed
@manovotn manovotn deleted the issue476 branch September 21, 2023 07:40
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.

ManagedBeanTypesTest not compatible with JDK 21
3 participants