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

Feature/myfaces 4466: Merge Pull request into main (RC5 Target) - Faces.js new scripts! #514

Merged
merged 152 commits into from
Feb 13, 2023

Conversation

werpu
Copy link
Contributor

@werpu werpu commented Feb 10, 2023

I have to open another pull request on the same branch, because the original one has been showing a merge error for quite some time. The merge is imminent, I just want to run another set of TCKs tests with the latest codebase

werpu and others added 30 commits September 26, 2022 12:53
…ES-4456

# Conflicts:
#	api/src/main/javascript/META-INF/resources/myfaces/_impl/xhrCore/_AjaxUtils.js
* MYFACES-4468: Implement f:selectItemGroup

* Create test for  f:selectItemGroup

* Resolve TCK signature failures (Remove Added Fields)
integrated dom query sources, another fix for the nonce handling
updating from the github upstream project.
Fix for an eval regression introduced by our nonce fixes
(only the new codebase is affected)
integratioin test 5, double eval failed because of it
updating to the latest mona-dish (no fixes but just to be
in sync)
# Conflicts:
#	api/src/main/javascript/META-INF/resources/myfaces/_impl/_util/_Dom.js
#	api/src/main/javascript/META-INF/resources/myfaces/_impl/core/_EvalHandlers.js
#	api/src/main/javascript/META-INF/resources/myfaces/_impl/core/_Runtime.js
werpu added 7 commits February 3, 2023 17:48
merging t-andraschkos changes with mine on the TCK 790...
All TCKs pass as good as on the master now!
Also mergin in the latest fixes for bugs reported by Tobago in the JS codebase
merging t-andraschkos changes with mine on the TCK 790...
All TCKs pass as good as on the master now!
Also mergin in the latest fixes for bugs reported by Tobago in the JS codebase
# Conflicts:
#	api/src/main/javascript/META-INF/resources/myfaces/_impl/xhrCore/_AjaxUtils.js
#	impl/src/main/java/org/apache/myfaces/component/validate/ValidateWholeBeanComponent.java
Update to the latest cleaned up codebase
XhrFormData noa finally in a state I can live with
all tests pass TCK as well!
@werpu
Copy link
Contributor Author

werpu commented Feb 10, 2023

For the sake of documentation, I will close the old pull request, because it has been broken for quite a while.
So that nothing gets lost documentationwise, here is the link:
#356

(The old one will be closed now)

@werpu werpu changed the title Feature/myfaces 4466: Merge Pull request into main Feature/myfaces 4466: Merge Pull request into main (RC5 Target) Feb 10, 2023
@werpu werpu changed the title Feature/myfaces 4466: Merge Pull request into main (RC5 Target) Feature/myfaces 4466: Merge Pull request into main (RC5 Target) - Faces.js new scripts! Feb 10, 2023
@werpu
Copy link
Contributor Author

werpu commented Feb 10, 2023

The failing test is invalid, it relies on the onerror handler being called if source is not set:
The spec states:

  • If the source element is null or undefined throw an error.

I also did a double check with a running mojarra server, and there also the onerror handler is not called for this case.
So the test passing a null as source element expecting the onerror handler to be called is wrong here. I will remove the testcase. It is not valid anymore! And the new implementation behaves 100% spec conform for this (and does the same as Mojarra)

removing an invalid test!
(not spec conform and neither does mojarra behave like that)
@werpu
Copy link
Contributor Author

werpu commented Feb 10, 2023

TCKs pass, however the Tobago people reported an issue with the fileupload for the RC34 of the jsf_ts codebase which needs a fix, so no merge this week, RC33 worked, but I do not want to fork the codebase there.

Fixed I will rerun the TCKs now, hopefully for a last time before going into the merge!

Refixing the fileupload after a report by the Tobag guys.
@werpu
Copy link
Contributor Author

werpu commented Feb 10, 2023

Integration Tests pass:
image

TCK:
Faces40 / Ajax:
image

Faces 22
image

Faces 23/ajax:
image

Namespace:
image

Note originally in faces 23 Test 1423 was failing due to a race condition between webdriver and js engine execution, this is an issue of the test itself (timing issue, with a race condition between the webdriver and the test code, which is not fixable on framework level, but the test itself needs adaption)
The adapted test passes. I will issue a fix for the test on the TCK side on monday together with a small selenium test framework update!

Tobago guys also gave the ok... seems like we are set now...

I already am in the weekend, so I will merge monday!
This was a long one, but finally!

@werpu
Copy link
Contributor Author

werpu commented Feb 12, 2023

Hi bad news... i got a mild infection, and currently being in bed with fever, so I will leave the honor for the merge to @volosied, @tandraschko or @pnicolucci unless you want to wait until I have recovered (which might be wednesday)
Feel free to go ahead with the merge!

@tandraschko
Copy link
Member

tandraschko commented Feb 13, 2023

@werpu i did a small review again and looks good
i just wonder if you could give a small explantation about the server side changes?

but wednesday is enough, gute besserung :)

@werpu
Copy link
Contributor Author

werpu commented Feb 13, 2023

This is mostly a complexity reduction, because instead of 4-5 different target builds (without mapping files back then) we had in the original, we now have only two: production and development, but also we have mapping files. Thats all there is to it.

@tandraschko
Copy link
Member

lets merge this
thanks for your great and hard work werner!

@tandraschko tandraschko merged commit e4a4aff into apache:main Feb 13, 2023
@melloware
Copy link
Contributor

I will second that I know this was a LOT of work.

@tandraschko
Copy link
Member

the build somehow fails on localhost:

myfaces.faces.ajax@1.0.0 test
env TS_NODE_PROJECT=./tsconfig-myfaces.json mocha

Der Befehl "env" ist entweder falsch geschrieben oder konnte nicht gefunden werden.

any idea?

@werpu
Copy link
Contributor Author

werpu commented Feb 13, 2023

@tandraschko thanks for the merge, this is a windows build issue, as it seems, which I have not caught upfront:

webpack/webpack.js.org#2733
Try the cross-env solution, if this fails I will work on this on wednesday when I am back.

You have to run an npm install first, it wont be there automatically: https://stackoverflow.com/questions/11928013/node-env-is-not-recognized-as-an-internal-or-external-command-operable-comman

Another option would be to detect the build os on programmatical level and set the environment vars accordingly. Either way if you cannot figure it out, then I will fix it Wednesday for you. Should not block the CI build which is running on a unixoid environment, but needs to be fixed, feel free to open a bug report and assign it to me.

@werpu
Copy link
Contributor Author

werpu commented Feb 15, 2023

@tandraschko could you resolve the issue? Otherwise I will look into it!

@tandraschko
Copy link
Member

@werpu i didnt had the time
if you can "fix" our code, i can just do a mvn install again on my machine to test it

@werpu
Copy link
Contributor Author

werpu commented Feb 15, 2023

resolved it with @tandraschko, we can see the issue as closed now. The fix was indeed to use the cross-env package as replacement for the env command. I was not aware of the cross platform issues env in Node has and many people aren´t as it seems. Fix is merged by @tandraschko

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.

5 participants