jakartaee / faces

Jakarta Faces
Other
103 stars 55 forks source link

For Faces 4.0 TCK Challenge issue [#1935] to use Date formats that are compatible with Java 21 #1939

Closed scottmarlow closed 1 month ago

scottmarlow commented 3 months ago

For Faces 4.0 TCK Challenge issue #1935 to use Date formats that are compatible with Java 21.

This change is similar to https://github.com/jakartaee/faces/commit/c971c205b5d038ed85c1d6b6ebca8ff13ef83a1b but instead changed from sept to september.

scottmarlow commented 3 months ago

I couldn't build this locally as I get failure with command mvn -X clean install -Dmojarra.version=4.0.5:

There is a process already using the admin port 4,848 -- it probably is another instance of a GlassFish server. [ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.838 s <<< FAILURE! -- in ee.jakarta.tck.faces.test.servlet30.ajax.Issue3473IT [ERROR] ee.jakarta.tck.faces.test.servlet30.ajax.Issue3473IT -- Time elapsed: 0.833 s <<< ERROR! org.jboss.arquillian.container.spi.client.container.DeploymentException: Cannot start GlassFish at org.omnifaces.arquillian.container.glassfish.managed.GlassFishManagedDeployableContainer.deploy(GlassFishManagedDeployableContainer.java:134)

I tried running netstat but port 4848 seems available:

netstat -nao |grep 4848

scottmarlow commented 2 months ago

Does anyone know which version of Java changed the date formatting rules that created the need to specify September instead of Sept?

scottmarlow commented 2 months ago

Copying conversation from https://eclipsefoundationhq.slack.com/archives/C0131MLD538/p1720797042872349?thread_ts=1720446904.437519&cid=C0131MLD538

Scott Marlow Perhaps we should accept the https://github.com/jakartaee/faces/issues/1935 challenge and ask the Faces lead to approve the user workaround of ignoring the challenged (4.0.x) tests on Java 21.

Scott Marlow @Arjan Tijms regarding my last point about accepting the challenge and having a user workaround, from https://jakarta.ee/committees/specification/tckprocess/: Accepted Challenges ... The specification project may approve (user) workarounds for an accepted TCK challenge (as an alternative to excluding TCK tests). ...

Scott Marlow Then we could fix the 4.0.x branch to be buildable when possible instead of right now.

Arjan Tijms We can ignore them for sure. Patching in @Bauke Scholtz for final confirmation

cc @BalusC

BalusC commented 2 months ago

I'm confused why we don't just backport https://github.com/jakartaee/faces/commit/c971c205b5d038ed85c1d6b6ebca8ff13ef83a1b? The current change basically breaks backwards compatibility with Java SE impls/versions where this CLDR change is not introduced. And when we are going to upmerge 4.x into master the existing change will be overwritten.

scottmarlow commented 2 months ago

@BalusC I couldn't test this pull request because I cannot build it. I also cannot build the 4.0.x branch as well so I cannot really do much to help backport https://github.com/jakartaee/faces/commit/c971c205b5d038ed85c1d6b6ebca8ff13ef83a1b. I'm personally fine with any change the Faces team can make to improve the test (for running on Java 21 as well as Java 11/17) or excluding the test also would help.

The https://github.com/jakartaee/faces/issues/1935 challenge is three weeks old now and I would like to certify WildFly on Java 21 as being Jakarta Faces 4.0 compatible (I'm looking to do this today if possible).

From the TCK Process doc (Accepted Challenges):

The specification project may approve (user) workarounds for an accepted TCK challenge (as an alternative to excluding TCK tests).

From the TCK Process doc (Active Resolution)

The failure to resolve a Challenge might prevent an implementation from going to market; Challenges SHOULD be given a high priority by the specification project and resolved in a timely manner. Two weeks or less SHOULD be considered the ideal period of time to resolve a challenge. Challenges may go longer as needed, but as a rule SHOULD avoid months.

If consensus cannot be reached by the specification project for a prolonged period of time, the default recommendation is to exclude the tests and address the dispute in a future revision of the specification.

If we cannot update the test or exclude it in a reasonable amount of time, I think the user workaround of ignoring the challenged test failures on Java 21+ would help as a short term workaround until someone can get the Faces 4.0.x branch to build.

BalusC commented 2 months ago

@arjantijms can you please confirm if we're allowed to backport https://github.com/jakartaee/faces/commit/c971c205b5d038ed85c1d6b6ebca8ff13ef83a1b into 4.0 branch? I'm not sure what the rules are wrt doing changes in a live TCK branch. Shouldn't this be a 'challenge' or what?

The current PR needs to be rejected because the TCK won't anymore pass when using a Java SE impl/version where this CLDR change is not introduced. The minimum Java SE requirement for TCK 4.0 is still Java SE 11. So it must be still compatible with Java SE 11. The change done in https://github.com/jakartaee/faces/commit/c971c205b5d038ed85c1d6b6ebca8ff13ef83a1b (using a specific month whose full and short names are exactly the same, the month of May) ensures compatibility with previous Java SE impls/versions.

jasondlee commented 2 months ago

The change done in c971c20 (using a specific month whose full and short names are exactly the same, the month of May) ensures compatibility with previous Java SE impls/versions.

Due to the nomadic history of Mojarra, some context has been lost on some of these tests. This test in particular seems really fragile. Why are we testing a specific date format? What part of the spec are we ensuring compatibility with, and can this not be handled in a more resilient fashion? Does anyone remember the history/genesis of this particular test?

BalusC commented 2 months ago

The change done in c971c20 (using a specific month whose full and short names are exactly the same, the month of May) ensures compatibility with previous Java SE impls/versions.

Due to the nomadic history of Mojarra, some context has been lost on some of these tests. This test in particular seems really fragile. Why are we testing a specific date format? What part of the spec are we ensuring compatibility with, and can this not be handled in a more resilient fashion? Does anyone remember the history/genesis of this particular test?

Agreed. See also https://github.com/eclipse-ee4j/mojarra/issues/5399 and specifically my 2nd comment. It should have tested a fixed pattern instead of a "format style".

BalusC commented 2 months ago

As per https://github.com/jakartaee/faces/issues/1935 apparently "the spec committee" needs to "grant us an exception to change the 4.0 tests". Once granted then we can backport https://github.com/jakartaee/faces/commit/c971c205b5d038ed85c1d6b6ebca8ff13ef83a1b

So the next step is to ask "the spec committee". I have however no clue how/where. @arjantijms should be able to do this.

scottmarlow commented 2 months ago

I pushed a small change to use may instead of september.

As per https://github.com/jakartaee/faces/issues/1935 apparently "the spec committee" needs to "grant us an exception to change the 4.0 tests". Once granted then we can backport https://github.com/jakartaee/faces/commit/c971c205b5d038ed85c1d6b6ebca8ff13ef83a1b

So the next step is to ask "the spec committee". I have however no clue how/where. @arjantijms should be able to do this.

The TCK Process was updated so that the Faces Specification team can make the decision:

Challenge Resolution The specification project may approve (user) workarounds for an accepted TCK challenge (as an alternative to excluding TCK tests).

As another alternative to excluding a challenged test, it may be possible to adjust the test validation logic to “expand” the validation check. E.g. if a test expects a value “A1” for a specific variable “x”, but a challenge is raised arguing that the specification language actually allows for either values “A1” and “A2” (but no other values) to be valid values for “x”, then it could be a valid course of action to modify the challenged test to allow either “A1” OR “A2” for “x”.

Since this line of thinking might be applied to cases that aren’t quite as straightforward as this example, care should be taken when using this approach. A particular danger is that an implementation that has already demonstrated compliance before the challenge was raised might actually not pass the new, modified test.

To limit the confusion and additional work such a scenario would cause, if there is already at least one certified compatible implementation before the challenge, the new, modified TCK should be run against at least one such implementation (and ideally all of them) before the changes are published, released, and finalized.

If such a change were released, and it was later found to cause a previously-certified implementation to fail the new, modified test, then excluding the test would likely be the only option, and this would require yet another, additional service release.

arjantijms commented 2 months ago

In the meantime we have simply allowed to exclude these tests for EE 10/Faces 4.0.

Shall we close this PR?

BalusC commented 1 month ago

ok let's close off for now