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 reworked to JUnit 5 and reformatted #276

Merged
merged 8 commits into from
Feb 3, 2022

Conversation

Verdent
Copy link
Contributor

@Verdent Verdent commented Jun 14, 2021

No description provided.

@Verdent Verdent added the TCK label Jun 14, 2021
@Verdent Verdent requested review from lukasj and m0mus June 14, 2021 11:49
@Verdent Verdent self-assigned this Jun 14, 2021
Copy link

@lukasj lukasj left a comment

Choose a reason for hiding this comment

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

LGTM

nice!

private static void startContainer() {
try {
//Verify that CDI container is already running
CDI.current();

Choose a reason for hiding this comment

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

Note that the Platform TCK vehicle.properties indicated that servlet was needed for Standalone jsonb 2.0 TCK which looks wrong since the JSONB 2.0 spec doesn't mention that requirement. So this seems fine to remove testing with servlet.

Any disagreement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This definitely does not make sense :-) Servlet is not needed run these TCKs. I am not sure why it is there at all. Could it be originally because of the CDI? Anyway I think it can be removed since we are capable of testing CDI just fine.

Choose a reason for hiding this comment

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

I agree that servlet does not make sense as there aren't any servlet tests to run.

Choose a reason for hiding this comment

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

AFAIK to be JSONB certified you don't need servlet run, to be EE certified for the JSONB part you need servlet, ejb, ejb embed, jsp and jsf runs too. Know some spec limited that list to standalone/servlet as a shortcut but think it is worth testing in a EE container for the platform otherwise it is like not having any test - but fine to move this TCK part in the platform TCK and not in the jsonb tck bundle.

Choose a reason for hiding this comment

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

AFAIK to be JSONB certified you don't need servlet run, to be EE certified for the JSONB part you need servlet, ejb, ejb embed, jsp and jsf runs too. Know some spec limited that list to standalone/servlet as a shortcut but think it is worth testing in a EE container for the platform otherwise it is like not having any test - but fine to move this TCK part in the platform TCK and not in the jsonb tck bundle.

Technically, we can leave jsonb tests in the Platform TCK for EE 10 but in the long term, it would be good to have a single source for JSONB tests. Perhaps once we have finished refactoring the Jakarta EE TCK tests (e.g. switch to multiple Maven modules) we can then achieve that.

Copy link
Contributor

Choose a reason for hiding this comment

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

These two TCK bundles (in jakartatck and in this repository) are different. This bundle contains tests for all newly added features and has better tests coverage. We are not going to maintain two bundles because we cannot have two sources of truth. We should remove all JSONB tests from jakartatck repository or leave only tests related to CDI. @scottmarlow, do you want me to file an issue for this?

Copy link

@scottmarlow scottmarlow Jan 19, 2022

Choose a reason for hiding this comment

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

These two TCK bundles (in jakartatck and in this repository) are different. This bundle contains tests for all newly added features and has better tests coverage. We are not going to maintain two bundles because we cannot have two sources of truth. We should remove all JSONB tests from jakartatck repository or leave only tests related to CDI.

^ Makes sense but I think the Platform TCK still needs to validate any Platform requirements that the new JSONB TCK doesn't validate.

@scottmarlow, do you want me to file an issue for this?

Yes, please, I think that the removal of the JSONB TCK tests from the Platform TCK needs to avoid removing tests that validate the Platform requirements. Perhaps this could be to just leave the CDI tests in place (option 1), or we could leave a few (small number) of other JSONB tests in the Platform TCK as well to provide more coverage (option 2).

Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
Signed-off-by: David Kral <david.k.kral@oracle.com>
@Verdent Verdent merged commit 46a30c1 into jakartaee:master Feb 3, 2022
@KyleAure
Copy link

KyleAure commented Feb 3, 2022

@Verdent Sorry, I'm a bit late to the conversation on this PR, but I'm confused about the removal of Arquillian. Arquillian was being used as a mechanism for deploying and running these tests on application servers. Without that, and without any other documentation, how are application server implementations supposed to run these tests? I believe many of the other Jakarta EE specifications have been migrating their TCKs to use Junit/TestNG + Arquillian for a standard way to automate test deployments.

@rmannibucau
Copy link

There is some interesting work in JBatch about using junit5 primarly and be able to back by arquillian engine through an extension to run in a container without polluting tck code itself, can be worth checking since arquillian is not trendy nor attracting contributors today anymore.

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.

6 participants