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 faces.js reimplementation (target 4.0.0 RC4) #356

Closed
wants to merge 68 commits into from
Closed

Feature/myfaces 4466 faces.js reimplementation (target 4.0.0 RC4) #356

wants to merge 68 commits into from

Conversation

werpu
Copy link
Contributor

@werpu werpu commented Oct 19, 2022

had to recreate the pull request because i had to rename the branch

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

Yikes OK then I will leave it up to @tandraschko @volosied what they want to do. Sounds like this could be tricky/messy.

@werpu
Copy link
Contributor Author

werpu commented Dec 6, 2022

Yes thats the reason why I started with the selenium port of the TCK to begin with, to get the tests out of the tech dead end they obviously have been for quite a while. Selenium simply is under active development and has strong corporate backing.
Whether the Jakarta is going to pick up the PR or not, good question, but so far I have not seen any significant feedback.

@volosied
Copy link
Contributor

volosied commented Dec 6, 2022

Hi, let me find out if there's been any progress or testing done with the Selenium TCK PR (1732) yet.

@werpu
Copy link
Contributor Author

werpu commented Dec 7, 2022

Hi I just noticed i ported from a rather old branch (4.0.1) there have been many bugfixes going into the TCK, so I will have te Re-port again and update the pull request accordingly. This will happen until sometime next week.
(we have a holiday tomorrow)
not a big deal but a little bit of work. With that I also can add my latest base framework which has a few smaller fixes over the PR I offered for the TCK.
(already added a comment, that they should postpone until next week for that), but nevertheless an info would be nice if they would be willing to add selenium based tck tests at least optionally.
So that we can test as well from the official tck codebase! I cannot do more than to hand over the ported code to them. I am not a comitter on their side.

@werpu
Copy link
Contributor Author

werpu commented Dec 9, 2022

I will resolve the conflicts next week, they are related to my work on MYFACES-4526, MYFACES-4040

@melloware
Copy link
Contributor

@werpu I really appreciate all your work on this!

@werpu werpu closed this by deleting the head repository Dec 12, 2022
@werpu werpu reopened this Dec 12, 2022
@melloware
Copy link
Contributor

Says there are conflicts?

@werpu werpu changed the title Feature/myfaces 4466 faces.js reimplementation (target 4.0.0 RC3) Feature/myfaces 4466 faces.js reimplementation (target 4.0.0 RC4) Dec 14, 2022
@werpu
Copy link
Contributor Author

werpu commented Dec 14, 2022

Hi I have reopened the tck ajax pull request, this time against the proper branch jakartaee/faces#1770

Lets see if they take it in!

@werpu
Copy link
Contributor Author

werpu commented Dec 21, 2022

Not sure what the problem atm is by showing me conflicts... the branches are in sync but github is giving me sync problems to my private fork
Lets wait until this is resolved.
Otherwise I need to refork before the final merge of this pr! But I guess it will resolve itself over time. Please ignore the conflict warning for now:

image

@bohmber
Copy link
Contributor

bohmber commented Jan 19, 2023

I would like to see this merged

@werpu
Copy link
Contributor Author

werpu commented Feb 3, 2023

Hi following:
Whats the plan. first I need to merge the fixes we got from the server for TCK 790, then also my client fixes for 790 which I have under a different branch.

The question now is after that how we proceed. The TCKs pass as well as with the old codebase, but with the selenium version. If that is good enough for being able to say we are TCK compliant, then I am fine.
If not we have to think about another solution maybe running a 4.0 next branch, or having both implementations available for a transition period!

On the TS side I want to do a cleanup of the XHRFormData the upcoming weeks, the aim is to reduce complexity (it has grown too complex for my liking and has a ton of simplification potential), but that is independent of the merge!

@tandraschko
Copy link
Member

tandraschko commented Feb 3, 2023

what about:

  • try to fix all TCK bugs (MYFACES-4517, MYFACES-4489)
  • create a new Milestone release
  • in meantime you can cleanup your code and we can merge it
  • create a new Milestone with the new JS base
  • some testing time
  • final release

WDYT @volosied @melloware @pnicolucci ?

@melloware
Copy link
Contributor

I like that idea.

@volosied
Copy link
Contributor

volosied commented Feb 3, 2023

MYFACES-4517 is a duplicate of MYFACES-4425

This test was challenged and accepted: https://github.com/jakartaee/faces/issues/1757. Do you still want this fixed?

MYFACES-4489 was also challenged: https://github.com/jakartaee/faces/issues/1734. However, since I worked on the previous flow issue, I might be able to find a fix for it.

@tandraschko
Copy link
Member

if they are challenged and closed, please lets just close our JIRA issues

@werpu
Copy link
Contributor Author

werpu commented Feb 3, 2023

As I said my cleanup is independent of anything... we can do it before or after. Preferrably before.

I just did the major merge and have pushed it.
The TCKs work as before, my integration tests pass as well as before.
The only thing I noticed is that the last ajax error case of the integration tests in myfaces falls (did not test that before)
I had a look at the test, it probably is outdated and tests for an impl detail which has slightly changed (I have similar tests in my newer integration testsuite)
Nevertheless I need to investigate this, but next week. Already filed a bug for it and assigned it to me!

@werpu
Copy link
Contributor Author

werpu commented Feb 3, 2023

Sorry for the delay, but I was held up work related issues.
Here is a small preview how the code will look like after the cleanup.

https://gist.github.com/werpu/47ef2a062e82032551706b9effed316d

I have to investigate a file upload issue reported by the Tobago guys, but once they give the ok, I will be ready to merge.
This should be the last bigger refactoring before the merge!

@werpu
Copy link
Contributor Author

werpu commented Feb 8, 2023

All the changes are merged in, waiting for the final test of the Tobago people, TCK tests pass!
Enjoy the improved code. This was very likely the last big refactoring on my part before the final merge. The rest of the code is more or less in a state I can live with!

Note, seems like the pull request is lost in git, we will have to reopen a new one before the merge, here is the commit:

werpu@854f1cc

and here the full branch, as you can see no conflicts:
https://github.com/werpu/myfaces/tree/feature/MYFACES-4466

@werpu
Copy link
Contributor Author

werpu commented Feb 8, 2023

@volosied @tandraschko, I have yet to check the status, but it seems like the selenium based tests are now part of the official TCK, how about a merge of this early next week?
So we can get finally the new codebase in for RC5?
The tests were the big showstopper, now they are resolved, is there anything preventing the merge?

@tandraschko
Copy link
Member

+1 from my side
we could even release 4.0.0 - all tickets are done after this
wdyt @volosied @pnicolucci ?

@volosied
Copy link
Contributor

volosied commented Feb 8, 2023

I discovered a selectOneRadio bug caused by one of my changes. I am working to fix it now (will make a JIRA soon).

@pnicolucci
Copy link
Contributor

I think an RC5 is the best bet to roll up all the changes then we can run everything against the TCK. Then do a 4.0.0 release.

@werpu
Copy link
Contributor Author

werpu commented Feb 8, 2023

The Tobago guys also gave my internal jsf_ts RC.33 the ok which is the build the last downstream merge is based on!
Just checked the TCK, my selenium tests have been added.
I will recheckout the TCK and run it against my codebase. (The tests which need selenium)
If all goes well I would merge around Monday or Tuesday if that is ok with everyone.

@werpu
Copy link
Contributor Author

werpu commented Feb 10, 2023

Closing this now, I have opened another pull request: #514

Please continue the discussion there

@werpu werpu closed this Feb 10, 2023
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.

7 participants