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

jetty-12 ee9 Omnibus tck failure analysis #9760

Closed
janbartel opened this issue May 11, 2023 · 4 comments
Closed

jetty-12 ee9 Omnibus tck failure analysis #9760

janbartel opened this issue May 11, 2023 · 4 comments
Assignees
@janbartel
Labels
Bug For general bugs on Jetty side TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc)

Comments

@janbartel
Copy link
Contributor

janbartel commented May 11, 2023 •
edited
Loading

As of build https://jenkins.webtide.net/job/tck/job/servlettck-run-12.0.x-ee9/299/ there are 64 failing ee9 tests. This issue is to track their analysis in a central place.

Note that: the CI build is running against servlet-tck-5, whilst local testing is running against servlet-tck-6 as that is the only version that has been extensively modified with arquillian etc to allow individual tests to run.

works in servlet-6-tck

com/sun/ts/tests/servlet/spec/security/secform/Client.java.test11
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test1
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test10
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test13
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test14
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test17
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test2
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test3
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test4
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test6
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test1_anno
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test2_anno
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test3_anno
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test4_anno
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test6_anno
com/sun/ts/tests/servlet/spec/security/secform/Client.java.test14_anno
com/sun/ts/tests/servlet/spec/requestdispatcher/URLClient.java.getRequestAttributes
com/sun/ts/tests/servlet/spec/requestdispatcher/URLClient.java.getRequestAttributes1
com/sun/ts/tests/servlet/spec/requestdispatcher/URLClient.java.getRequestAttributes3
com/sun/ts/tests/servlet/spec/requestdispatcher/URLClient.java.getRequestAttributes4
com/sun/ts/tests/servlet/spec/requestdispatcher/URLClient.java.getRequestAttributes6
com/sun/ts/tests/servlet/spec/requestdispatcher/URLClient.java.getRequestURIForwardTest
com/sun/ts/tests/servlet/spec/requestdispatcher/URLClient.java.getRequestURLForwardTest
com/sun/ts/tests/servlet/api/jakarta_servlet_http/cookie/URLClient.java.setMaxAgeNegativeTest
com/sun/ts/tests/servlet/api/jakarta_servlet_http/cookie/URLClient.java.setMaxAgeZeroTest
com/sun/ts/tests/servlet/api/jakarta_servlet_http/cookie/URLClient.java.setPathTest
com/sun/ts/tests/servlet/pluggability/api/jakarta_servlet_http/cookie/URLClient.java.setMaxAgeNegativeTest
com/sun/ts/tests/servlet/pluggability/api/jakarta_servlet_http/cookie/URLClient.java.setMaxAgeZeroTest
com/sun/ts/tests/servlet/pluggability/api/jakarta_servlet_http/httpservletrequest/URLClient.java.getContextPathTest
com/sun/ts/tests/servlet/pluggability/api/jakarta_servlet_http/httpservletrequestwrapper/URLClient.java.getContextPathTest
com/sun/ts/tests/servlet/api/jakarta_servlet_http/httpservletrequest/URLClient.java.getContextPathTest
com/sun/ts/tests/servlet/api/jakarta_servlet_http/httpservletrequestwrapper/URLClient.java.getContextPathTest
com/sun/ts/tests/servlet/spec/serverpush/Client.java.serverPushTest
com/sun/ts/tests/servlet/spec/serverpush/Client.java.serverPushMiscTest
com/sun/ts/tests/servlet/spec/serverpush/Client.java.serverPushNegtiveTest

cross context dispatch

com/sun/ts/tests/servlet/api/jakarta_servlet_http/httpsessionx/URLClient.java.expireHttpSessionxrfTest
com/sun/ts/tests/servlet/api/jakarta_servlet_http/httpsessionx/URLClient.java.expireHttpSessionxri1Test
com/sun/ts/tests/servlet/api/jakarta_servlet_http/httpsessionx/URLClient.java.expireHttpSessionxriTest
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.dispatchAfterCommitTest4
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.dispatchAfterCommitTest5
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.dispatchReturnTest4
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.dispatchReturnTest5
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.negativeDispatchTest12
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.negativeDispatchTest13
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.negativeDispatchTest8
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.negativeDispatchTest9
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.startAsyncAgainTest12
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.startAsyncAgainTest13
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.startAsyncAgainTest14
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.startAsyncAgainTest15
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.startAsyncAgainTest16
com/sun/ts/tests/servlet/api/jakarta_servlet/dispatchtest/URLClient.java.startAsyncAgainTest17

does not exist in servlet-6-tck

com/sun/ts/tests/servlet/api/jakarta_servlet_http/httpsessionx/URLClient.java.invalidateHttpSessionxTest

tck challenged

com/sun/ts/tests/servlet/spec/httpservletresponse/URLClient.java.flushBufferTest

to investigate

com/sun/ts/tests/servlet/api/jakarta_servlet_http/httpupgradehandler/URLClient.java.upgradeTest - covered by #9657
com/sun/ts/tests/servlet/api/jakarta_servlet_http/cookie/URLClient.java.getPathTest - covered by #9762
com/sun/ts/tests/servlet/api/jakarta_servlet_http/cookie/URLClient.java.getDomainTest - covered by #9762
com/sun/ts/tests/servlet/api/jakarta_servlet_http/cookie/URLClient.java.getVersionTest - covered by #9762
com/sun/ts/tests/servlet/pluggability/api/jakarta_servlet_http/cookie/URLClient.java.getDomainTest - covered by #9762
com/sun/ts/tests/servlet/pluggability/api/jakarta_servlet_http/cookie/URLClient.java.getPathTest - covered by #9762
com/sun/ts/tests/servlet/pluggability/api/jakarta_servlet_http/cookie/URLClient.java.getVersionTest - covered by #9762
com/sun/ts/tests/servlet/spec/serverpush/Client.java.serverPushCookieTest - covered by #9766
com/sun/ts/tests/servlet/spec/serverpush/Client.java.serverPushSessionTest2 - covered by #9766
@janbartel janbartel added Bug For general bugs on Jetty side TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc) Jetty 12 labels May 11, 2023
@janbartel janbartel self-assigned this May 11, 2023
@gregw gregw moved this to 🏗 In progress in Jetty 12.0.0.beta2 Jun 6, 2023
@janbartel
Copy link
Contributor Author

Following tests are still failing after initial fixes:

com/sun/ts/tests/servlet/api/jakarta_servlet_http/cookie/URLClient.java.getPathTest - covered by #9762
com/sun/ts/tests/servlet/api/jakarta_servlet_http/cookie/URLClient.java.getDomainTest - covered by #9762
com/sun/ts/tests/servlet/api/jakarta_servlet_http/cookie/URLClient.java.getVersionTest - covered by #9762
com/sun/ts/tests/servlet/pluggability/api/jakarta_servlet_http/cookie/URLClient.java.getDomainTest - covered by #9762
com/sun/ts/tests/servlet/pluggability/api/jakarta_servlet_http/cookie/URLClient.java.getPathTest - covered by #9762
com/sun/ts/tests/servlet/pluggability/api/jakarta_servlet_http/cookie/URLClient.java.getVersionTest - covered by #9762

When running locally (with arquillian and tck 6.0.0) these tests work. When running in ci against the 5.0.2 tck they don't work.
Checking the tck code for eg the getPathTest we see that the 5.0.2 is testing the cookie differently, and looks like it is expecting RFC2965 parsing:

Cookie[] cookie = request.getCookies();
    int index = ServletTestUtil.findCookie(cookie, "name1");
    if (index >= 0) {
      String expectedResult = request.getContextPath();
      String result = cookie[index].getPath();
      if (result != null) {
        if (!result.equals(expectedResult)) {
          passed = false;
          pw.println("getPath() returned an incorrect result ");
          pw.println("Expected = " + expectedResult + " ");
          pw.println("Actual = |" + result + "| ");
        } else {
          passed = true;
        }
      } else {
        passed = false;
        pw.println("Error: getPath() returned a null result");
      }
    } else {
      passed = false;
      pw.println("Error: The expected cookie was not received from the client");
    }

@janbartel
Copy link
Contributor Author

Update: even after switching to RFC2965 compliance these tests are still failing. Jetty RFC2965 parsing seems to be failing to parse a cookie from the client as simple as: Cookie: name1=value1. This can be reproduced in a test case.

@janbartel
Copy link
Contributor Author

janbartel commented Jun 7, 2023 •
edited
Loading

Update: using RFC2965_LEGACY compliance jetty is able to correctly parse the simple cookie Cookie: name1=value1.

@gregw if we use RFC2965 some tck cookie tests fail; if we use RFC2965_LEGACY different cookie tests fail. Be good if there was a solution.

gregw added a commit that referenced this issue Jun 7, 2023
@gregw
Fix #9760 EE9 Cookies
ec0059a 
Fix #9760 Only set path and domain if they are not blank
gregw added a commit that referenced this issue Jun 8, 2023
@gregw
Fix #9760 EE9 Cookies
0f8adbc 
Fix #9760 Only set path and domain if they are not blank
Switch on violation rather than type
@janbartel janbartel mentioned this issue Jun 8, 2023
Merged
janbartel added a commit that referenced this issue Jun 8, 2023
@janbartel @gregw
Jetty 12.0.x 9760 fix cookie parsing (#9894)
c13c869 
* Add test to show failure

* Fix #9760 EE9 Cookies

Fix #9760 Only set path and domain if they are not blank

* Fix #9760 EE9 Cookies

Fix #9760 Only set path and domain if they are not blank
Switch on violation rather than type

* Handle legacy cookie version and comment

* Handle cookie version and comment

---------

Co-authored-by: gregw <[email protected]>
@janbartel janbartel mentioned this issue Jun 12, 2023
Merged
@janbartel
Copy link
Contributor Author

With #9902 committed, the last tck failures that are not related to cross context have all been fixed.

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.0.0.beta2 Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@janbartel janbartel

Labels
Bug For general bugs on Jetty side TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@janbartel