Piranha fails to pass all OmniFaces tests - needs fixing #1622

arjantijms · 2021-03-26T15:33:15Z

Piranha currently does not pass all the OmniFaces tests.

The results are as of now:

Failures: 
  FacesConverterIT.test:68 ResourceDependency is injected in facesConverterITManagedConverter expected:<[facesConverterITManagedConverterResourceDependency]> but was:<[]>
  InputFileIT.uploadSingleAccept:126 expected:<[uploadSingle: 6, file-16088362504427706317.gif]> but was:<[label: file-16088362504427706317.gif is not image/*]>
  FacesViewsIT.testNonExistingPage:129->verify404:156 expected:<404> but was:<null>
  MultiViewsIT.testNonExistingPage:118->verify200:164 expected:<...ewsITNonExistingPage[]> but was:<...ewsITNonExistingPage[/MultiViewsIT]>
  MultiViewsIT.testWelcomeFile:67->verify200:164 expected:</MultiViewsIT/foo/42[]> but was:</MultiViewsIT/foo/42[/MultiViewsIT]>
  CDNResourceHandlerIT.cdnResources:44 expected:<http[s://code.jquery.com/ui/1.12.1/themes/base/jquery-ui.css]> but was:<http[://localhost:8080/CDNResourceHandlerIT/RES_NOT_FOUND]>

Errors: 
  FullAjaxExceptionHandlerIT.throwDuringRenderResponse:74 » NoSuchElement Unable...
  FullAjaxExceptionHandlerIT.throwNonAjaxDuringInvokeApplication:79 » IllegalState
  FullAjaxExceptionHandlerIT.throwNonAjaxDuringRenderResponse:100 » NoSuchElement
  FullAjaxExceptionHandlerIT.throwNonAjaxDuringUpdateModelValues:85 » IllegalState
  SocketIT.org.omnifaces.test.push.socket.SocketIT » IllegalState 
  CombinedResourceHandlerIT.ajax:99->verifyElements:128 » NoSuchElement Unable t...
  CombinedResourceHandlerIT.mixed:108->verifyElements:128 » NoSuchElement Unable...
  CombinedResourceHandlerIT.nonAjax:90->verifyElements:128 » NoSuchElement Unabl...

Tests run: 93, Failures: 6, Errors: 8, Skipped: 0

OmniFaces tests can be executed from the root of the Piranha project using:

mvn -amd -B -P external -pl external/omnifaces verify

Debugging can be done by e.g. cd'ing into the external/omnifaces/target/omnifaces-4.x folder and executing:

mvn clean verify -Dmaven.javadoc.skip=true -Dit.test=InputFileIT -P piranha -fae -Dmaven.failsafe.debug

If needed, timeouts can be disabled in OmniFacesIT#init by adding:

browser.manage().timeouts().implicitlyWait(10000, SECONDS);
browser.manage().timeouts().pageLoadTimeout(-1, SECONDS);
	    
System.setProperty(
    "sun.net.client.defaultConnectTimeout", 
    String.valueOf(1000000));// (Unit: ms)
System.setProperty(
    "sun.net.client.defaultReadTimeout",
    String.valueOf(10000000)); // (Unit: ms)

The text was updated successfully, but these errors were encountered:

BalusC · 2021-04-17T13:46:02Z

This one

FacesConverterIT.test:68 ResourceDependency is injected in facesConverterITManagedConverter expected:<[facesConverterITManagedConverterResourceDependency]> but was:<[]>

is Mojarra specific bug: eclipse-ee4j/mojarra#4913

I've already bypassed it for piranha for now: omnifaces/omnifaces@a0ae247

BalusC · 2021-04-17T16:36:07Z

This one

InputFileIT.uploadSingleAccept:126 expected:<[uploadSingle: 6, file-16088362504427706317.gif]> but was:<[label: file-16088362504427706317.gif is not image/*]>

was caused by presumption that all containers have at least a default mime mapping for "common" files (https://www.iana.org/assignments/media-types/media-types.xhtml), however this was not yet true for Piranha, so I've for now added it to web.xml of IT omnifaces/omnifaces@5c66cb8

BalusC · 2021-04-17T16:44:46Z

This one

CDNResourceHandlerIT.cdnResources:44 expected:<http[s://code.jquery.com/ui/1.12.1/themes/base/jquery-ui.css]> but was:<http[://localhost:8080/CDNResourceHandlerIT/RES_NOT_FOUND]>

failed because Piranha's web.xml parser doesn't trim context param values before returning ServletContext#getInitParameter() (and hence the to-be-tested one unexpectedly contained a trailing whitespace character). Looks like bug in Piranha itself.

For the record, this was the context param involved in test:

	<context-param>
		<param-name>org.omnifaces.CDN_RESOURCE_HANDLER_URLS</param-name>
		<param-value>
			cdn:jquery-ui.css=https://code.jquery.com/ui/1.12.1/themes/base/jquery-ui.css,
			cdn:jquery.js=https://code.jquery.com/jquery-2.2.4.min.js
		</param-value>
	</context-param>

BalusC · 2021-04-17T16:50:42Z

This one

SocketIT.org.omnifaces.test.push.socket.SocketIT » IllegalState

failed because of

Caused by: java.lang.NullPointerException: Cannot invoke "jakarta.websocket.server.ServerContainer.addEndpoint(jakarta.websocket.server.ServerEndpointConfig)" because "container" is null
	at org.omnifaces.cdi.push.Socket.registerEndpointIfNecessary(Socket.java:1118)
	... 103 more

coming from

1115		try {
1116			ServerContainer container = (ServerContainer) context.getAttribute(ServerContainer.class.getName());
1117			ServerEndpointConfig config = ServerEndpointConfig.Builder.create(SocketEndpoint.class, SocketEndpoint.URI_TEMPLATE).build();
1118			container.addEndpoint(config);
1119			context.setAttribute(Socket.class.getName(), TRUE);
1120		}
1121		catch (Exception e) {
1122			throw new FacesException(e);
1123		}

So most probably WS container needs to be explicitly initialized for Piranha first in some way?

BalusC · 2021-04-17T16:52:26Z

These:

  FullAjaxExceptionHandlerIT.throwDuringRenderResponse:74 » NoSuchElement Unable...
  FullAjaxExceptionHandlerIT.throwNonAjaxDuringInvokeApplication:79 » IllegalState
  FullAjaxExceptionHandlerIT.throwNonAjaxDuringRenderResponse:100 » NoSuchElement
  FullAjaxExceptionHandlerIT.throwNonAjaxDuringUpdateModelValues:85 » IllegalState
  CombinedResourceHandlerIT.ajax:99->verifyElements:128 » NoSuchElement Unable t...
  CombinedResourceHandlerIT.mixed:108->verifyElements:128 » NoSuchElement Unable...
  CombinedResourceHandlerIT.nonAjax:90->verifyElements:128 » NoSuchElement Unabl...

work fine for me on Piranha 21.4.0.

Thihup · 2021-04-17T16:53:57Z

This one

SocketIT.org.omnifaces.test.push.socket.SocketIT » IllegalState

failed because of

Caused by: java.lang.NullPointerException: Cannot invoke "jakarta.websocket.server.ServerContainer.addEndpoint(jakarta.websocket.server.ServerEndpointConfig)" because "container" is null
	at org.omnifaces.cdi.push.Socket.registerEndpointIfNecessary(Socket.java:1118)
	... 103 more

coming from

1115		try {
1116			ServerContainer container = (ServerContainer) context.getAttribute(ServerContainer.class.getName());
1117			ServerEndpointConfig config = ServerEndpointConfig.Builder.create(SocketEndpoint.class, SocketEndpoint.URI_TEMPLATE).build();
1118			container.addEndpoint(config);
1119			context.setAttribute(Socket.class.getName(), TRUE);
1120		}
1121		catch (Exception e) {
1122			throw new FacesException(e);
1123		}

So most probably WS container needs to be explicitly initialized for Piranha first in some way?

This failed because it is now only included the tyrus-core, not tyrus-servlet, so it doesn't find the ServletContainerInitializer and doesn't start Tyrus. Probably there is an issue wrt to immutable set too.
I guess the change should be in Omnifaces, to include the tyrus-container-servlet, but I'm not sure.

BalusC · 2021-04-17T17:02:19Z

I guess the change should be in Omnifaces, to include the tyrus-container-servlet, but I'm not sure.

Ahh, I see, Piranha didn't ship with any WS container itself, it had to be installed via deployment. I've fixed the test dependency now, but it still fails, this time with

Apr 17, 2021 12:58:46 PM cloud.piranha.webapp.impl.DefaultWebApplication initializeInitializers
WARNING: Initializer org.glassfish.tyrus.servlet.TyrusServletContainerInitializer failing onStartup
java.lang.UnsupportedOperationException
	at java.base/java.util.ImmutableCollections.uoe(ImmutableCollections.java:142)
	at java.base/java.util.ImmutableCollections$AbstractImmutableCollection.removeAll(ImmutableCollections.java:151)
	at org.glassfish.tyrus.servlet.TyrusServletContainerInitializer.onStartup(TyrusServletContainerInitializer.java:71)
	at cloud.piranha.webapp.impl.DefaultWebApplication.initializeInitializers(DefaultWebApplication.java:1399)
	at cloud.piranha.webapp.impl.DefaultWebApplication.initialize(DefaultWebApplication.java:1323)
	at cloud.piranha.micro.core.MicroInnerDeployer.start(MicroInnerDeployer.java:233)

This issue suggests that Piranha is passing an immutable set of classes as 1st argument of onStartup() method. Tyrus was apparently expecting it to be mutable. And frankly I don't see in ServletContainerInitializer#onStartup() Javadoc either that it must be immutable.

BalusC · 2021-04-17T17:22:16Z

This one

FacesViewsIT.testNonExistingPage:129->verify404:156 expected:<404> but was:<null>

failed because piranha didn't send back any Content-Type header on a 404 caused by non-existent *.jsf request, and hence the resulting response wasn't interpretable as a HTML page (in this specific test, the 404 was extracted from the <html><head><title>404</title></head></html> but that obv works only if it's interpreted as a real HTML page).

BalusC · 2021-04-17T17:39:24Z

These

  MultiViewsIT.testNonExistingPage:118->verify200:164 expected:<...ewsITNonExistingPage[]> but was:<...ewsITNonExistingPage[/MultiViewsIT]>
  MultiViewsIT.testWelcomeFile:67->verify200:164 expected:</MultiViewsIT/foo/42[]> but was:</MultiViewsIT/foo/42[/MultiViewsIT]>

I've reproduced, but the code behind this is relatively complicated so I cannot "debug" it by just reading the code. I've not been able to debug Piranha logic yet, so I'll leave this for later. I can at least tell that this test is also failing in Liberty. It's basically related to using path parameters on a welcome file. In Piranha, the test is failing because the context path gets duplicated in request.getRequestURI(). I'm not sure yet if this is caused by OmniFaces or Piranha.

manorrock · 2021-04-18T00:49:09Z

@BalusC thank for analyzing some many of these. @Thihup feel free to pick these off one at a time.

BalusC · 2021-04-18T13:51:37Z

Coming back to the MultiViewsIT, I've fixed the failing case in Liberty. It was caused because it unexpectedly returned an empty string as servlet path when no servlet is found for the request but there is a catch-all filter on /* (this is actually not per spec, it may only return empty string as servlet path when there is an actual servlet on /* ). Liberty has set the expected path in the path info, which is kind of reasonable, so I have work arounded that in OmniFaces: omnifaces/omnifaces@fbdfe7d

But this is not the case in Piranha. Based on the observable symptoms it seems that it's explicitly setting the welcome file path as the path info of the request when no servlet is found, but the servlet path is non-empty. It shouldn't be doing that. I'm also noticing that the return values of the getRequestURI() and getRequestURL() methods seem to be composed based on getPathInfo() while it should really be the other way round; the getPathInfo() must be extracted from the incoming request URL (as set by the client), not be calculated internally.

github-actions · 2021-12-26T01:06:55Z

This issue is stale because it has been open 170 days with no activity. Remove stale label or comment or this will be closed in 10 days

BalusC · 2022-01-13T09:45:40Z

FYI: the following ones still fail in 21.12.0:

FacesViewsIT: Piranha fails to pass all OmniFaces tests - needs fixing #1622 (comment)
CDNResourceHandlerIT: Piranha fails to pass all OmniFaces tests - needs fixing #1622 (comment)
SocketIT: Piranha fails to pass all OmniFaces tests - needs fixing #1622 (comment)

And I've been able to reproduce #1622 (comment), it basically boiled down to a XML parsing error on partial response so the new elements weren't at all applied to the HTML document. I have not debugged it yet. My educated guess would be that the Content-Type header is missing.

BalusC · 2023-12-03T21:25:12Z

Current state: https://github.com/omnifaces/omnifaces/blob/f19595baa1aa9e2da61e9ba059008b198298d31c/pom.xml#L994

The following is the current status as-of Piranha 23.11.0:

[WARNING] Tests run: 141, Failures: 0, Errors: 0, Skipped: 40

The skipped ones can be found via @DisabledIfSystemProperty and they all boil down to following 4 pending issues:
- welcome-file not correctly interpreted (servlet path returns wrong value)
- error-page not correctly interpreted (probably because of being in /WEB-INF or using FacesServlet mapping)
- request.getRequestURL returns different domain (e.g. 'view-localhost' or 'ip6-localhost' instead of 'localhost')
- flash cookie hangs one request too long

And I've for CDNResourceHandlerIT simply bypassed Piranha's web.xml whitespace trimming bug: https://github.com/omnifaces/omnifaces/blob/069bff2b6295827996ef1b72e1d3f04f54f78993/src/test/resources/WEB-INF/web.xml/withCDNResources.xml#L25 although that should also have been fixed -- no one other server fails on this.

As of now, SocketIT passes completely \o/ But apparently a new bug was introduced wrt error-page interpreting. The associated IT tests have previously passed successfully. The getRequestURL and flash cookie issues are also new but the associated IT tests didn't exist at the time when this issue ticket was opened.

mnriem · 2023-12-04T23:28:36Z

@BalusC Can you open issues for each of the 4 pending issues you mentioned above? It will make it easier for me to tackle them one by one. Thanks!

arjantijms added help wanted micro labels Mar 26, 2021

manorrock removed the test label Apr 5, 2021

manorrock removed the micro label Jul 8, 2021

github-actions bot added the stale label Dec 26, 2021

github-actions bot closed this as completed Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Piranha fails to pass all OmniFaces tests - needs fixing #1622

Piranha fails to pass all OmniFaces tests - needs fixing #1622

arjantijms commented Mar 26, 2021 •

edited by Thihup

BalusC commented Apr 17, 2021

BalusC commented Apr 17, 2021 •

edited

BalusC commented Apr 17, 2021 •

edited

BalusC commented Apr 17, 2021

BalusC commented Apr 17, 2021 •

edited

Thihup commented Apr 17, 2021 •

edited

BalusC commented Apr 17, 2021 •

edited

BalusC commented Apr 17, 2021 •

edited

BalusC commented Apr 17, 2021

manorrock commented Apr 18, 2021 •

edited

BalusC commented Apr 18, 2021

github-actions bot commented Dec 26, 2021

BalusC commented Jan 13, 2022 •

edited

BalusC commented Dec 3, 2023 •

edited

mnriem commented Dec 4, 2023

Piranha fails to pass all OmniFaces tests - needs fixing #1622

Piranha fails to pass all OmniFaces tests - needs fixing #1622

Comments

arjantijms commented Mar 26, 2021 • edited by Thihup

BalusC commented Apr 17, 2021

BalusC commented Apr 17, 2021 • edited

BalusC commented Apr 17, 2021 • edited

BalusC commented Apr 17, 2021

BalusC commented Apr 17, 2021 • edited

Thihup commented Apr 17, 2021 • edited

BalusC commented Apr 17, 2021 • edited

BalusC commented Apr 17, 2021 • edited

BalusC commented Apr 17, 2021

manorrock commented Apr 18, 2021 • edited

BalusC commented Apr 18, 2021

github-actions bot commented Dec 26, 2021

BalusC commented Jan 13, 2022 • edited

BalusC commented Dec 3, 2023 • edited

mnriem commented Dec 4, 2023

arjantijms commented Mar 26, 2021 •

edited by Thihup

BalusC commented Apr 17, 2021 •

edited

BalusC commented Apr 17, 2021 •

edited

BalusC commented Apr 17, 2021 •

edited

Thihup commented Apr 17, 2021 •

edited

BalusC commented Apr 17, 2021 •

edited

BalusC commented Apr 17, 2021 •

edited

manorrock commented Apr 18, 2021 •

edited

BalusC commented Jan 13, 2022 •

edited

BalusC commented Dec 3, 2023 •

edited