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

Initial version of core profile TCK #966

Merged
merged 4 commits into from
May 17, 2022
Merged

Initial version of core profile TCK #966

merged 4 commits into from
May 17, 2022

Conversation

starksm64
Copy link
Contributor

Describe the change
This is the candidate TCK for the new Jakarta Core Profile.

Additional context
When discussed where this should live on the platform group, we thought it was best to live under the jakartaee-tck project.

CC @alwin-joseph @anajosep @arjantijms @cesarhernandezgt @dblevins @m0mus @edbratt @gurunrao @jansupol @jgallimore @kazumura @kwsutter @LanceAndersen @bhatpmk @RohitKumarJain @shighbar @gthoman @brideck @scottmarlow

Signed-off-by: Scott M Stark <starksm64@gmail.com>
@@ -0,0 +1,201 @@
Apache License
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi for other reviewers, https://blogs.eclipse.org/post/wayne-beaton/eclipse-project-licenses states that Eclipse projects already are using Apache License Version 2.0

<mkdir dir="${project.build.directory}/tcks"/>
<!-- download file -->
<get src="https://download.eclipse.org/jakartaee/jsonp/2.1/jakarta-jsonp-tck-2.1.0.zip"

Copy link
Contributor

Choose a reason for hiding this comment

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

As a user that has installed the core-profile-tck distribution, is it okay if I manually update the installed copy of this file to update to new versions of jsonp-tck (e.g. to use 2.1.1.zip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is just what matches the current dependencies stated in the TCK. If the component specs release new service releases they can be used. Do we need to say something to that effect?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, would be good to state that users can manually update their local installed copy of artifacts-pom.xml to reference new service releases (e.g. any new TCK releases in https://download.eclipse.org/jakartaee/jsonp/2.1).

Copy link
Contributor

@scottmarlow scottmarlow May 12, 2022

Choose a reason for hiding this comment

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

I created tracking issue #975 for making this change so this pr can be merged.

@scottmarlow
Copy link
Contributor

Looks great other than mentioned (minor) feedback.

Signed-off-by: Scott M Stark <starksm64@gmail.com>
.withTransitivity()
.asResolvedArtifact();

WebArchive archive = ShrinkWrap.create(WebArchive.class, "jsonp-tck-tests-all.war").as(WebArchive.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps I do not understand this well. But the core profile does not support servlet, how come it should support a war archive? Would e.g. Quarkus or Helidon be happy with Arquillian deploying a war?

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, Jakarta REST TCK contains mainly wars. But still, no container to deploy to...

Copy link
Contributor

Choose a reason for hiding this comment

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

But the core profile does not support servlet

I've always felt we're making our lives more difficult by officially not supporting Servlet, but then practically assuming Servlet anyway.

Maybe we should clearly state why we think we dislike Servlet, and why we need to officially avoid it. Based on that we may want to discuss if a servlet-lite is feasible, which excludes all the things we think we need to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We talked a bit about this at the end of the last platform call. We don't have a pure LibraryContainer archive type in Arquillian that can be used, so WebArchive is used as a proxy. We had to do this in CDI Lite as well. This is something that needs to be address going forward.

Restful does not support servlet either, but its TCK is what is bringing in the requirement to use a servlet. One aspect is wanting a lighter servlet api, but the other is allowing for the possibility to use another invocation mechanism. As of now, supporting servlets can be considered a TCK porting package implementation detail. We will need to address this in the future if a compatible implementation challenges this aspect of the TCK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Restful does not support servlet either

If you mean Jakarta RESTful Web Services, then the Servlet 2.5 container was the only mandatory environment that needs to be supported by each implementation. That is why the TCK chose to use it. See https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-3.1.html#servlet. Since 3.1 there is another possibility for SE Bootstrap, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this at least allows the runtimes that support servlet execute the json-p and json-b tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point was that JSON-P and JSON-B are standalone tests only, they do not need any container to be run. I do not mind this being resolved later, though, if that's an Arquillian issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @starksm64 demonstrates how to get them running in a container, which is useful for EE container as a means to execute the tests.

Copy link
Contributor

@gurunrao gurunrao May 13, 2022

Choose a reason for hiding this comment

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

IMO, we should have consistent approach for JSONP tests running in a container or as a standalone for all of Jakarta EE profiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For EE10 the standalone approach is all that is required. The in container module is just an example for future work or for containers to experiment with.

core-profile-tck/pom.xml Show resolved Hide resolved
<include>jakarta.ee.tck:core-tck-parent</include>
<include>jakarta.ee.tck:cdi-lite-tck-suite</include>
<include>jakarta.ee.tck:core-profile-tck-impl</include>
<include>jakarta.ee.tck:core-tck-jsonp-extension</include>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do the same thing for jsonb?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Emily-Jiang could you please create a tracking Platform TCK issue for adding a tck-jsonb-extension similar to jakarta.ee.tck:core-tck-jsonp-extension.

@scottmarlow
Copy link
Contributor

Merging later today so we can start building this TCK via CI. Further changes should be made via separate pull requests.

@scottmarlow scottmarlow merged commit 1d5346e into jakartaee:master May 17, 2022
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.

6 participants