Skip to content

Introduce tests for @Priority declared on stereotypes.#311

Merged
Ladicek merged 2 commits intojakartaee:masterfrom
manovotn:issue282
Nov 12, 2021
Merged

Introduce tests for @Priority declared on stereotypes.#311
Ladicek merged 2 commits intojakartaee:masterfrom
manovotn:issue282

Conversation

@manovotn
Copy link
Copy Markdown
Contributor

@manovotn manovotn commented Nov 4, 2021

Fixes #282

Tests are taken from Weld PR here - weld/core#2559
I also added one extra test (testSeveralPrioritiesInSingleStereotypeHierarchy) that works with several declarations of @Priority in a hierarchy of single stereotype. We don't explicitly define this in spec text, but it is implied by annotation inheritance and overriding.

Copy link
Copy Markdown
Member

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

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

Otherwise looking good.

@manovotn
Copy link
Copy Markdown
Contributor Author

manovotn commented Nov 5, 2021

Incorporated javadoc suggestions, the only remaining questionmark is #311 (comment)

@Ladicek
Copy link
Copy Markdown
Member

Ladicek commented Nov 5, 2021

Just to repeat it here: maybe add a test or two with an @Inherited stereotype? :-)

@manovotn
Copy link
Copy Markdown
Contributor Author

manovotn commented Nov 5, 2021

Just to repeat it here: maybe add a test or two with an @Inherited stereotype? :-)

True, we could add at least a simple test to see if the working case (single `@Priority) is inherited properly.
Quickly scanning TCKs, I can see we test this for scopes declared on stereotypes, but that's about it. But maybe I am just not seeing it.

@manovotn
Copy link
Copy Markdown
Contributor Author

manovotn commented Nov 9, 2021

@Ladicek I have added requested tests, do you think we need any further changes?

@manovotn
Copy link
Copy Markdown
Contributor Author

@Ladicek this PR now reflects the changes made to specification and tests for definition exception if a single stereotype indirectly declares more than one priority value. I think we are good to go now.

@Ladicek
Copy link
Copy Markdown
Member

Ladicek commented Nov 12, 2021

Otherwise LGTM.

@Ladicek Ladicek merged commit 06d4277 into jakartaee:master Nov 12, 2021
@manovotn manovotn deleted the issue282 branch November 12, 2021 16:54
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.

Provide test coverage for @Priority used on a stereotype

2 participants