faces icon indicating copy to clipboard operation
faces copied to clipboard

[localized-composite] Fixes mojarra #5160 and MYFACES-4491

Open cristof opened this issue 2 years ago • 2 comments

Adds tests to check issues mojarra #5160 and MYFACES-4491. Currently, fails with non patched mojarra and myfaces snapshot versions.

cristof avatar Nov 15 '22 14:11 cristof

Note that new tests are in principal for Faces.Next, as the TCK process disallows to add tests for the existing TCK. As we probably do want to use these to test 4.0 implementations, we may want to create something like an optional group for them which has to be excluded for 4.0 certification purposes.

arjantijms avatar Nov 17 '22 18:11 arjantijms

I've checked the Jakarta EE TCK Process 1.3 and my interpretation of it is that adding a given test is an enhancement of TCK, allowing to release a point version of it (provided it is mentioned in faces spec releases). A point version of TCK would not impact previous TCK certification. So, if mojarra or myfaces are considered 2.3.1 jakarta8 TCK compliant, it would not change if a new 2.3.2 TCK release is made. In that case, is it necessary to add the optional group?

cristof avatar Nov 20 '22 12:11 cristof

@cristof Given all the process around simply updating a TCK test for some internal detail (not changing anything about the test external behaviour), where we have to ask for exceptions and the full Jakarta Specification committee has to organise a vote for such trivial change, I don't think adding a whole new test would even be considered, let alone allowed ;)

arjantijms avatar Nov 23 '22 20:11 arjantijms

I see. In that case, any new PR with tests like this should pass a vote. How may I proceed with this one, where a given feature is expected by spec, but is not checked by TCK? Move the proposed test to each implementation instead of BalusC suggestion? If the road to take is the "optional group" approach, should I add a junit4 category to my PR and reflect that in a optional maven profile? This approach would somehow fragment the TCK - we'd have an "official" TCK and a superset of it, with specification-wise greater reach.

cristof avatar Nov 23 '22 21:11 cristof

Yes. I "created" it on purpose just to test if the implementation would be able to deal with variants.

cristof avatar Apr 04 '23 19:04 cristof

I have reorganized the project a bit and added a 4.1 branch so we can finally start working on TCKs for 4.1.

As it has been a while ago, can at least one reviewer review+approve this?

BalusC avatar Jul 16 '23 19:07 BalusC

With 4.1 underway I've finally taken the time to review/run this test. The test itself looks good (albeit I wish it tested more locales), but the run fails in GlassFish, WildFly and Tomcat for the very simple reason that request.getLocale() didn't return a Locale instance containing the PB subtag/variant. It only returns pt-BR even when the Accept-Language header specifies pt-BR-PB. This a problem which in turn probably needs to be addressed in the Servlet spec.

When I locally edit Mojarra's MultiViewHandler to directly consider Accept-Language header instead of request.getLocale() then the test passes on all servers, so it at least confirms the cause of problem is not anymore in the Mojarra side.

BalusC avatar Feb 17 '24 12:02 BalusC

I adjusted the test via https://github.com/jakartaee/faces/pull/1891

The key change is thus directly parsing Accept-Language header in the backing bean instead of using request.getLocale() (not production quality code, it doesn't at all consider weighting/ranking and all ;) it's just perfectly fine in the isolated context of the IT): https://github.com/jakartaee/faces/pull/1891/files#diff-a1f3b18f81f9b310740b068e74d4ca61dd8458381875814ad252b9355db10c10R38

I also added more corner cases to the test: https://github.com/jakartaee/faces/pull/1891/files#diff-983df73b268c7517c651bdbdaf83bd32d26e2063e71a98e59ce09ecc8d3ece47R70

Thank you for the initial contribution!

BalusC avatar Feb 17 '24 13:02 BalusC