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.

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 Challenge: UIComponent.bindings field should not fail Faces signature test #1714

Closed
brideck opened this issue Nov 8, 2022 · 10 comments
Closed
Labels
challenge TCK challenge invalid This doesn't seem right

Comments

@brideck
Copy link
Contributor

brideck commented Nov 8, 2022

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 #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.

@pnicolucci pnicolucci added the challenge TCK challenge label Nov 8, 2022
@scottmarlow
Copy link

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

@pnicolucci
Copy link
Contributor

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

@scottmarlow
Copy link

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
Copy link

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?

@pnicolucci
Copy link
Contributor

pnicolucci commented Nov 9, 2022

@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.

@edburns
Copy link

edburns commented Nov 15, 2022

@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.

@pnicolucci
Copy link
Contributor

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

@pnicolucci
Copy link
Contributor

I see @arjantijms opened the following issue so we don't miss this removal in the next release: #1725 Thanks!

@arjantijms arjantijms added the appealed-challenge TCK challenge was appealed label Nov 26, 2022
@scottmarlow
Copy link

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.

@arjantijms arjantijms added invalid This doesn't seem right and removed appealed-challenge TCK challenge was appealed labels Nov 28, 2022
@arjantijms
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
challenge TCK challenge invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

5 participants