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

Update jstl dates for java21 #1312

Open
wants to merge 42 commits into
base: main
Choose a base branch
from
Open

Conversation

pnicolucci
Copy link
Contributor

@pnicolucci pnicolucci commented May 28, 2024

Fixes Issue
This is for Jakarta Tags Challenge: jakartaee/tags#255

Related Issue(s)
jakartaee/tags#255

Describe the change
I'm working through the failing sub buckets to update golden files for Java pre and post Java 19. This is a work in progress as I have a couple more sub buckets to work through but I wanted to get a PR up to show progress.

I've decided to leave the original JSPs alone and just create a new set of JSPs and golden files where necessary. This is done where ever we have AM/PM in the actual JSP. If there is no AM/PM in the JSP and we just need to work with the new space character I've just modified the existing JSP to UTF-8 encoding and added a new golden file.

There are more clever ways to do this but I wanted to limit the changes to the existing code base with the intention of this change running on both EE10 and EE11 and being conscious of the fact that these changes are to address a challenge. We can make more clever changes in the future if we have to address a similar problem.

A few additional comments:

  1. positivePDDateStyleTest / positivePDTimeStyleTest / positivePDParseLocaleTest all have an existing goldenfile but the golden file is not used within the JSTLClient for that test so I didn't touch these goldenfiles.
  2. positiveFDTimeZoneNullEmptyTest / positivePDTimeZoneNullEmptyTest were both spelled incorrectly so I fixed the spelling.
  3. If I removed the STANDARD or STANDARD_COMPAT property so that I could override the goldenfiles then I added the TEST_NAME property.
  4. For the new JSPs that were created they are just copies of the original JSP with updates to spacing and formatting.
  5. There are a couple tests that required additional formatting changes from the original in addition to just the space between AM and PM, mainly full date formats in some instance us a , rather than at such as: Friday, December 26, 1997 at 7:11:34 PM Pacific Standard Time in the original and Friday, December 26, 1997, 7:11:34 PM Pacific Standard Time in the updated golden file. These golden files include:
    onedotzero\positiveSetTimezoneValueTestJava20Plus.gf
    onedotzero\positiveTimezoneValueTestJava20Plus.gf
    timezone\positiveTimezoneValueNullEmptyTestJava20Plus.gf
    timezone\positiveTimezoneValueTestJava20Plus.gf
    settimezone\positiveTimezoneValueNullEmptyTestJava20Plus.gf
    settimezone\positiveTimezoneValueTestJava20Plus.gf

Additional context
See the Jakarta Tags Challenge.

CC @alwin-joseph @anajosep @arjantijms @cesarhernandezgt @dblevins @m0mus @edbratt @gurunrao @jansupol @jgallimore @kazumura @kwsutter @LanceAndersen @bhatpmk @RohitKumarJain @shighbar @gthoman @brideck @OndroMih @dmatej
@starksm64 @scottmarlow

scottmarlow and others added 30 commits September 29, 2022 10:55
Signed-off-by: Scott Marlow <[email protected]>
…so change WildFly Preview to WildFly

Signed-off-by: Scott Marlow <[email protected]>
…e_submodule

Replace coreprofile submodule
…STful Web Services 3.1 a default ExceptionMapper is required. The suggestion of the specification is to return 500 as the response status.

Signed-off-by: James R. Perkins <[email protected]>
[1135] Do not add the CustomJsonbResolver as a service provider for a…
[1134] Allow the response type to be either 400 or 500. In Jakarta RE…
Create a core profile tck service release
Signed-off-by: Scott M Stark <[email protected]>
…enge

Accept unique constraint as part of the create table statement
Signed-off-by: David Matějček <[email protected]>
- previous version
  - used root to run builds
  - had no permissions to write to the home dir
  - had no permissions to kill processes
- this version
  - prints useful information about user, network, env
  - uses user tck based on Jenkins user (see Elipses' documentation)
  - added AS_TRACE parameter, useful when GF asadmin behaves badly
  - with fixed GlassFish (PR24200) doesn't need to kill (but for sure...)

Signed-off-by: David Matějček <[email protected]>
…hallenge

JPA#391 Fix wrongly typed comparison expressions
Signed-off-by: Jorge Bescos Gascon <[email protected]>
ClassLoader issue in embedded Arquillian
Also updated associated dependencies and plugins in pom.

Signed-off-by: Arjan Tijms <[email protected]>
Signed-off-by: Jorge Bescos Gascon <[email protected]>
 ClassLoader issue in embedded Arquillian (additional null check)
…ee#1173)

Bumps [testng](https://github.com/cbeust/testng) from 7.4.0 to 7.5.1.
- [Release notes](https://github.com/cbeust/testng/releases)
- [Changelog](https://github.com/testng-team/testng/blob/master/CHANGES.txt)
- [Commits](testng-team/testng@7.4.0...7.5.1)

---
updated-dependencies:
- dependency-name: org.testng:testng
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@scottmarlow
Copy link
Contributor

scottmarlow commented May 31, 2024

I do not see any problems other than the few source files that I commented on today regarding the copyright in newly added files.

@pnicolucci
Copy link
Contributor Author

@scottmarlow the new JSP files are exact copies of the existing JSPs with minor updates, UTF-8 encoding, and an update of the space character for Java 20 Plus. I kept the original copyright since they were copies. I wanted to clarify why I left the original copyright, I didn't feel comfortable removing it since I was copying existing code.

Is the consensus of the platform-tck project that I should remove the original copyright from the new JSPs even if they are copies of existing code?

@pnicolucci pnicolucci marked this pull request as ready for review May 31, 2024 18:16
@scottmarlow
Copy link
Contributor

@scottmarlow the new JSP files are exact copies of the existing JSPs with minor updates, UTF-8 encoding, and an update of the space character for Java 20 Plus. I kept the original copyright since they were copies. I wanted to clarify why I left the original copyright, I didn't feel comfortable removing it since I was copying existing code.

Good point, I knew you were copying existing code. I now think either way would of been okay. I'm going to resolve the my comments.

Is the consensus of the platform-tck project that I should remove the original copyright from the new JSPs even if they are copies of existing code?

@scottmarlow
Copy link
Contributor

Looks good to me, lets give it a few more days before merging in case @alwin-joseph or @gurunrao have feedback.

@pnicolucci
Copy link
Contributor Author

@scottmarlow thanks very much for all of your help and testing!! Very much appreciated!

@pnicolucci
Copy link
Contributor Author

@scottmarlow This PR is against the master branch. Do I need any other PR for 10.0.x or tckrefactor branches?

@alwin-joseph
Copy link
Contributor

alwin-joseph commented Jun 3, 2024

Context on Tags tests for EE11:

Tags TCK Fix :

  • IMO the current changes in this PR belongs to 10.0.x branch primarily and the PR should be re-targeted for same.
  • For tckrefactor branch(EE11) a separate PR will need to be created based on the test failures and fixes required. Currently there were 15 test failures in tckrefactor branch with Glassfish8+JDK21 related to Tags 3.0 TCK Challenge against tests that include space character before AM/PM tags#255 due to space character before AM/PM. Other implementations would need to execute this refactored tck to find the results.
  • IMO if we are to merge tckrefactor with master soon, it would be appropriate to have the fixes in tckrefactor branch and test first.

cc @gurunrao

@pnicolucci
Copy link
Contributor Author

@scottmarlow @alwin-joseph

  1. You don't want these changes in the master branch.
  2. Create this same PR to the 10.0.x branch.
  3. You also want another PR to the tckrefactor branch, I'll need some help here as I'm not familiar with the tests and how they've been rewritten in that branch. Can someone else take a shot at making these same changes to the tckrefactor branch? Are the failures between the tckrefactor branch and the 10.0.x branch the same when running with Java 21?

I'm confused about what branch will eventually be used to test Jakarta Tags in EE11. I thought the intent was to update the 3.0.0 tests so that we can use the same TCK for both EE10 and EE11.

@alwin-joseph
Copy link
Contributor

3. You also want another PR to the `tckrefactor` branch, I'll need some help here as I'm not familiar with the tests and how they've been rewritten in that branch. Can someone else take a shot at making these same changes to the `tckrefactor` branch? Are the failures between the `tckrefactor` branch and the `10.0.x` branch the same when running with Java 21?

I can take up this, will create a PR in tckrefactor branch so the same will be included in EE11 Platform TCK. It would certainly help if other implementations can validate the test runs after the fix is applied.

I'm confused about what branch will eventually be used to test Jakarta Tags in EE11. I thought the intent was to update the 3.0.0 tests so that we can use the same TCK for both EE10 and EE11.

The refactored Tags tests in tckrefactor is intended to be included in EE11 Platform TCK and not standalone TCK.
For standalone Tags TCK EE11, the 10.0.x branch will be used, same for EE10 and EE11.
cc @scottmarlow @gurunrao Please add/correct me if I missed anything.

@scottmarlow
Copy link
Contributor

@scottmarlow @alwin-joseph

1. You don't want these changes in the `master` branch.

No need to merge to the master branch. Sorry that I missed this before.

2. Create this same PR to the 10.0.x branch.

Please do. I validated your change as if it was a 10.0.x change.

3. You also want another PR to the `tckrefactor` branch, I'll need some help here as I'm not familiar with the tests and how they've been rewritten in that branch. 

Clearly we will need to figure out which Tags tests will be run against Jakarta EE 11 implementations (e.g. Tags 3.0 standalone TCK or EE 11 Platform TCK).

Can someone else take a shot at making these same changes to the tckrefactor branch? Are the failures between the tckrefactor branch and the 10.0.x branch the same when running with Java 21?

Eventually (after EE 11) we will need to make changes to be able to pass the Tags tests against a future EE release but we shouldn't run refactored Tags TCK tests against EE 11 implementations as that wouldn't be valid.

I'm confused about what branch will eventually be used to test Jakarta Tags in EE11. I thought the intent was to update the 3.0.0 tests so that we can use the same TCK for both EE10 and EE11.

Yes, but I think we previously missed the impact on EE 11 implementation testing.

@scottmarlow
Copy link
Contributor

We should discuss this further...

@scottmarlow
Copy link
Contributor

scottmarlow commented Jun 4, 2024

Updated: I now think that it is fine for the EE 11 Platform TCK to run JSTL tests (based on Tags 3.0) against EE 11 implementations. So in summary, I am +1 for merging this pr to the 10.0.x branch. More inline below:

Context on Tags tests for EE11:

* Tags tests were refactored for EE11 Platform TCK in `tckrefactor` branch (https://github.com/jakartaee/platform-tck/tree/tckrefactor/jstl) to use Arquillian and Junit.

* The tests can be run with Glassfish 8(JDK17 & JDK21) using https://github.com/jakartaee/platform-tck/tree/tckrefactor/glassfish-runner/jstl-tck.

Tags TCK Fix :

* IMO the current changes in this PR belongs to `10.0.x` branch primarily and the PR should be re-targeted for same.

+1

* For `tckrefactor` branch(EE11) a separate PR will need to be created based on the test failures and fixes required. Currently there were 15 test failures in `tckrefactor` branch with Glassfish8+JDK21 related to [Tags 3.0 TCK Challenge against tests that include space character before AM/PM tags#255](https://github.com/jakartaee/tags/issues/255) due to space character before AM/PM. Other implementations would need to execute this refactored tck to find the results.

+1

* IMO if we are to merge `tckrefactor` with `master` soon, it would be appropriate to have the fixes in `tckrefactor` branch and test first.

It would be fine to merge the changes into both 10.0.x + master or just 10.0.x. Personally, I would probably just update this pr to target 10.0.x which should be an easy change since master is almost the same as 10.0.x. The master branch is probably causing confusion from being around still.

I also want to mention that some unrelated changes in the master branch will need to be merged to the tckrefactor before we complete the EE 11 release. More specifically, I am thinking of the #1220 changes that moved tests from the CDI TCK to the Platform TCK (via cdi-ee-tck folder).

@pnicolucci
Copy link
Contributor Author

10.0.x PR: #1320

@pnicolucci pnicolucci changed the title Update Jakarta Tags TCK for Java 21 Update jstl dates for java21 Jun 4, 2024
Copy link
Contributor

@gurunrao gurunrao left a comment

Choose a reason for hiding this comment

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

Branch should be 10.0.X, changes LGTM.

@pnicolucci
Copy link
Contributor Author

3. You also want another PR to the `tckrefactor` branch, I'll need some help here as I'm not familiar with the tests and how they've been rewritten in that branch. Can someone else take a shot at making these same changes to the `tckrefactor` branch? Are the failures between the `tckrefactor` branch and the `10.0.x` branch the same when running with Java 21?

I can take up this, will create a PR in tckrefactor branch so the same will be included in EE11 Platform TCK. It would certainly help if other implementations can validate the test runs after the fix is applied.

I'm confused about what branch will eventually be used to test Jakarta Tags in EE11. I thought the intent was to update the 3.0.0 tests so that we can use the same TCK for both EE10 and EE11.

The refactored Tags tests in tckrefactor is intended to be included in EE11 Platform TCK and not standalone TCK. For standalone Tags TCK EE11, the 10.0.x branch will be used, same for EE10 and EE11. cc @scottmarlow @gurunrao Please add/correct me if I missed anything.

@alwin-joseph did you have a chance to get a PR for the tckrefactor branch?

@alwin-joseph
Copy link
Contributor

@alwin-joseph did you have a chance to get a PR for the tckrefactor branch?

Not yet. I will get to that soon.

@scottmarlow scottmarlow changed the base branch from master to tckrefactor July 18, 2024 12:55
@scottmarlow
Copy link
Contributor

Updated to target the tckrefactor branch that will soon be renamed to main and the current master branch will be renamed as well to a different name to be determined.

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.

10 participants