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

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

Conversation

scottmarlow
Copy link

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

This change is similar to c971c20 but instead changed from sept to september.

@scottmarlow
Copy link
Author

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

Copy link
Contributor

@jasondlee jasondlee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me. Will wait to merge for @arjantijms' take.

@scottmarlow
Copy link
Author

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

@scottmarlow
Copy link
Author

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

Scott Marlow
Perhaps we should accept the #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
Copy link
Member

BalusC commented Jul 22, 2024

I'm confused why we don't just backport c971c20? 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
Copy link
Author

@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 c971c20. 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 #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
Copy link
Member

BalusC commented Jul 22, 2024

@arjantijms can you please confirm if we're allowed to backport c971c20 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 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.

@jasondlee
Copy link
Contributor

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

BalusC commented Jul 22, 2024

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 eclipse-ee4j/mojarra#5399 and specifically my 2nd comment. It should have tested a fixed pattern instead of a "format style".

@BalusC
Copy link
Member

BalusC commented Jul 22, 2024

As per #1935 apparently "the spec committee" needs to "grant us an exception to change the 4.0 tests". Once granted then we can backport c971c20

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 scottmarlow force-pushed the 4.0.x_backport_jdk17_21_date_format_two branch from e8fa0b1 to 70a1917 Compare July 22, 2024 14:10
@scottmarlow
Copy link
Author

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

As per #1935 apparently "the spec committee" needs to "grant us an exception to change the 4.0 tests". Once granted then we can backport c971c20

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

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

Shall we close this PR?

@BalusC
Copy link
Member

BalusC commented Sep 5, 2024

ok let's close off for now

@BalusC BalusC closed this Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants