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.

Sign up for GitHub

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

Jump to bottom

Initial TCK for the language model #310

Merged
merged 8 commits into from
Nov 23, 2021
Merged

Initial TCK for the language model #310

merged 8 commits into from
Nov 23, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Nov 2, 2021

No description provided.

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 2, 2021

There's a couple of TODOs in the code, plus I want to add one or two other smaller items, but this PR should have like 90% of what's required, IMHO.

@graemerocher
Copy link
Contributor

So there is no actual test framework involved, the docs will say to run it from your test framework of choice?

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 3, 2021

That's the idea, yes.

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 4, 2021

Added a commit with some more tests (for a bunch of corner cases, I especially enjoyed figuring out how inherited repeatable annotations are supposed to work). I think the tests in this PR now have pretty good coverage.

There are still 8 TODOs in the code. The TODOs are of 3 kinds:

  • 1 TODO for testing records -- but I have no idea how
  • 2 TODOs for unbounded wildcard types -- they currently don't test what the spec says, but I tend to believe that the spec should change
  • 5 TODOs where I'm not exactly sure what the correct behavior is

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 8, 2021

Added one more commit that resolves some TODOs and adds some more tests.

Summary of the 5 remaining TODOs:

  • 1 TODO for testing records -- but I have no idea how
  • 2 TODOs for unbounded wildcard types -- they currently don't test what the spec says, but I tend to believe that the spec should change
  • 2 TODOs for annotations on transitive type parameter bounds

manovotn
manovotn approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

LGTM, great job!

@Ladicek Ladicek force-pushed the lang-model-tck branch 2 times, most recently from 1a82bce to 874f54b Compare November 16, 2021 16:17
@Ladicek Ladicek mentioned this pull request Nov 22, 2021
Merged
@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 22, 2021

Added two more commits for some strange cases (in particular, annotations and type annotations on parameters of enum constructors, which are notorious for having synthetic parameters).

By now, there is only 1 TODO remaining: how to test support for records.

@Ladicek
Initial TCK for the language model
c57736e
@Ladicek
Improve the TCK for the language model
4f6bc44 
- test annotations on receiver types
- test annotations on types in the `throws` clause
- test bridge methods (they are _not_ returned)
- test default constructors (they _are_ returned)
- test abstract `enum` methods
- test equality (and hash code)
- test inherited annotations (on classes)
- test repeatable annotations (including inherited)
@Ladicek
Resolve some TODOs and add some more tests
03500b2
@Ladicek
Add README.adoc
32db150
@Ladicek
Add tests for some more corner cases
fee59cd 
- annotations on type parameter bounds that are parameterized types
- annotations on enum constants, enum constructors and enum constructor parameters
- access to declaring classes of inherited methods
- number of constructor parameters, especially for enum constructors
- remove TODOs for annotations on transitive type parameter bounds
- workaround for bug JDK-8202469
@Ladicek
Remove TODOs for unbounded wildcard representation
e984730 
The API was respecified to do what this test always verified.
@Ladicek
Depend only on jakarta.enterprise.lang-model in the lang model TCK
ae9ef11
@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 23, 2021

Rebased on current master which uses CDI 4.0.0.Beta2 and hence the split lang-model module.

@manovotn
Copy link
Contributor

The README change looks good!
Going to merge this since it was already approved and we discussed during the meeting that it is ready.

@manovotn manovotn merged commit 1084ceb into jakartaee:master Nov 23, 2021
@manovotn manovotn linked an issue Nov 23, 2021 that may be closed by this pull request
Provide test coverage for CDI Lite language model #306
Closed
@manovotn manovotn mentioned this pull request Nov 23, 2021
Closed
@Ladicek Ladicek deleted the lang-model-tck branch November 23, 2021 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graemerocher graemerocher graemerocher approved these changes

@manovotn manovotn manovotn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide test coverage for CDI Lite language model
3 participants
@Ladicek @graemerocher @manovotn