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

rework TCK to Junit5 #297

Merged
merged 7 commits into from
Nov 12, 2021
Merged

rework TCK to Junit5 #297

merged 7 commits into from
Nov 12, 2021

Conversation

aserkes
Copy link
Contributor

@aserkes aserkes commented Jul 19, 2021

rework TCK to Junit5, replace System.out/err.println by java.util.logging.Logger

Signed-off-by: aserkes andrii.serkes@oracle.com

…ging.Logger

Signed-off-by: aserkes <andrii.serkes@oracle.com>
Copy link
Contributor

@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, @scottmarlow what do you think?

@scottmarlow
Copy link
Contributor

I noticed that this removes Arquillian which I think was added via #232. Why are removing Arquillian when I think Arquillian supports JUnit5 now?

@lukasj
Copy link
Contributor

lukasj commented Oct 5, 2021

@scottmarlow I believe first step was to move tests from the platform to this project as is, this second step is to simplify them. I believe that at least 99% tests do not require server-like environment and even that 1%, if it is there, can be rewritten to not require it.

@aserkes correct me if I'm wrong

lukasj
lukasj previously approved these changes Oct 5, 2021
Copy link
Contributor

@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

Signed-off-by: aserkes <andrii.serkes@oracle.com>
# Conflicts:
#	tck/tck-tests/src/main/java/jakarta/jsonp/tck/api/jsonparsertests/ClientTests.java
Signed-off-by: aserkes <andrii.serkes@oracle.com>
@scottmarlow
Copy link
Contributor

@scottmarlow I believe first step was to move tests from the platform to this project as is, this second step is to simplify them. I believe that at least 99% tests do not require server-like environment and even that 1%, if it is there, can be rewritten to not require it.

At the Jakarta Platform (EE 9) specification level, I found https://jakarta.ee/specifications/platform/9/jakarta-platform-spec-9.html#json-processing-2-0-json-p-requirements states:

6.14. JSON Processing 2.0 (JSON-P) Requirements

JSON (JavaScript Object Notation) is a lightweight data-interchange format used by many web services. The Jakarta JSON Processing (JSON-P) provides a convenient way to process (parse, generate, transform, and query) JSON text.

In a full Jakarta EE product, all Jakarta EE application client containers, web containers, and enterprise beans containers are required to support the JSON-P API.

The Jakarta JSON Processing specification can be found at https://jakarta.ee/specifications/jsonp .

In the Platform TCK, there is a list of which vehicles are used to test each technology as pointed out by @gurunrao (thanks Guru!).
At the platform TCK level, https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/install/jakartaee/other/vehicle.properties shows the following which I think is proving that jsonp can be used within each of the containers mentioned above:

com/sun/ts/tests/jsonp = ejb jsp servlet appclient
com/sun/ts/tests/jsonp/pluggability = ejb jsp servlet appclient

@edbratt helped me to better understand the TCK requirements for Platform versus component Spec level TCK yesterday in comments on TCK call discussion..

In summary, if the component SPEC TCK doesn't meet the EE 10 Platform requirements, it is up to the Platform TCK to meet the Platform level requirements. The Jakarta Platform committers could also vote to not require the Platform TCK to test that jsonp works as expected in the mentioned EE containers, in which case it would be sufficient for the component SPEC TCK to only run testing in Java SE mode.

@lukasj do you have feedback on whether you think the Platform TCK should continue to verify that use of JSONP in the stated EE containers, does work as expected? If yes, will there be changes in the JSONP TCK to support running in the different EE containers? The JSONP tests would need to run in both Web Profile + Full Platform as per keywords.properties.

CC @alwin-joseph @gurunrao

@lukasj
Copy link
Contributor

lukasj commented Oct 15, 2021

@scottmarlow you're basically saying, that arquillian must stay since platform tck has no way to "wrap" standalone test into a vehicle for appropriate container, right?

@scottmarlow
Copy link
Contributor

@scottmarlow you're basically saying, that arquillian must stay since platform tck has no way to "wrap" standalone test into a vehicle for appropriate container, right?

Regardless of where the work is done (either in jsonp tck or platform tck or both), what are our current options for being able to run the junit tests on ejb jsp servlet appclient in some way?

@lukasj
Copy link
Contributor

lukasj commented Oct 18, 2021

@scottmarlow Can't the example by mail be followed? There is standalone TCK for jakarta mail (mail-tck repo) and jakarta mail specific platform-level tests see https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/src/com/sun/ts/tests/javamail

@jbescos
Copy link
Member

jbescos commented Oct 18, 2021

@scottmarlow you're basically saying, that arquillian must stay since platform tck has no way to "wrap" standalone test into a vehicle for appropriate container, right?

Regardless of where the work is done (either in jsonp tck or platform tck or both), what are our current options for being able to run the junit tests on ejb jsp servlet appclient in some way?

I was discussing this with @lukasj and we understand this in different way. Lets clarify what is the meaning of running junit tests on ejb jsp servlet appclient.

This is my understanding:

Some TCKs need to deploy some war or ear files in a servlet container (glassfish, weblogic, etc). In this case, the ant script was compiling and deploying the code in the sevlet container before the TCK tests starts to run.

Then, the TCK tests make some requests to the server, then the server run any operation and returns back the result to the TCK. Then TCK evaluates it and checks whether fails or success.

Few observations from this:

  • Server is never running TCKs, this is done by the client. There is no need to have JUnit or harness dependency in the server.
  • Some projects are lucky (like jakarta activation). They don't depend on deployments in the server. We don't need to care about servlet containers in this case, right?.

@scottmarlow
Copy link
Contributor

@lukasj

@scottmarlow Can't the example by mail be followed? There is standalone TCK for jakarta mail (mail-tck repo) and jakarta mail specific platform-level tests see https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/src/com/sun/ts/tests/javamail

Yes, that is the both approach IMO, regardless of how it is accomplished (via Java Test Harness, Junit and/or something else).

@jbescos

I was discussing this with @lukasj and we understand this in different way. Lets clarify what is the meaning of running junit tests on ejb jsp servlet appclient.

This is my understanding:

Some TCKs need to deploy some war or ear files in a servlet container (glassfish, weblogic, etc). In this case, the ant script was compiling and deploying the code in the sevlet container before the TCK tests starts to run.

Then, the TCK tests make some requests to the server, then the server run any operation and returns back the result to the TCK. Then TCK evaluates it and checks whether fails or success.

Few observations from this:

* Server is never running TCKs, this is done by the client. There is no need to have JUnit or harness dependency in the server.

Agreed that server is never running TCKs but test harness is needed in the server currently for some TCKs. https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/user_guides/jakartaee/src/main/jbake/content/troubleshooting.adoc mentions this (as does https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/user_guides/jaxrs/src/main/jbake/content/config.inc). Look for tsharness.jar in either file (also mentioned in a few other user guide doc files).

* Some projects are lucky (like jakarta activation). They don't depend on deployments in the server. We don't need to care about servlet containers in this case, right?.

Correct, I'm only raising a concern with new component TCKs that contain tests that we were discussing removing from the Platform tck. The concern that I meant to raise is that Jakarta EE Platform Specification requirements need to still be validated in some way that meets the expectation of Jakarta EE Platform team/committers.

So, with the current jsonp TCK approach of not using Arquillian, I think we should leave enough jsonp tests in the Platform TCK to validate that json works as expected for the various Jakarta EE Platform vehicles.

Is it possible for the Platform TCK jsonp tests to do its validation using the new JSONP SPEC component TCK test classes? If yes, then in setting that up in the Platform TCK, it seems that the duplicate classes could be deleted from the Platform TCK.

In terms of how the user running the Platform TCK will obtain the correct JSONP SPEC component TCK that contains those test classes is to be worked out. Perhaps the Platform TCK could include a JSONP pom.xml that specifies the https://download.eclipse.org/jakartaee/jsonp/2.1/jakarta-jsonp-tck-2.1.0.zip but the user could manually update to a newer jakarta-jsonp-tck-2.1.x version as JSONP service releases become available. This JSONP pom.xml in the Platform TCK could copy json TCK artifact(s) to the Platform TCK lib for the Platform TCK JSONP tests to use.

@lukasj
Copy link
Contributor

lukasj commented Oct 18, 2021

@scottmarlow I'm really lost now. I'd expect there's clear guidance by the platform available... If I want to submit spec PR this Friday, I need TCK. To have TCK, I need standalone version (since jsonp is part of the core profile) and platform side of things finalized as well (correct me if I'm wrong). So what do we have to do? And more important - how? Isn't "duplicity" - no deletion from the platform TCK - way to go for EE 10?

@scottmarlow
Copy link
Contributor

@scottmarlow I'm really lost now. I'd expect there's clear guidance by the platform available...

The current Platform requirements are already clearly stated in the Platform SPEC (for example see json-processing-2-0-json-p-requirements). If we change our current Platform TCK to not validate Jakarta EE application client containers, web containers, and enterprise beans containers are required to support the JSON-P API, depending on the change we make in the Platform TCK, we could need guidance from the Platform.

Regarding the Platform, we currently have Platform requirements for what the Platform TCK requires. If we decide to question those requirements, IMO whether that delays JSONP or not is up to you. You can say no more changes to the JSONP SPEC

If I want to submit spec PR this Friday, I need TCK. To have TCK, I need standalone version (since jsonp is part of the core profile) and platform side of things finalized as well (correct me if I'm wrong).

If we submit JSONP spec PR this Friday, we need the JSONP SPEC TCK to be complete in the sense that we understand that no more JSONP SPEC TCK changes will be made after JSONP SPEC goes final on Friday.

So what do we have to do?

I would like to acknowledge the possible plan for handling the Platform validation of use of JSONP SPEC API.

And more important - how? Isn't "duplicity" - no deletion from the platform TCK - way to go for EE 10?

Possible outcomes from deletion of all Platform JSONP tests are (in no particular order):

  1. Platform requirements for JSONP SPEC are met via signature test verification of JSONP API classes on classpath. This change would need to be discussed with Platform team.
  2. Or Platform requirements for JSONP SPEC are met via use of JSONP SPEC TCK tests in ejb jsp servlet appclient.
  3. Or Platform requirements for JSONP SPEC use are changed (I'm not suggesting we follow this path but its an option).

Does ^ help?

@lukasj
Copy link
Contributor

lukasj commented Oct 18, 2021

@scottmarlow

The current Platform requirements are already clearly stated in the Platform SPEC

whole chapter 6 is about requirements. How the platform TCK wants to validate these requirements for specs providing standalone TCKs? Or is standalone TCK required to validate platform-level requirements?

...we need the JSONP SPEC TCK to be complete...

That's the plan. To achieve TCK completion, one needs to know how the platform wants to validate requirements from the chapter 6 in the platform spec and/or what the platform TCK needs from the standalone TCK in order to be able to perform such validation. Does the platform require Arquillian based test(s) to be available?

And more important - how? Isn't "duplicity" - no deletion from the platform TCK - way to go for EE 10?

Possible outcomes from deletion of all Platform JSONP tests are (in no particular order):

I did not ask for removal of existing tests from the jakartaee-tck repo. I was asking if keeping them "duplicated" in the platform TCK - in the other words not removing them (or at least some of them) - for EE 10 does solve the problem with validation of the platform level requirement. If so, the question of using Arquillian in the standalone version of the TCK is, IMO, irrelevant.

@scottmarlow
Copy link
Contributor

@scottmarlow

The current Platform requirements are already clearly stated in the Platform SPEC

whole chapter 6 is about requirements. How the platform TCK wants to validate these requirements for specs providing standalone TCKs?

We have a few options. IMO, JSONP SPEC TCK shouldn't be blocked on dealing with Platform requirements but some component TCKs do deal with Platform requirements (CDI, BV), so I wanted you to have a chance to do that also if you were interested.

Or is standalone TCK required to validate platform-level requirements?

No, the standalone TCK is not required to validate platform-level requirements.

...we need the JSONP SPEC TCK to be complete...

That's the plan. To achieve TCK completion, one needs to know how the platform wants to validate requirements from the chapter 6 in the platform spec and/or what the platform TCK needs from the standalone TCK in order to be able to perform such validation. Does the platform require Arquillian based test(s) to be available?

No, the platform does not require Arquillian based test(s) to be available. If they were, that might be helpful but is not required. Even if Arquillian tests were available, its not clear if we could use them in the Platform TCK (we just might be able to use them).

And more important - how? Isn't "duplicity" - no deletion from the platform TCK - way to go for EE 10?

Possible outcomes from deletion of all Platform JSONP tests are (in no particular order):

I did not ask for removal of existing tests from the jakartaee-tck repo. I was asking if keeping them "duplicated" in the platform TCK - in the other words not removing them (or at least some of them) - for EE 10 does solve the problem with validation of the platform level requirement. If so, the question of using Arquillian in the standalone version of the TCK is, IMO, irrelevant.

Yes, sorry, if we keep some or all of existing tests from the jakartaee-tck repo that could help ensure we can validate the platform level requirements.

# Conflicts:
#	tck/pom.xml
#	tck/tck-tests/src/main/java/jakarta/jsonp/tck/api/jsonobjecttests/ClientTests.java
#	tck/tck-tests/src/main/java/jakarta/jsonp/tck/api/jsonvaluetests/ClientTests.java
Signed-off-by: aserkes <andrii.serkes@oracle.com>
Signed-off-by: aserkes <andrii.serkes@oracle.com>
Copy link
Contributor

@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

@lukasj lukasj merged commit 7d8d30f into jakartaee:master Nov 12, 2021
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.

4 participants