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

TCK tests for virtual threads #425

Merged
merged 7 commits into from
Feb 23, 2024

Conversation

KyleAure
Copy link
Contributor

@KyleAure KyleAure commented Feb 5, 2024

These are tests for virtual=true assuming option 2 will win the vote (which is currently in the lead).

When testing on Java 17:

  • Deploys an application to that contains annotations and deployment descriptors with virtual=false and virtual=true configurations
  • Asserts that when virtual=false a platform thread is returned for ManagedExecutorService, ManagedExecutorScheduledService, ManagedThreadFactory, ForkJoinPool.
  • Asserts that when virtual=true a platform thread is returned for ManagedExecutorService, ManagedExecutorScheduledService, ManagedThreadFactory, ForkJoinPool.

When testing on Java 21:

  • Deploys an application to that contains annotations and deployment descriptors with virtual=false and virtual=true configurations.
  • Asserts that when virtual=false a platform thread is returned for ManagedExecutorService, ManagedExecutorScheduledService, ManagedThreadFactory, ForkJoinPool.
  • Asserts that when virtual=true a platform thread is returned for a ForkJoinPool backed by a ManagedThreadFactory as defined by Java.
  • Assumes that when virtual=true a virtual thread is returned for ManagedExecutorService, ManagedExecutorScheduledService, ManagedThreadFactory
    • If a platform thread is returned, the test passes and we make no assertions about virtual thread behavior
    • If a virtual thread is returned, the test then makes assertions about virtual thread behavior

@KyleAure KyleAure added the TCK label Feb 5, 2024
@KyleAure KyleAure self-assigned this Feb 5, 2024
Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

This looks great Kyle! Thanks for writing this and demonstrating this approach to cover virtual=true in the TCK in a way that can be skipped on Java SE 17. I added a few minor comments - mostly on a typo of the word virtual that ended up copied to several places.

@KyleAure KyleAure force-pushed the 414-update-tck-for-virual-java-17 branch 2 times, most recently from 1804b4a to f4bcb3c Compare February 5, 2024 20:34
Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

Looks good! I noted one very minor type with two period characters (which might even be my fault if I mistyped my prior suggestion on that one). If you don't want to correct it, don't worry about it.

@Emily-Jiang
Copy link

I don't think you should skip the testPlatformThreadFactory for Java 17. It should work same as Java 21.

@njr-11
Copy link
Contributor

njr-11 commented Feb 7, 2024

I don't think you should skip the testPlatformThreadFactory for Java 17. It should work same as Java 21.

That will depend on the outcome of the vote that has yet to happen over what the behavior of virtual=true on Java SE 17 is. If vendor-specific wins, then we have no choice but to skip it because a vendor could choose to fail deployment. Kyle wrote this pull to investigate whether we would even be able to handle such an outcome, and did a very good job of figuring out a way. This pull will remain in pending state until a vote occurs and then be updated accordingly.

@Emily-Jiang
Copy link

I don't think you should fail the deployment when setting virutal=false on Java 17 as it is a valid scenario.
Even setting virutal=true on Java 17 might not fail deployment as the code would only be invoked during run time not something you can detect at the application startup stage.

@KyleAure KyleAure force-pushed the 414-update-tck-for-virual-java-17 branch 4 times, most recently from a857fb5 to 6c79fda Compare February 14, 2024 20:56
@KyleAure KyleAure marked this pull request as ready for review February 15, 2024 22:27
@KyleAure
Copy link
Contributor Author

@njr-11 I think this needs a re-review now that it has been updated to comply with the results of the vote.

Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

Thanks for updating this. I did spot some questionable assertions that are technically not guaranteed by the spec, although in most cases seem unlikely not to be the case.

@KyleAure KyleAure force-pushed the 414-update-tck-for-virual-java-17 branch from 6c79fda to 6e00848 Compare February 22, 2024 18:24
@KyleAure
Copy link
Contributor Author

@njr-11 I have handled all of the review comments you left. Please re-review when you get a chance.

Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

Added suggestions for very minor corrections. Otherwise looks good. Thanks for writing these!

KyleAure and others added 2 commits February 23, 2024 13:38
…ual/VirtualThreadServlet.java

Co-authored-by: Nathan Rauh <nathan.rauh@us.ibm.com>
…ual/VirtualThreadServlet.java

Co-authored-by: Nathan Rauh <nathan.rauh@us.ibm.com>
@KyleAure
Copy link
Contributor Author

This PR has been opened for a while and there have been no other comments/reviews. Now that all the comments have been resolved I'll go ahead and merge this.

@KyleAure KyleAure merged commit 1e346f3 into jakartaee:main Feb 23, 2024
4 checks passed
@KyleAure KyleAure deleted the 414-update-tck-for-virual-java-17 branch February 23, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants