Closed arjantijms closed 8 months ago
CC @alwin-joseph @gurunrao
I can help, the question is how to partition the work.
@starksm64 easiest I think by just taking ownership of one of the items. If you can mention the item here, e.g. 1, then everyone knows you'll be looking at that one.
Ok.
@arjantijms for 1, do you want that test2 module added as a new tck module?
@starksm64 Indeed, the regular /tck folder. This TCK btw does not have the kind of split that Jakarta REST has. We still need to see what the best way is for vendors to add their own connector.
Ok, I'll take item 1. and create a PR for it.
Should the tests be passing? I have copied test over and made the group name consistent, but I see the first test that are run failing. I do see the same com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT fail in the mojarra/test2 main brach as well:
[INFO] -------------------------------------------------------
[INFO] Running com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT
Starting container using command: [/Library/Java/JavaVirtualMachines/jdk-11.0.13.jdk/Contents/Home/bin/java, -jar, /Users/starksm/Dev/JBoss/Jakarta/rh-faces/target/glassfish7/glassfish/modules/admin-cli.jar, start-domain, -t]
Successfully started the domain : domain1
domain Location: /Users/starksm/Dev/JBoss/Jakarta/rh-faces/target/glassfish7/glassfish/domains/domain1
Log File: /Users/starksm/Dev/JBoss/Jakarta/rh-faces/target/glassfish7/glassfish/domains/domain1/logs/server.log
Admin Port: 4848
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 12.132 s <<< FAILURE! - in com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT
[ERROR] com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT.testValidatorInjection Time elapsed: 1.074 s <<< FAILURE!
java.lang.AssertionError
at com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT.testValidatorInjection(Issue3014IT.java:70)
Stopping container using command: [/Library/Java/JavaVirtualMachines/jdk-11.0.13.jdk/Contents/Home/bin/java, -jar, /Users/starksm/Dev/JBoss/Jakarta/rh-faces/target/glassfish7/glassfish/modules/admin-cli.jar, stop-domain, -t] [INFO]
I have copied test over and made the group name consistent, but I see the first test that are run failing. I do see the same com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT fail in the mojarra/test2 main brach as well:
That's "correct", as Hibernate Validator still hasn't released a Jakarta EE 10 compatible version, so Bean Validation doesn't work at all in GlassFish 7.
See https://hibernate.atlassian.net/browse/HV-1857 a little nudge to them would sure help I think ;)
Ok, I have added the dependencies that need to be updated to the HV-1857 issue. You will need to push out the Index of org/glassfish:jakarta.el:5.0.0-M1 staging implementation of EL 5.0.0 as well.
Now I see 2, to change the package names. I'll do that in the current #1601 PR. So is the change com.sun.faces. -> ee.jakarta.tck.faces.?
Now I see 2, to change the package names. I'll do that in the current #1601 PR. So is the change com.sun.faces. -> ee.jakarta.tck.faces.?
Indeed, ee.jakarta.tck.faces
seems to be the preferred package.
Ok, item 2 is also in the #1601 PR
Incorporating the existing TCK from https://github.com/eclipse-ee4j/jakartaee-tck and disabling whatever doesn't work anymore (?)
Hi Arjan - I wanted to clarify on this part and the overall goals for faces TCK in JakartaEE 10.
Is it the expectation that a new Faces TCK should be built from this repo for JakartaEE10 release cycle? If so, are all tests(or the working ones) from jakartaee-tck repo(src/com/sun/ts/tests/jsf) need to be migrated to this repo as JUnit/Arquillian tests ?
In old-style tck tests(from jakartaee-tck) , currently most tests does not work due to compilation issues after the removal of deprecated methods in the api, I am working to fix them. I was looking to get help for converting jakarta.faces.bean.ManagedBean to CDI bean here.
If so, are all tests(or the working ones) from jakartaee-tck repo(src/com/sun/ts/tests/jsf) need to be migrated to this repo as JUnit/Arquillian tests ?
This is probable way too much work for this cycle, as it concerns thousands of tests (I don't know how Jakarta REST/JAX-RS did this).
My idea was a little among the following:
WDYT?
Please note that for the old-style TCK the existing *.jsp files won't work anymore either, and those would have to be converted to .xhtml, which means extra work on top of the already big task.
If so, are all tests(or the working ones) from jakartaee-tck repo(src/com/sun/ts/tests/jsf) need to be migrated to this repo as JUnit/Arquillian tests ?
This is probable way too much work for this cycle, as it concerns thousands of tests (I don't know how Jakarta REST/JAX-RS did this).
JAXRS has less than half the number of Faces tests. It required relatively fewer changes than what is required for faces now.
My idea was a little among the following:
1. Fix in the old-style TCK what's possible to fix given the time and resource constraints. 2. Exclude/move from the old-style TCK what can not be fixed in time 3. From the new JUnit/Maven based TCK, include and run the old-style TCK as we've been doing for GlassFish here: https://github.com/eclipse-ee4j/glassfish/blob/master/appserver/tests/tck/faces/pom.xml
WDYT?
IIUC this means we will use the new JUnit/Maven based TCK as the main TCK bundle within which we can invoke the old-style TCK tests. I think the old tests(source & classes) that are run also need to be part of the TCK bundle. Is Glassfish (in the above appserver/tests/tck/faces/pom.xml) expecting the old TCK dependency to be obtained from maven repository ?
Please note that for the old-style TCK the existing *.jsp files won't work anymore either, and those would have to be converted to .xhtml, which means extra work on top of the already big task.
Agree.
Is Glassfish (in the above appserver/tests/tck/faces/pom.xml) expecting the old TCK dependency to be obtained from maven repository ?
In that example, yes, but it's added locally to Maven via an other pom, which is this one:
If they eventually need to be packaged into a single bundle that should be doable.
Hey guys, what's the status of this? Looks like (1) and (2) are complete?
@kito99 Yes, 1 and 2 have been done. I added a checkmark to them. Now it's a matter of starting to add assertion IDs, specifically to https://github.com/jakartaee/faces/tree/master/tck/faces40
4. Incorporating the existing TCK from https://github.com/eclipse-ee4j/jakartaee-tck and disabling whatever doesn't work anymore (?)
This would require below sub tasks to be complete in jakartaee-tck.
a. Fix the build issues in the current source as reported in https://github.com/eclipse-ee4j/jakartaee-tck/issues/807. This is fixed and under review https://github.com/eclipse-ee4j/jakartaee-tck/pull/815 . b. Convert jakarta.faces.bean.ManagedBean to CDI bean c. Convert *.jsp files to .xhtml d. Run the TCK using Glassfish 7 (atleast) & exclude/move the tests that cannot be fixed in time. e. Package this old TCK built from jakartaee-tck into the new Faces TCK from this repo
Copying the (not yet updated for 4.0) Faces TCK user guide from https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/user_guides/jsf. The Platform TCK install guide could also be copied or merged with existing test2/README.md.
Signature testing tooling is needed (e.g. TCK Migration: Move standalone jaxrs tck signature tests from jakartaee-tck eclipse-ee4j/jaxrs-api#1044).
CC @alwin-joseph @gurunrao
I will work on 5 & 6. Help would be appreciated to update the TCK Userguide contents for 4.0.0 after they are moved to this repo.
@arjantijms excuse my ignorance, but what is an assertion ID?
@kito99
The assertions take some literal requirement from either the spec or the javadoc and assign an ID to it. This ID is then used in tests.
They are listed in a file, here the rendered HTML versions:
https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/install/jsf/docs/assertions
There's often a source .xml too, but sometimes those are lost or duplicated. It's a bit messy at times, unfortunately. For source files see here: https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/internal/docs/jsf (@alwin-joseph maybe those need some cleaning up?)
Here you see them used in the actual tests: https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/tests/jsf/api/jakarta_faces/application/application/URLClient.java#L61
(p.s. the assertion files are maybe a reflection of the "mapping file" thinking which was very popular in the late 90s and mid 00s. Maybe the current culture and thinking has more shifted towards tagging them at the location where they are defined, e.g. directly in the spec document and the javadoc)
Wiki page https://github.com/eclipse-ee4j/jakartaee-tck/wiki/Guide-to-adding-Specification-Assertions has the intention of adding a guide for adding assertions but we need to decide if we will include test assertions or not in new TCKs, which is noted in the wiki page.
I spent some time reading through (some of) the linked JCP TCK documents which reference to a tree of other documents, of which comprise W3C, OASIS, Google, others documents.
The Google test approach is to simply use test assertion macros that do not involve assertion IDs. I think that our equivalent of that could be for each TCK test to identify the SPEC section or API (e.g. class name, class method/field/class aspect that it is covering). If this approach is taken, each test should still have a test assertion
in the test method JavaDoc that identifies what the test is testing, so that TCK users can reference the test assertion in TCK challenges (see TCK Process 1.1 document description of valid TCK challenges).
My understanding of the W3C + JCP TCK approach is to define a standard way of listing all of the test assertions. The JCP TCK approach is to use the list of of test assertions to drive development of the TCK tests, in the sense that someone first creates the list of test assertions based on what seems important to them to create tests for (e.g. different parts of the Specification + SPEC API), and then a TCK test is developed specifically for each Test Assertion.
Sorry for the small book inline here but thought it would be good to share some possible options to consider.
Item 2 was not done correctly as the source directory did not change to match the package. As I mentioned in #1606 I don't know how this was compiling, but #1607 corrects this.
Convert jakarta.faces.bean.ManagedBean to CDI bean has been done in https://github.com/eclipse-ee4j/jakartaee-tck/pull/821
For source files see here: https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/internal/docs/jsf (@alwin-joseph maybe those need some cleaning up?)
I will see if I can generate api/javadoc assertions. For spec assertions I'm afraid they are manually modified on the existing assertion file by going through the spec document.
I have a patched hibernate validator and glassfish that get past the CNFE problem, but the cdiBeanValidator is now claiming to need authentication even though I don't see any security settings in the test war. Why would this be?
[INFO] Running ee.jakarta.tck.faces.test.javaee7.cdibeanvalidator.Issue3014IT
Starting container using command: [/Library/Java/JavaVirtualMachines/jdk-11.0.13.jdk/Contents/Home/bin/java, -jar, /Users/starksm/Dev/JBoss/Jakarta/faces/tck/faces22/cdiBeanValidator/target/glassfish7/glassfish/modules/admin-cli.jar, start-domain, -t]
Successfully started the domain : domain1
domain Location: /Users/starksm/Dev/JBoss/Jakarta/faces/tck/faces22/cdiBeanValidator/target/glassfish7/glassfish/domains/domain1
Log File: /Users/starksm/Dev/JBoss/Jakarta/faces/tck/faces22/cdiBeanValidator/target/glassfish7/glassfish/domains/domain1/logs/server.log
Admin Port: 4848
Feb 04, 2022 11:44:59 PM com.gargoylesoftware.htmlunit.WebClient printContentIfNecessary
INFO: statusCode=[403] contentType=[text/html]
Feb 04, 2022 11:44:59 PM com.gargoylesoftware.htmlunit.WebClient printContentIfNecessary
INFO: <html><head><meta http-equiv='refresh' content='1;url=/login?from=%2FcdiBeanValidator%2F'/><script>window.location.replace('/login?from=%2FcdiBeanValidator%2F');</script></head><body style='background-color:white; color:white;'>
Authentication required
<!--
-->
</body></html>
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 17.638 s <<< FAILURE! - in ee.jakarta.tck.faces.test.javaee7.cdibeanvalidator.Issue3014IT
[ERROR] ee.jakarta.tck.faces.test.javaee7.cdibeanvalidator.Issue3014IT.testValidatorInjection Time elapsed: 1.604 s <<< ERROR!
com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 403 Forbidden for http://localhost:8080/cdiBeanValidator/
at ee.jakarta.tck.faces.test.javaee7.cdibeanvalidator.Issue3014IT.testValidatorInjection(Issue3014IT.java:66)
For source files see here: https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/internal/docs/jsf (@alwin-joseph maybe those need some cleaning up?)
I will see if I can generate api/javadoc assertions. For spec assertions I'm afraid they are manually modified on the existing assertion file by going through the spec document.
I have generated new API assertions for 4.0 at https://github.com/eclipse-ee4j/jakartaee-tck/pull/829 . This needs to be reviewed closely by the spec team before considering it as final and moving it to this repo.
The assertions that have been modified are present multiple times with old one with "_OLD" appended. Those will need to be removed after the review. For eg: Both JSF:JAVADOC:6OLD & JSF:JAVADOC:6 ids are present, before removing the JSF:JAVADOC:6OLD it is ideal to be reviewed.
Please note that for the old-style TCK the existing *.jsp files won't work anymore either, and those would have to be converted to .xhtml, which means extra work on top of the already big task.
Any volunteers for this ? What are the other possible changes needed in the test source when the *.jsp files are renamed to .xhtml .
@alwin-joseph I might be able to help. Can you point me to them?
@alwin-joseph I might be able to help. Can you point me to them?
Thanks @kito99 !
The old-style TCK test source are in repo https://github.com/eclipse-ee4j/jakartaee-tck.
The jsf/faces TCK files are present at below folders : https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/src/com/sun/ts/tests/jsf. https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/src/web/jsf
Steps to build/run the Faces TCK :
Setup:
To build the Jakarta Faces TCK zip bundle:
To run the Jakarta Faces TCK against Glassfish:
Thanks! I'll try building sometime in the next few days..
On Wed, Feb 16, 2022 at 7:07 AM Alwin Joseph @.***> wrote:
@alwin-joseph https://github.com/alwin-joseph I might be able to help. Can you point me to them?
Thanks @kito99 https://github.com/kito99 !
The old-style TCK test source are in repo https://github.com/eclipse-ee4j/jakartaee-tck.
The jsf/faces TCK files are present at below folders :
https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/src/com/sun/ts/tests/jsf . https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/src/web/jsf
Steps to build/run the Faces TCK :
- cd /temp ; git clone https://github.com/eclipse-ee4j/jakartaee-tck
- export WORKSPACE=/temp/jakartaee-tck;JAVA_HOME= ; ANT_HOME=<ANT 1.10.11+>;M2_HOME=<Maven3.6.3+> ; GF_BUNDLE_URL=<url for glassfish 7> https://download.eclipse.org/ee4j/glassfish/glassfish-7.0.0-SNAPSHOT-nightly.zip
To build the Jakarta Faces TCK zip bundle:
- cd /temp/jakartaee-tck
- sh docker/build_standalone-tcks.sh jsf
To run the Jakarta Faces TCK against Glassfish:
- sh docker/jsftck.sh
— Reply to this email directly, view it on GitHub https://github.com/jakartaee/faces/issues/1600#issuecomment-1041422245, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYKGUVGQ3IVPK6Q44MFTMLU3OHP5ANCNFSM5MWK4UQQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you were mentioned.Message ID: @.***>
We need to coordinate who is doing what in order to avoid overlap.
I'll for time being take the entire /faces23 on me (juist to avoid overlap because faces22 is the first and faces40 is the last which are more prone to be already picked by someone else).
Sounds good @BalusC!
I'll take a look at "Adding example to run TCK as dependency from Maven into your own pom." then.
/faces23 is done: https://github.com/jakartaee/faces/pull/1614
Given no response I'll presume no one is doing /faces40, I'll take it on me as well.
/faces40 is done: https://github.com/jakartaee/faces/pull/1616
Given no response I'll presume no one is doing /faces22, I'll take it on me as well.
I've moved "additional" tests to TCK (just migrated unmigrated tests from Mojarra 2.3 branch into Faces repo):
That were in total about 80 tests.
@kito99
Thanks! I'll try building sometime in the next few days..
Did you already succeed in building sometime?
Did you already succeed in building sometime? @arjantijms unfortunately not... haven't had time yet.
@alwin-joseph despite @kito99 not having had time, have you been able to make any progress?
@tandraschko @mnriem or @BalusC would you be able to help?
Otherwise maybe we simply must accept that we don't have the time we need and exclude whatever doesn't run by now?
How much time do we have left to finish this?
On Mon, Feb 28, 2022 at 6:15 PM Arjan Tijms @.***> wrote:
@alwin-joseph https://github.com/alwin-joseph despite @kito99 https://github.com/kito99 not having had time, have you been able to make any progress?
@tandraschko https://github.com/tandraschko @mnriem https://github.com/mnriem or @BalusC https://github.com/BalusC would you be able to help?
Otherwise maybe we simply must accept that we don't have the time we need and exclude whatever doesn't run by now?
— Reply to this email directly, view it on GitHub https://github.com/jakartaee/faces/issues/1600#issuecomment-1054799098, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYKGUV2PKLMILBYPQC7XZLU5VHR3ANCNFSM5MWK4UQQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you were mentioned.Message ID: @.***>
How much time do we have left to finish this?
That's a good question. The deadline for doing the spec PR is today, and it references the TCK. But after having done the spec PR it's not that the ballot for it immediately starts. So to the best of my knowledge I'd say it's "not much".
Maybe @scottmarlow or @starksm64 have a better answer here.
The next deadline is completion of the platform specs and TCKs at the end of May. All component specs will have to completed their final ballot which would require TCKs and compatible implementations prior to that, so say May 15 would be the last date for a component spec to have gone to ballot.
@alwin-joseph despite @kito99 not having had time, have you been able to make any progress?
Otherwise maybe we simply must accept that we don't have the time we need and exclude whatever doesn't run by now?
I had not spend time on it yet as I needed some guidance on how to convert the existing *.jsp files in the old-style TCK to .xhtml. What are the additional changes that would require when we rename the .jsp to .xhtml. It would be helpful if there is a sample PR done in the jakartaee-tck for a subset of tests.
Is it also mandatory to change the taglib URIs to new URNs as it was done in https://github.com/jakartaee/faces/issues/1553 for the tests to pass?
For the record, the latest faces tck from jakartaee-tck repo is built at https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/jakarta-faces-tck-4.0.0.zip , the TCK run against Glassfish 7.0.0-M2 (https://jakarta.oss.sonatype.org/content/repositories/staging/org/glassfish/main/distributions/glassfish/7.0.0-M2/glassfish-7.0.0-M2.zip) had only 1 %(~50) of the tests passing now out of the 5k odd tests.
Jenkins job : https://ci.eclipse.org/jakartaee-tck/blue/organizations/jenkins/jakartaee-tck/detail/master/1667/pipeline/67 Full test run logs : https://ci.eclipse.org/jakartaee-tck/blue/rest/organizations/jenkins/pipelines/jakartaee-tck/branches/master/runs/1667/nodes/67/steps/235/log/?start=0
It might also be helpful if these logs are analysed to see which changes are required for most tests to pass.
@BalusC would you be able to help?
With what exactly?
If you're referring to https://github.com/jakartaee/specifications/pull/459#issuecomment-1054394898, the cause is:
11:50:32 AM: Deploy did not succeed: Invalid filename 'specifications/faces/4.0/jsdoc/faces.html#.ajax'. Deployed filenames cannot contain # or ? characters
This has been fixed in https://github.com/eclipse-ee4j/mojarra/pull/5057
With what exactly?
Mostly referring to:
https://github.com/jakartaee/faces/issues/1600#issuecomment-1035912507
Kito offered to help, but unfortunately didn't had time for it.
This was also stated here:
_What are the additional changes that would require when we rename the .jsp to .xhtml. It would be helpful if there is a sample PR done in the jakartaee-tck for a subset of tests.
Is it also mandatory to change the taglib URIs to new URNs as it was done in https://github.com/jakartaee/faces/issues/1553 for the tests to pass?_
So essentially a template to convert pages using JSP to pages using Facelets.
Prepare a new Faces TCK by
ee.jakarta.tck
cc @BalusC @scottmarlow @mriem