search

arquillian / arquillian-container-jetty

Arquillian Jetty Containers
7 stars 14 forks source link

Skipped assertions in Jetty 12 tests #197

Closed manovotn closed 6 months ago

manovotn commented 6 months ago

I noticed that tests for Jetty 12 adapter have Weld-related exclusions such as this one - https://github.com/arquillian/arquillian-container-jetty/blob/master/jetty-embedded-12-ee10/src/test/java/org/jboss/arquillian/container/jetty/embedded_12_ee10/JettyEmbeddedInContainerTestCase.java#L119-L121

This should be because you are using empty beans.xml which in CDI 4.0+ translates to discovery mode annotated and only beans with bean defining annotations will be discovered.

In other words, if you add @Dependent annotation onto your bean class (here) it will be discovered. Alternatively, you could provide beans.xml which explicitly declares all discovery mode. Both approaches should get you to previous behavior.

That being said, doing so gave me a new stack that's not Weld related and that someone here (@joakime or @olamy) might know more about:

[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 2.401 s <<< FAILURE! -- in org.jboss.arquillian.container.jetty.embedded_12_ee10.JettyEmbeddedInContainerTestCase
[ERROR] org.jboss.arquillian.container.jetty.embedded_12_ee10.JettyEmbeddedInContainerTestCase -- Time elapsed: 2.401 s <<< ERROR!
org.jboss.arquillian.container.spi.client.container.DeploymentException: Could not deploy 1a2f6627-44ff-4fd1-a990-56df29197acd.war
    at org.jboss.arquillian.container.jetty.embedded_12_ee10.JettyEmbeddedContainer.deploy(JettyEmbeddedContainer.java:273)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$3.call(ContainerDeployController.java:151)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$3.call(ContainerDeployController.java:118)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.executeOperation(ContainerDeployController.java:239)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.deploy(ContainerDeployController.java:118)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
    at org.jboss.arquillian.container.impl.client.container.DeploymentExceptionHandler.verifyExpectedExceptionDuringDeploy(DeploymentExceptionHandler.java:47)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
    at org.jboss.arquillian.container.impl.client.ContainerDeploymentContextHandler.createDeploymentContext(ContainerDeploymentContextHandler.java:71)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
    at org.jboss.arquillian.container.impl.client.ContainerDeploymentContextHandler.createContainerContext(ContainerDeploymentContextHandler.java:54)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
    at org.jboss.arquillian.core.impl.EventImpl.fire(EventImpl.java:62)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$1.perform(ContainerDeployController.java:92)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$1.perform(ContainerDeployController.java:77)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.forEachDeployment(ContainerDeployController.java:232)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.forEachManagedDeployment(ContainerDeployController.java:212)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.deployManaged(ContainerDeployController.java:77)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
    at org.jboss.arquillian.core.impl.EventImpl.fire(EventImpl.java:62)
    at org.jboss.arquillian.container.test.impl.client.ContainerEventController.execute(ContainerEventController.java:96)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
    at org.jboss.arquillian.test.impl.TestContextHandler.createClassContext(TestContextHandler.java:83)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
    at org.jboss.arquillian.test.impl.TestContextHandler.createSuiteContext(TestContextHandler.java:69)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
    at org.jboss.arquillian.test.impl.EventTestRunnerAdaptor.beforeClass(EventTestRunnerAdaptor.java:89)
    at org.jboss.arquillian.junit5.ArquillianExtension.beforeAll(ArquillianExtension.java:35)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.NullPointerException: Cannot invoke "jakarta.servlet.ServletContext.getContextPath()" because the return value of "org.eclipse.jetty.ee10.servlet.ServletHolder.getServletContext()" is null
    at org.jboss.arquillian.container.jetty.embedded_12_ee10.JettyEmbeddedContainer.deploy(JettyEmbeddedContainer.java:269)
    ... 53 more
joakime commented 6 months ago 
Caused by: java.lang.NullPointerException: Cannot invoke "jakarta.servlet.ServletContext.getContextPath()" because the return value of "org.eclipse.jetty.ee10.servlet.ServletHolder.getServletContext()" is null
  at org.jboss.arquillian.container.jetty.embedded_12_ee10.JettyEmbeddedContainer.deploy(JettyEmbeddedContainer.java:269)
  ... 53 more

That comes from using the wrong (not recommended) initialization of weld with Jetty 12.

Specifically the environment is using either a Fallback or DecoratingListener approach to initialize Weld.

There are many ways to initalize Weld with Jetty.

Take this example from Jetty 11.0.x

https://github.com/jetty/jetty-examples/blob/11.0.x/embedded/jersey-weld/src/main/java/examples/Main.java

The only one that should be used is the one labelled as RECOMMENDED.

If you look at that same example, but for Jetty 12, you'll see that options for FALLBACK and DECORATING_LISTENER have been removed, as they do not work with the Environment features on Jetty 12. (Weld makes assumptions that no longer hold true in Jetty 12)

https://github.com/jetty/jetty-examples/blob/12.0.x/embedded/ee10-jersey-weld/src/main/java/examples/Main.java

manovotn commented 6 months ago

So you're basically saying that you can no longer use org.jboss.weld.environment.servlet.Listener as a listener to bootstrap Weld with Jetty?

In that case that looks like a fault in the container test setup, namely in the web.xml file - https://github.com/arquillian/arquillian-container-jetty/blob/master/jetty-embedded-12-ee10/src/test/resources/in-container-web.xml#L21

Either way, it means that part of the JettyContainer Weld code is probably outdated by now. It is also unfortunate as other servlets will work with the Listener we provide while Jetty requires extra steps.

joakime commented 6 months ago

Use the following code as an example of what the Weld folks recommend.

// Jetty setup telling Jetty's CdiDecoratingListener how to operate.
context.setInitParameter(CdiServletContainerInitializer.CDI_INTEGRATION_ATTRIBUTE, CdiDecoratingListener.MODE);
// jetty setup for layer between Weld and Jetty.
context.addServletContainerInitializer(new CdiServletContainerInitializer());
// weld setup, the Weld recommended way to use initialize weld. (this is supposed to be true for all containers)
context.addServletContainerInitializer(new org.jboss.weld.environment.servlet.EnhancedListener());

Note: the ee11 changes to CDI are going to change things again. And the expected changes in ee12 for CDI will be dramatic. (eg: ee12 weld will container NO servlet integration points, this responsibility is being moved to the Servlet spec itself, ugh)

manovotn commented 6 months ago

... Weld folks recommend.

I am a Weld folk and I am just trying to figure out where we stand with servlet integration and what needs to be updated :)

@joakime based on what you said, I think the Arq. adapter should then look like this -> https://github.com/arquillian/arquillian-container-jetty/pull/198

That's passing with zero changes to Weld itself. That being said, the reason it previously failed is because Weld environment detection failed to spot it's running on newer Jetty (because of missing init parameter) and ended up with legacy jetty fallback. We can purge the legacy bits from Weld (both 5.x and upcoming 6.x) as those aren't useful in any case.

We could also update Weld's JettyContainer and only keep the two valid options which are:

If so, then the integration can be simplified like so -> https://github.com/weld/core/pull/2959 Is that correct?

Note: the ee11 changes to CDI are going to change things again.

How exactly? Do we need to adapt Weld 6 to this somehow?

joakime commented 6 months ago

... Weld folks recommend.

I am a Weld folk and I am just trying to figure out where we stand with servlet integration and what needs to be updated :)

@joakime based on what you said, I think the Arq. adapter should then look like this -> #198

That's passing with zero changes to Weld itself. That being said, the reason it previously failed is because Weld environment detection failed to spot it's running on newer Jetty (because of missing init parameter) and ended up with legacy jetty fallback. We can purge the legacy bits from Weld (both 5.x and upcoming 6.x) as those aren't useful in any case.

Yes I agree with the changes in #198 and the proposed removal of the legacy bits. Jetty 9 thru 11 are at End of Community support anyway.

We could also update Weld's JettyContainer and only keep the two valid options which are:

  • CdiSpiDecorator
  • CdiDecoratingListener

If so, then the integration can be simplified like so -> weld/core#2959 Is that correct?

The CdiSpiDecorator is an internal class. Not sure why are even referencing that.

Lets look at the example from Jetty 9.4.x

https://github.com/jetty/jetty-examples/blob/9.4.x/embedded/servlet-with-cdi/src/main/java/examples/ServerMain.java#L66-L69

 // Enable Weld + CDI
context.setInitParameter(CdiServletContainerInitializer.CDI_INTEGRATION_ATTRIBUTE, CdiDecoratingListener.MODE);
context.addBean(new ServletContextHandler.Initializer(context, new CdiServletContainerInitializer()));
context.addBean(new ServletContextHandler.Initializer(context, new org.jboss.weld.environment.servlet.EnhancedListener()));

And also in Jetty 12 (ee-10)

https://github.com/jetty/jetty-examples/blob/12.0.x/embedded/ee10-servlet-with-cdi/src/main/java/examples/ServerMain.java#L66-L69

// Enable Weld + CDI
context.setInitParameter(CdiServletContainerInitializer.CDI_INTEGRATION_ATTRIBUTE, CdiDecoratingListener.MODE);
context.addServletContainerInitializer(new CdiServletContainerInitializer());
context.addServletContainerInitializer(new org.jboss.weld.environment.servlet.EnhancedListener());

Just using that technique is compatible across many major versions of Jetty.

As for removing the Weld side legacy features, that seems very similar to the past issues.

We have no issues with that.

Note: the ee11 changes to CDI are going to change things again.

How exactly? Do we need to adapt Weld 6 to this somehow?

There has been a slow process to push some of the responsibilities of the CDI implementations (weld, owb, etc) onto the various containers (servlet, websocket, etc).

See thread "[servlet-dev] New home for HttpServletRequest injection requirements" in https://www.eclipse.org/lists/servlet-dev/msg00638.html

While the changes in ee11 are minor, they are impacting Jetty as we have more and more tasks down in Jetty that we need to do for CDI. The plan for ee12 will be dramatic, I fully expect weld and owb to fully REMOVE their servlet integration points, and instead rely on what is provided by the Servlet containers. (eg: you will no longer have ServletContainerInitializers, and instead the Servlet container will have to integrate with weld via code. you will no longer have the ability to have weld in the WEB-INF/lib as the CDI layer is provided by the servlet container. you will no longer be able to have different versions of CDI per webapp, as CDI will be forced from the servlet container.)

manovotn commented 6 months ago

The CdiSpiDecorator is an internal class. Not sure why are even referencing that.

That seems to have gotten there as part of https://github.com/weld/core/commit/c5b20b05f62acf062c7a565c0ead73c294e7f8a1 I don't know why it was added there originally but it most likely server as means of telling Weld that if present we needn't do anything else (neither perform extra work nor throw an exception). I can at least add a comment mentioning its internal class and only servers as means of telling apart undefined state versus no-op state.

While the changes in ee11 are minor, they are impacting Jetty as we have more and more tasks down in Jetty that we need to do for CDI.

If those changes need to be reflected in Weld integration code please do reach out so that we can fix that :)

The plan for ee12 will be dramatic, I fully expect weld and owb to fully REMOVE their servlet integration points, and instead rely on what is provided by the Servlet containers. (eg: you will no longer have ServletContainerInitializers, and instead the Servlet container will have to integrate with weld via code. you will no longer have the ability to have weld in the WEB-INF/lib as the CDI layer is provided by the servlet container. you will no longer be able to have different versions of CDI per webapp, as CDI will be forced from the servlet container.)

I see, so essentially the same as what you have in full blown app servers.

Thanks for the information and your feedback. I am (re)discovering lots of this as I was trying to re-enable Weld testing (https://github.com/arquillian/arquillian-container-jetty/issues/185); so far unsuccessfully but at least I learned something.