TCK servlet using Arquillian

[olamy]

Fixes Issue
Specify the issue (link) that is solved with this pull request.

Related Issue(s)
Specify any related issue(s) links.

Describe the change
A clear and concise description of the change.

Additional context
Add any other context about the problem here.

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

[arjantijms]

Btw, just in case it might be helpful, I moved a number of tests from the Servlet TCK to junit / Arquillian before.

See here: https://github.com/javaee-samples/jakartaee-samples/tree/main/ee9-tck/servlet

[olamy]

Btw, just in case it might be helpful, I moved a number of tests from the Servlet TCK to junit / Arquillian before.

See here: https://github.com/javaee-samples/jakartaee-samples/tree/main/ee9-tck/servlet

thanks for the pointer! This looks very similar to what has been done here.
Except I didn't go so deep with a pom per test class :)
currently I'm more struggle with enabling https (clientcert) and http2 listener :)

[arjantijms]

Arquillian famously doesn't support the https URL injection. There's a PR open for that for almost longer than Arquillian exists, but it just doesn't get merged (probably should be start from scratch after the odd decade it's open)

[olamy]

Arquillian famously doesn't support the https URL injection. There's a PR open for that for almost longer than Arquillian exists, but it just doesn't get merged (probably should be start from scratch after the odd decade it's open)

ah
here it's only used to get hostname and port (which can be random with Jetty look at here https://github.com/jetty-project/servlet-tck-run/blob/main/src/test/resources/arquillian.xml )

[scottmarlow]

@olamy how are things going with these changes?

What would it take to bring your refactored Servlet TCK changes into a new folder in the master branch?

[olamy]

@olamy how are things going with these changes?

it's fine. I still have an issue I haven't figured out how to fix for 1 test!
com.sun.ts.tests.servlet.spec.security.clientcert
And sorry for that but I had other work to do and gave up.
the test which include client certificate. I cannot see how to have the glassfish arquillian setup correctly glassfish to start a https connector with the certificate.
I don;t really want to add some specific glassfish code in the TCK. (I got this working with Jetty because everything is done in the arquillian impl)
not sure if someone can help? (tbh I'm not really glassfish expert and do not really intend to be ;) )

What would it take to bring your refactored Servlet TCK changes into a new folder in the master branch?

first I will have to rebase from master branch to include few of the recent changes (the current PR is based on a very old master). This is probably 1 or 2 weeks of work.
I will start now.

[arjantijms]

the test which include client certificate. I cannot see how to have the glassfish arquillian setup correctly glassfish to start a https connector with the certificate.

The https connector is always there. But adding a certificate to a server is always a server specific operation. Getting the https port is something that Arquillian "forgot". We've been trying for almost 10 years to get this support into Arquillian, but it always fails for some reason. See arquillian/arquillian-core#43

On GlassFish https is on a different port, not sure how you did that with Jetty and got the correct port in the tests.

There's a client-cert test here, maybe it's helpful: https://github.com/javaee-samples/jakartaee-samples/tree/main/ee7/servlet/security-clientcert

[olamy]

the test which include client certificate. I cannot see how to have the glassfish arquillian setup correctly glassfish to start a https connector with the certificate.

The https connector is always there. But adding a certificate to a server is always a server specific operation. Getting the https port is something that Arquillian "forgot". We've been trying for almost 10 years to get this support into Arquillian, but it always fails for some reason. See arquillian/arquillian-core#43

On GlassFish https is on a different port, not sure how you did that with Jetty and got the correct port in the tests.

well I don't really mind to have something specific to say this is an https port at the end it's an endpoint accepting http connection with eventually tls and cert mandatory ;)

in Jetty via arquilian it;s done as this https://github.com/jetty-project/servlet-tck-run/blob/1268f324de54dd9cdbac05aeb7a4734982e4f15a/src/test/resources/arquillian.xml#L25

<container qualifier="https"> then in the TCK code we use this id https to run to deploy to see https://github.com/olamy/jakartaee-tck/blob/981fa5e53bd4f8a1188e044bfb0acd3d951e6d3c/servlet/src/main/java/com/sun/ts/tests/servlet/spec/security/clientcert/Client.java#L80

There's a client-cert test here, maybe it's helpful: https://github.com/javaee-samples/jakartaee-samples/tree/main/ee7/servlet/security-clientcert

I try a bit here https://github.com/olamy/jakartaee-tck/blob/981fa5e53bd4f8a1188e044bfb0acd3d951e6d3c/servlet/src/main/java/com/sun/ts/tests/servlet/spec/security/clientcert/Client.java#L84
but I feel like adding some specific glassfish here which I don't think it;s right as this should be done by the arquilian container. Is it possible to delegate this to arquilian glassfish?

[alwin-joseph]

+1 to get the servlet module and changes in common modules merged.

[olamy]

@scottmarlow are you fine with this one?

[scottmarlow]

@scottmarlow are you fine with this one?

Lets see if @markt-asf or @stuartwdouglas have feedback.

[markt-asf]

This PR is too big to review. It is so big that the GitHub review UI constantly locks up while trying to perform the review.
What I have been able to determine:

• The bulk of the changes are contained in a single commit. This needs to be broken down into separate commits / steps in the conversion process so reviewers can follow what is happening and review the changes. If it can be broken down in separate PRs, even better.
• The PR contains a large number of purely formatting changes. As a minimum these should be in a separate commit and ideally an entirely separate PR.
• The PR contains a large number of refactoring changes (e.g. switch from StringBuffer to StringBuilder) unrelated to the switch to Arquillian. As a minimum these should be in a separate commit and ideally an entirely separate PR.
• The commit adds several (possibly more I wasn't able to review most of the changes) TODO comments. These suggest that the PR is not complete.

It is likely I will have further comments once I am able to review the PR in its entirety.

[olamy]

@markt-asf TBH I spend months on that with frequent emails asking questions without any answers...
And now you are asking to redo everything to split into few commits... (with sorry to say but nitpicking on StringBuffer to StringBuilder)
Really?
this branch is used daily on the Jetty project to run TCK as it turn down TCK execution from more than 1h30 to less than 20minutes and it offers more feature than the current technology of the TCK
This is converting the servlet TCK to use Arquillian to run test. Not sure what else to add?
Regarding format, I started a discussion on the dev list and even a PR (#1150) again no real answer. And now format is the problem with this PR?

Have you even try locally the PR? Github is not the only way to review PR.
sorry for my reaction but sad to see how such huge effort get such feedback.

[markt-asf]

Thank you for explaining why this change is being made. That is useful information that I hadn't found anywhere else. A reduction in runtime to under 20 minutes is a huge improvement and very welcome.

If the TCK project team didn't respond to your questions you need to take that up with the TCK project. I was asked to review this PR as a member of the Servlet project late Monday evening my time and I provided that feedback less than 36 hours later. There was no prior request from you (or anyone else) asking for feedback on this PR on the Servlet project's mailing lists.

My comment regarding StringBuffer vs StringBuilder was just the first example I came across of refactorings unrelated to the move to Arquillian that added noise to this PR.

I have no objection to cleaning up the code nor to the various refactorings that improve the quality of the code. My objection is to those changes obscuring the changes that are part of the migration to Arquillian.

Irrespective of whether GitHub's UI can handle the size of this PR, it is too big for me to review regardless of how it is reviewed.

My review comments stand.

[stuartwdouglas]

@markt-asf being realistic there is not really any way to do this without it being to big to review. At a minimum it requires an @deployment method and @test annotations being added to every class/test, and that alone will overwhelm the github UI.

I have not really been following the broader TCK project, but have other projects already done a similar change? Arquillian itself is kinda in maintenance mode, and making this change may require other implementations to have to create an Arquillian adapter to run the TCK.

[markt-asf]

I disagree that this PR can't be structured in a way that makes it reviewable. The size isn't the (main) issue at this point. It is the size combined with the unrelated changes. My request is not for a PR that is reviewable in the GitHub UI. My request is for a PR that is reviewable. I'm quite happy reviewing a text diff. Large commits that make bulk changes (such as adding @Test annotations) are easy to review.

[scottmarlow]

Have you even try locally the PR?
@olamy I assume you mean that Servlet 6.0 implementations should try running against the PR. Or do you mean that we should review it from the command line instead of in a browser?

Regardless, it would be good to hear from different Servlet 6.0 implementations on whether they can pass the TCK with the changes. Does that make sense to pursue?

I have not really been following the broader TCK project, but have other projects already done a similar change?

@stuartwdouglas
Yes, for example https://github.com/jakartaee/rest/pulls?q=is%3Apr+is%3Aclosed+TCK+Migration%3A+, other Specifications also did as well for EE 10.

I disagree that this PR can't be structured in a way that makes it reviewable. The size isn't the (main) issue at this point. It is the size combined with the unrelated changes. My request is not for a PR that is reviewable in the GitHub UI. My request is for a PR that is reviewable. I'm quite happy reviewing a text diff. Large commits that make bulk changes (such as adding @test annotations) are easy to review.

@markt-asf
For reviewing big pull requests myself, I found the most relief from using different git options, but my eyes still didn't enjoy the process (mostly during the javax => jakarta namespace change in the Platform TCK which had several greater than 60k line PRs). Some of the options that helped a little bit were using shortcuts like:

alias "gitdiff=git diff -w --no-ext-diff --unified=0  $* | diff-highlight"
alias "gitshow=git show --unified=0 $* | diff-highlight"
alias "gitshowdeletes=git show --unified=0 --diff-filter=D $*"
alias "gitnames=git show --unified=0 --name-only $*"
alias "gitnossl=git -c http.sslVerify=false $*"
alias "gitshowdeletesnames=git show --unified=0 --diff-filter=D --name-only $*"
alias "gitshownodeletes=git show --unified=0 --diff-filter=ACMRTUXB $*"

I also gave feedback to github about improving the browser experience for large pull request reviewing but none of my suggestions were made.

[scottmarlow]

@scottmarlow @starksm64 all good to merge it to tckrefactor branch.

I'll go ahead and merge this change to the tckrefactor branch where we are refactoring all of the EE 11 Platform TCK tests + the Standalone TCKs generated from the Platform TCK.

[scottmarlow]

@olamy Thanks for your long and tireless effort on preparing this pull request!

[arjantijms]

Have you even try locally the PR?
@olamy I assume you mean that Servlet 6.0 implementations should try running against the PR. Or do you mean that we should review it from the command line instead of in a browser?

Regardless, it would be good to hear from different Servlet 6.0 implementations on whether they can pass the TCK with the changes. Does that make sense to pursue?

please note this PR includes a module called servlet-tck submodule of glassfish-runner which include some setup to run the change with glassfish 6.2.5 (sorry I cannot use more recent version as the arquilian container I found looks to not support it)

Which Arquillian container did you try? The recommended one of course supports GlassFish 7 as we continuously use it to pass the EE 10 TCKs and certify for it.

See https://github.com/OmniFish-EE/arquillian-container-glassfish