werpu / jsfs_js_ts

Apache License 2.0
2 stars 1 forks source link

Various reported issues in external integration tests #26

Closed werpu closed 1 year ago

werpu commented 1 year ago

https://github.com/apache/myfaces/pull/356#issuecomment-1306143241

They need to be fixed before running an RC.2

werpu commented 1 year ago

I do not want to open a new project for the 4 reported bugs, so I will keep the status updates verbally: Investigating now into the first bug, the f:param, it is very likely a small issue in our new encoder code, which does very likely not merge in passed parameters properly. I had a closely related bug a while ago during development. I will add an integration test on my side to cover this properly in the future.

werpu commented 1 year ago

Normal passthrough seems to work: I am getting a passed test on

             faces.ajax.request(element, null, {
                execute: "input_1",
                render: "@form",
                pass1: "pass1",
                pass2: "pass2",
                message: "Hello World",
                onevent: (evt: any) => {
                    localCnt++;
                }
            });
            ...
            expect(requestBody.indexOf("pass1=pass1")).not.to.eq(-1);
            expect(requestBody.indexOf("pass2=pass2")).not.to.eq(-1);
            expect(requestBody.indexOf("message=Hello%20World")).not.to.eq(-1);

I will add a full integration test now to see what the problem is.

werpu commented 1 year ago

First bug not even in an integration environment reproducible, everything hints towards @volosied having take the outdated master branch as base for his testing. Which would explain the parameter bug. This eerily looks like a blank in params bug i had fixed a while ago. Trying to clear things up on this one, information wise. Cannot really go further until I know he is on the right implementation.

werpu commented 1 year ago

Fileupload test failing, I am not getting the Part data in... Environment Tomcat 10 embedded, this should work out of the box and used to work.

werpu commented 1 year ago

Ok seems like the file data is not properly sent down: ------WebKitFormBoundaryN9g56vmEDjgBUeZk Content-Disposition: form-data; name="testForm:bla"

[object File]

Probably we have a bug in the way we handle form data. This is a regression bug, because it used to work. I am on it.

werpu commented 1 year ago

Found the file upload issue: We have an explicit call to getViewState, to allow decoration of parameters. This viewstate encodes accidentally the input file elements. Also I noticed we basically call the encoding of the viewstates twice, once explicitly and once within our XhrFormData object. This needs to be optimized, but later not yet.

Push Implementation: Seems like this has been a porting regression, the onerror parameter was lost "in translation" I will add another bunch of tests to secure this api once and for all. (The parts of this code were a straight 1:1 port and do not have tests on top yet)

werpu commented 1 year ago

The Push was correctly ported, but there was a spec change, apparently the faces 4.0 spec has added the onerror parameter to the push specification hence breaking backwards compatibility. 3.0 still does not have this parameter (just checked the specs). This is a breaking change which is only documented in the jsdoc of faces 4.0. Just checked against 3.0, the onerror is not there.

Also another huge annoyance, I am not getting any websocket connection at all, seems like our implementation has a problem with registering a jsf websocket endpoint (the server allows it) This is not client related however. Will check what the problem is! Might be related to the fact that I use Weld as IOC provider.

Edit: Not related to Weld, same issue with openwebbeans, I will isolate the issue and send in a bugreport to myfaces.

werpu commented 1 year ago

I now have added a bugreport on the myfaces side with an example replicating the websocket issue, I now to be able to test will write a custom backend on top of the servlet api to bypass the implementation on the servlet side. The push api fortunately is generically enough to allow that. Also I will look into unit tests for this topic to check whether we can add some.

werpu commented 1 year ago

Fixed everything which was reproducible from my side but still needs final testing from @volosied, the html unit issue will be investigated under a new issue, because I am not sure whether we will fix anything here. But I need a proper isolated documentation on the issue.