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.

Sign up for GitHub

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

Jump to bottom

TCK updates for Servlet 6.0 #773

Merged
merged 18 commits into from
Jan 20, 2022
Merged

TCK updates for Servlet 6.0 #773

merged 18 commits into from
Jan 20, 2022

Conversation

markt-asf
Copy link
Contributor

name: Pull Request
about: Create a pull request for a Platform TCK change
title: 'Servlet 6.0 TCK updates'
labels: ''
assignees: ''

Fixes Issue
None

Related Issue(s)
None

Describe the change
Updates required to the Servlet TCK for the Servlet 6.0 release. Includes some commits that need to be back-ported to earlier TCK versions.

Additional context
None

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

@markt-asf
Copy link
Contributor Author

The "Ensure passed flag is correctly set on test failure" needs to be back-ported.

@scottmarlow
Copy link
Contributor

Thanks Mark for working on this, very much appreciated!

@markt-asf markt-asf changed the title Ensure passed flag is correctly set on test failure TCK updates for Servlet 6.0 Nov 22, 2021
@markt-asf
Copy link
Contributor Author

This PR is now at the point where a compliant Servlet 6.0 implementation should pass it. What is missing is new tests for new features.

@markt-asf
Copy link
Contributor Author

Just a rebase. No functional changes.

@markt-asf markt-asf marked this pull request as ready for review December 8, 2021 10:53
@gurunrao
Copy link
Contributor

@markt-asf - For readers of commit history, we can squash 17 commits of the PR. Also can we requested Servlet SPEC team to review the PR?

@markt-asf
Copy link
Contributor Author

I'd prefer that the commits weren't squashed into a single commit.

Obviously you have my +1 to these changes. I've asked on the servlet dev list for other committers to comment here.

@gurunrao
Copy link
Contributor

I'd prefer that the commits weren't squashed into a single commit.

Obviously you have my +1 to these changes. I've asked on the servlet dev list for other committers to comment here.

Please add any volunteers from servlet-dev list as reviewer of the PR.

@gurunrao
Copy link
Contributor

gurunrao commented Jan 4, 2022

I'd prefer that the commits weren't squashed into a single commit.

Obviously you have my +1 to these changes. I've asked on the servlet dev list for other committers to comment here.

@markt-asf - Please assign volunteers from servlet-dev for review.

@markt-asf
Copy link
Contributor Author

I asked for additional review volunteers on servlet-dev. No-one stepped forward.

@pzygielo
Copy link
Contributor

pzygielo commented Jan 8, 2022

Should the HttpUtils references be removed from ServletJavadocAssertions.html?

@markt-asf
Copy link
Contributor Author

Possibly. Does any actually use the assertion information? I'm wondering if we should continue to maintain it or just remove it entirely.

@pzygielo
Copy link
Contributor

pzygielo commented Jan 9, 2022

Asking only because it was updated in this PR but for other than HttpUtils reason.

@Thihup
Copy link
Contributor

Thihup commented Jan 12, 2022

Is fixing #736 related to this PR?

@markt-asf
Copy link
Contributor Author

@pzygielo If the assertion information stays then references to HttpUtils should be removed but I haven't yet seen an argument for keeping the assertion information.

@Thihup I haven't noticed any issues related to #736 while working on this but I have been using Java 11.

@scottmarlow
Copy link
Contributor

@Thihup I haven't noticed any issues related to #736 while working on this but I have been using Java 11.

@markt-asf According to https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/tests/servlet/spec/serverpush/README.md:

Once the TCK no longer needs to execute on Java 8, this package can be updated to use the java.net.http module in Java 11 onwards and lib/http.jar may be removed.

The servetpush test seems to still work on Java 17 (from WildFly testing with EE 9.1) but from the serverpush/README/md it sounds like lib/http.jar could be removed and the serverpush test changed to use the java.net.http package since EE 10 will require Java 11+ to run TCKs. I guess that doesn't have to be tied to this pr for Servlet 6.0 specific changes but it could.

@markt-asf
Copy link
Contributor Author

@scottmarlow Now I remember the hack referenced in #736. If we can get rid of this hack then that would be a good thing. I'll take a look today.

@markt-asf
Ensure passed flag is correctly set on test failure
0bd9200
@markt-asf
Remove the exclusion since PR 558 has been merged
fa4b540 
The PR amended the test to accept a 401 or 404
@markt-asf
Fix TCK bug in mapping test that led to challenge and re-enable test
1e6547d 
Challenges:
jakartaee/servlet#385
jakartaee#594
@markt-asf
Re-enable asyncStartedTest3 test as underlying issue has been resolved
5c7f816 
jakartaee/servlet#109
@markt-asf
Remove upgradeTest from platform exclusions
9454021 
It was never excluded at the Servlet level and the test passes for
Tomact 10.1.x so the assumption is that the test is valid.
@markt-asf
Remove exclusion resulting from challenge servlet-api#378
32e03e6 
The Servlet spec and APi has been updated in Servlet 6.0 to refer
exclusively to RFC 6265.
@markt-asf
Remove the SingleThreadModel tests as the interface has been removed
17a15fd
@markt-asf
Update major version test to 6 for Servlet 6.0
25b0e07
@markt-asf
Servlet 6.0 removed the restriction on calling these getter methods
a009fe9
@markt-asf
Fix the SessionCookieConfig tests now comment is unused
d7f95ea 
The test used comment to pass data. Comment is now a NO-OP so it can't
be used going forwards. The use of comment has been replaced with a
ServletContext attribute.
@markt-asf
Copy link
Contributor Author

I re-based the PR and added a commit to fix #736.

@gurunrao gurunrao mentioned this pull request Jan 20, 2022
Closed
@scottmarlow scottmarlow merged commit aa187c7 into jakartaee:master Jan 20, 2022
@markt-asf markt-asf deleted the servlet-6-tck branch February 1, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmarlow scottmarlow scottmarlow left review comments

@stuartwdouglas stuartwdouglas stuartwdouglas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@markt-asf @scottmarlow @gurunrao @pzygielo @Thihup @stuartwdouglas