jakartaee / faces

Jakarta Faces
Other
109 stars 55 forks source link

Remove JspFactory from TCK following challenge 1718 #1738

Closed arjantijms closed 2 years ago

arjantijms commented 2 years ago

Fixes #1718

Signed-off-by: Arjan Tijms arjan.tijms@gmail.com

Emily-Jiang commented 2 years ago

@arjantijms it seems that @brideck had some comments on this. Can you take a look at them to see how best to address them?

arjantijms commented 2 years ago

@Emily-Jiang yes, they are to be addressed indeed.

brideck commented 2 years ago

We're also some issues with the changes to the el.elresolvers test. It's failing for us with these changes in a rather odd manner -- we haven't gotten to the bottom of why yet.

Were those changes tested with Mojarra+Glassfish?

arjantijms commented 2 years ago

Were those changes tested with Mojarra+Glassfish?

Yes, everything was tested on Mojarra+Glassfish for every change. But we may want to go for a different approach here, and use the same unwrapping that was done for one of the other TCK challenges.

volosied commented 2 years ago

Hi @arjantijms,

Just to follow up to Brian's comment about el.elresolversabove - we have identified the cause of the failure.

Long story short -- can the test create a session in the setup?

Here's more background about the error: Open Liberty encountered an NPE with this stack:

Exception thrown by application class 'com.sun.ts.tests.jsf.common.servlets.HttpTCKServlet.invokeTest:165'
jakarta.servlet.ServletException: java.lang.NullPointerException
at com.sun.ts.tests.jsf.common.servlets.HttpTCKServlet.invokeTest(HttpTCKServlet.java:16

The occurred on the "session" implicit info. Liberty doesn't have a session at this point in the test, and, when retrieving a session via SessionImplicitObject, MyFaces doesn't create one: externalContext(context).getSession(false);

With the prior code, the test did create a session with JspFactory.getDefaultFactory().getPageContext(this, request, response, null, true, 1024, true); method call since needsSession was set true in getPageContext.

arjantijms commented 2 years ago

@volosied

Where does the NullPointerException exactly occur?

With the prior code and change you are referring to this part right?

https://github.com/jakartaee/faces/pull/1738/files#diff-acc5f58a95628b31133fe5f9c7f87f5c198404d3aeaedd752df6561540a0d391R1540

volosied commented 2 years ago

I'm sorry about my mistake -- I completely left out the caused by portion of the stack trace. And yes, I am referring to the change you linked above.

Caused by: java.lang.NullPointerException: 
at com.sun.ts.tests.jsf.spec.el.elresolvers.TestServlet.validateImplicitObjectValue(TestServlet.java:1566)
at com.sun.ts.tests.jsf.spec.el.elresolvers.TestServlet.facesImplicitObjectResolverGetValueTest(TestServlet.java:615)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

https://github.com/jakartaee/faces/blob/master/tck/old-tck/source/src/com/sun/ts/tests/jsf/spec/el/elresolvers/TestServlet.java#L1566

The result variable is null since there's no session for us.

brideck commented 1 year ago

Bumping this conversation with @arjantijms so we can get thoughts on a follow-on PR to establish a session.

volosied commented 1 year ago

Perhaps this could be used: https://github.com/jakartaee/faces/compare/master...volosied:create-session-1738?expand=1