faces icon indicating copy to clipboard operation
faces copied to clipboard

TCK Challenge: UIComponent.bindings field should not fail Faces signature test

Open brideck opened this issue 2 years ago • 8 comments

Challenged Tests: ee.jakarta.tck.faces.signaturetest.JSFSigTestIT#signatureTest com.sun.ts.tests.signaturetest.javaee.JaveEESigTest#signatureTest_from_jsp com.sun.ts.tests.signaturetest.javaee.JaveEESigTest#signatureTest_from_servlet

TCK Version: Jakarta Faces 4.0.x

Tested Implementation: Open Liberty -- containing MyFaces 4.0

Description: When running the signature tests against MyFaces 4.0, the following failure is seen:

[javatest.batch] Missing Fields
[javatest.batch] --------------
[javatest.batch] 
[javatest.batch] jakarta.faces.component.UIComponent:    field protected java.util.Map<java.lang.String,jakarta.el.ValueExpression> jakarta.faces.component.UIComponent.bindings
[javatest.batch]  anno 0 java.lang.Deprecated(boolean forRemoval=false, java.lang.String since="")

For Faces 4.0, work was done under https://github.com/jakartaee/faces/issues/1548 to remove deprecated expression language references from the API. Consequently, the deprecated methods UIComponent.get/setValueBinding were removed. However, it looks as though there was an oversight in Mojarra, where the deprecated UIComponent.bindings field was not removed, whereas it was in MyFaces.

MyFaces does not think it would be appropriate to have to add this unused field back to the API, if there is no use case for it being there. An experimental Mojarra commit has been tested to see what impact removing the field would have there, and no problems were observed, although the full TCK, etc. has not been tested yet.

brideck avatar Nov 08 '22 20:11 brideck

FYI, WildFly is passing Faces signature tests with both Platform TCK Faces signature (which seems to have been updated after Faces Specification TCK signature file was added) and with Faces TCK. WildFly output from running Faces TCK signature tests is at https://gist.github.com/scottmarlow/08f5089015b159bc3056a06da74f35b3

scottmarlow avatar Nov 08 '22 21:11 scottmarlow

In my opinion, this looks like an oversight and this challenge should be accepted but I want to hear what @arjantijms and @BalusC have to say. There is precedence for accepting a challenge to the signature tests, for instance: https://github.com/jakartaee/faces/issues/1474

pnicolucci avatar Nov 09 '22 14:11 pnicolucci

https://jakarta.ee/specifications/faces/4.0/apidocs/jakarta/faces/component/uicomponent would be expected to reflect what the Mojarra project uses since https://github.com/jakartaee/faces depends on Mojarra to provide the Faces 4.0 SPEC API.

It would be better if the Faces SPEC API project was able to maintain the SPEC API directly which sounds like a good topic to discuss on the Faces mailing list.

scottmarlow avatar Nov 09 '22 16:11 scottmarlow

In my opinion, this looks like an oversight and this challenge should be accepted but I want to hear what @arjantijms and @BalusC have to say. There is precedence for accepting a challenge to the signature tests, for instance: #1474

Is there a reason why the MyFaces SPEC API wouldn't be updated to have the missing bindings field?

scottmarlow avatar Nov 09 '22 16:11 scottmarlow

@scottmarlow the argument here is that the bindings field isn't necessary and should have been removed for Faces 4.0. This to me seemed like an oversight. Adding the bindings field back into the MyFaces Spec API just to satisfy signature tests doesn't seem reasonable if there is no use for it within the implementation and the sole purpose would be to satisfy the signature test.

pnicolucci avatar Nov 09 '22 17:11 pnicolucci

@pnicolucci wrote:

Adding the bindings field back into the MyFaces Spec API just to satisfy signature tests doesn't seem reasonable if there is no use for it within the implementation and the sole purpose would be to satisfy the signature test.

I see where you're coming from. In forming your response, I request you keep the following in mind. The process for handling issues such as this is clearly defined with regard to backward compatibility policy of Jakarta EE. We discussed this at the 2022-11-15 Jakarta EE Platform Architecture call. I was reminded that we can't take such a field out in a non-major release of Faces. Therefore, we must ask MyFaces to put bindings back until Faces 5.0. We agreed it was acceptable to put the field back as a no-op.

edburns avatar Nov 15 '22 17:11 edburns

@edburns thanks, I've opened the following MYFACES JIRA: https://issues.apache.org/jira/browse/MYFACES-4502

pnicolucci avatar Nov 15 '22 17:11 pnicolucci

I see @arjantijms opened the following issue so we don't miss this removal in the next release: https://github.com/jakartaee/faces/issues/1725 Thanks!

pnicolucci avatar Nov 15 '22 17:11 pnicolucci

Should the appealed-challenge be removed and instead invalid be used here?

From the Rejected Challenges and Remedy section of https://jakarta.ee/committees/specification/tckprocess:

When a challenge issue is rejected, it MUST be closed with a label of invalid to indicate it has been rejected.

I only pasted the first sentence from the referenced TCK Process section. There is additional guidance on appealing a challenge that can lead to appealed-challenge being used but I don't think that path has been followed.

scottmarlow avatar Nov 28 '22 14:11 scottmarlow

@scottmarlow thanks, I updated the label to be invalid instead.

arjantijms avatar Nov 28 '22 15:11 arjantijms