-
Notifications
You must be signed in to change notification settings - Fork 109
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 servlet using Arquillian #912
TCK servlet using Arquillian #912
Conversation
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. |
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 |
@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 |
it's fine. I still have an issue I haven't figured out how to fix for 1 test!
first I will have to rebase from |
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 |
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
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 |
93e6ba4
to
09cad4d
Compare
29b3de8
to
e464785
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to get the servlet module and changes in common modules merged.
@scottmarlow are you fine with this one? |
Lets see if @markt-asf or @stuartwdouglas have feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
toStringBuilder
) 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.
@markt-asf TBH I spend months on that with frequent emails asking questions without any answers... Have you even try locally the PR? Github is not the only way to review PR. |
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 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. |
@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. |
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 |
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?
@stuartwdouglas
@markt-asf
I also gave feedback to github about improving the browser experience for large pull request reviewing but none of my suggestions were made. |
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
… on closing streams Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
6f801c7
to
f042c24
Compare
I'll go ahead and merge this change to the |
@olamy Thanks for your long and tireless effort on preparing this pull request! |
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 |
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