TCK : Get hostname and port number injected using ArquillianResource url

[alwin-joseph]

• Avoids hardcoding the webServerHost and webServerPort as system properties.
• Tested with Glassfish/Jersey only now.

@jim-krueger @jamezp @jansupol @arjantijms @spericas

[spericas]

@alwin-joseph Seems like the preferred way now is to drop the second year in the copyright header, instead of updating it as you have done.

[jbescos]

I tried it with Helidon and @alwin-joseph branch:

[ERROR] Errors: 
[ERROR]   MultipartSupportIT.basicTest » Runtime Could not lookup value for field private java.net.URI ee.jakarta.tck.ws.rs.jaxrs31.ee.multipart.MultipartSupportIT.uri
[ERROR]   MultipartSupportIT.multiFormParamTest » Runtime Could not lookup value for field private java.net.URI ee.jakarta.tck.ws.rs.jaxrs31.ee.multipart.MultipartSupportIT.uri
[ERROR]   JAXRSSigTestIT.signatureTest:415 Fault signatureTest failed with an unexpected exception
[INFO] 
[ERROR] Tests run: 2649, Failures: 0, Errors: 3, Skipped: 128

It seems MultipartSupportIT was added in other commit, so I guess from Helidon point of view this PR looks fine:

commit 4a3ca8c9856a2d0d16d39ff722e30e5a7807291b
Author: Jim Krueger <[email protected]> 2024-03-20 15:14:07
Committer: GitHub <[email protected]> 2024-03-20 15:14:07
Branches: alwin/javadoc_assertions, alwin/TCK_ArquillianURLInjection

fast-track: Add multipart EntityPart tests (#1230)

* Add multipart EntityPart tests

* Fix tabs to spaces

* Fix issue when changing/adding headers to an EntityPart

* Fix code review comments.

[jamezp]

I think this makes a lot of sense. I made a couple comments that IMO should not block merging given our short time frame.

The two main things I've seen is most of the simple invocations to the super.setup() are not required as the @BeforeEach will be inherited. I was wrong as the parent setup() is not annotated with @BeforeEach.

Also the pattern of port -> string back to an integer doesn't give us much for the isNullOrEmpty() as it will never be null or empty.

I also ran this change against RESTEasy and it passes all but the known issues with the signature tests.

[jamezp]

This shouldn't be required.

[alwin-joseph]

Agree. It was not introduced in this PR, diff has the same line removed at the beginning. Hopefully these along with other occurrences can be addressed in later changes.

[jansupol]

@alwin-joseph

• How does that work? Where does the Arquillian get the hostname and port from? Can the default "localhost:8080" be changed?

• Is it necessary to parse the hostname and port for each test? Is it not possible to do it once for the test class in the constructor as it was done?

[jamezp]

I'm not Alwin, but I can answer some of this :)

@alwin-joseph

• How does that work? Where does the Arquillian get the hostname and port from? Can the default "localhost:8080" be changed?

It's pulled from the org.jboss.arquillian.container.spi.client.protocol.metadata.HTTPContext. Generally this is initialized in the Arquillian protocol being used during the server startup. To my knowledge it's the responsibility of the Arquillian adapter to create this. In WildFly Arquillian we get it from our server configuration after the server has been started.

• Is it necessary to parse the hostname and port for each test? Is it not possible to do it once for the test class in the constructor as it was done?

It more than likely depends, but we could try it with a static @BeforeAll. The depending part depends on how the container is managed by Arquillian.

[alwin-joseph]

@alwin-joseph

* How does that work? Where does the Arquillian get the hostname and port from? Can the default "localhost:8080" be changed?

IIUC the Arquillian adapter for each container handles it. The current jersey-tck/ module uses org.jboss.arquillian.container:arquillian-glassfish-managed-6 to run with Glassfish.

We could also try org.omnifaces.arquillian:arquillian-glassfish-server-managed:1.2 (also used for JSP TCK) where we need to set glassfish.home as system property to locate the glassfish server. The properties for glassfish is set in this connector at https://github.com/OmniFish-EE/arquillian-container-glassfish/blob/36b96e6a9501c1029e76f3eabb746a64695f4920/glassfish-managed/src/main/java/org/omnifaces/arquillian/container/glassfish/managed/GlassFishManagedContainerConfiguration.java#L78C1-L90C96 as suggested here. From what I see the default httpport is 8080 here, glassfish.httpPort is the system property that can be overridden to set the port.

• Is it necessary to parse the hostname and port for each test? Is it not possible to do it once for the test class in the constructor as it was done?

It more than likely depends, but we could try it with a static @BeforeAll. The depending part depends on how the container is managed by Arquillian.

Agree static @BeforeAll can be tried but it looked to me as a lot of places needing changes.

[alwin-joseph]

Is it not possible to do it once for the test class in the constructor as it was done?

The url is injected with @OperateOnDeployment so invoking setup in constructor didn't work for me as the static createDeployment method was not complete then. Invoking the setup in constructor did not work for me as url was not injected with that (The static deployment method was called before constructor) .

Maybe there is a better way (with BeforeAll) but as I see it needs full revamp of the current code structure mostly because the parent JAXRSCommonClient is used for all (with and without deployment) tests.

[jansupol]

I am not very happy with this PR, but maybe I do not see something.

Basically this PR:

• drops calling setup() from the constructor and instead it calls it for each test method
• I do not know HOW EXACTLY the port is changed for instance for running on an app server that runs on a different server than 8080
• it hard codes the ports in some tests
• it comes right at a hectic time of finishing the release, the PR is huge and needs extra time for testing, and I do not see an urge to have it in 4.0

[jamezp]

FWIW JUnit constructs the test for each test method anyway so setup() in a constructor is effectively the same as a @BeforeEach.

The port would be changed in the container and the test would get the correct URL, with the correct deployment context path.

The hard-coded host/ports should probably be configurable via a system property.

That said, I agree this could wait.

[jansupol]

FWIW JUnit constructs the test for each test method anyway so setup() in a constructor is effectively the same as a @beforeeach

That's my bad, then.

The port would be changed in the container and the test would get the correct URL, with the correct deployment context path.

Which container? The Arquillian container, the WebServer, or some other? I really would think it is changed in arquillian.xml.

[jamezp]

FWIW JUnit constructs the test for each test method anyway so setup() in a constructor is effectively the same as a @beforeeach

That's my bad, then.

Nah, it's definitely not well known and honestly took me by surprised when I was debugging once :)

The port would be changed in the container and the test would get the correct URL, with the correct deployment context path.

Which container? The Arquillian container, the WebServer, or some other? I really would think it is changed in arquillian.xml.

By container, I mean the application server. Arquillian has a HTTPContext that is created from which Arquillian Adaptor is used. I WildFly what we do is look at the Undertow subsystem to get the HTTP port number. I assume something similar would be done in GlassFish as well.

I will say this only works for container tests. API tests that are not run with Arquillian wouldn't work.

[jansupol]

By container, I mean the application server. Arquillian has a HTTPContext that is created from which Arquillian Adaptor is used. I WildFly what we do is look at the Undertow subsystem to get the HTTP port number. I assume something similar would be done in GlassFish as well.

This is hard to imagine tbh. whatever lib lies beneath needs to ask the server, but the server needs to listen on some port. If the port is changed, there would need to be some way to tell the arquillian whom to ask. Or instead, tell the Arquillian the port directly. There is the following settings in the arquillian.xml:

<container qualifier="...." default="true">
  <configuration>   
   <property name="managementPort">9991</property>
   <property name="username">admin</property>
   <property name="password">admin</property>
  </configuration>
  <protocol type="Servlet 3.0">
   <property name="port">8081</property>
  </protocol>
</container> 

Which I assume is what is needed, but I would need to test it.

I will say this only works for container tests. API tests that are not run with Arquillian wouldn't work.

The API tests do not need the server, so I assume either port is fine.

[jamezp]

This is hard to imagine tbh. whatever lib lies beneath needs to ask the server, but the server needs to listen on some port. If the port is changed, there would need to be some way to tell the arquillian whom to ask. Or instead, tell the Arquillian the port directly. There is the following settings in the arquillian.xml:

<container qualifier="...." default="true">
  <configuration>   
   <property name="managementPort">9991</property>
   <property name="username">admin</property>
   <property name="password">admin</property>
  </configuration>
  <protocol type="Servlet 3.0">
   <property name="port">8081</property>
  </protocol>
</container> 

Which I assume is what is needed, but I would need to test it.

tl;dr

Yes, the URL is created based on the arquillian.xml configuration.

Longer answer:

Yes, that is how it should work. Arquillian works on the observer pattern and it sends events. It sends an event to start the server and the org.jboss.arquillian.container.spi.client.container.DeployableContainer implementation handles starting the server based on the org.jboss.arquillian.container.spi.client.container.ContainerConfiguration setting, which is created from the arquillian.xml file.

For injecting the URL, that happens after the deployment has been done. The DeployableContainer.deploy() method, returns a ProtocolMetaData which is where the HTTPContext would be attached to the ProtocolMetaData. That HTTPConext is used to get the URL.

The port itself can only be changed before the server is started, so we don't really need to be too concerned about it changing.

[jansupol]

I still do not understand the purpose. Instead of setting the port in the pom.xml we will set the port in the arquillian.xml.
For that, the tests do

1. instead of calling super.setup() they do

  @BeforeEach
  public void setup() {
    super.setup();
  }

2. Many tests include additional logic in a setup method distributed among them:

 public void setup() {

    TestUtil.logTrace("setup method JAXRSSubClientIT");

    assertFalse((url==null), "[JAXRSSubClientIT] 'url' was not injected.");

    String hostname = url.getHost();
    int portnum = url.getPort();

    assertFalse(isNullOrEmpty(hostname), "[JAXRSSubClientIT] 'webServerHost' was not set.");
    _hostname = hostname.trim();
    assertFalse(isPortInvalid(portnum), "[JAXRSSubClientIT] 'webServerPort' was not set.");
    _port = portnum;
    TestUtil.logMsg("[JAXRSSubClientIT] Test setup OK");

3. The tests no longer contain information messages

TestUtil.logMsg("STARTING TEST : "+testInfo.getDisplayName());
TestUtil.logMsg("FINISHED TEST : "+testInfo.getDisplayName());

which might be intentional, or might be moved to the super-class, but the PR description does not say.

[jamezp]

I still do not understand the purpose. Instead of setting the port in the pom.xml we will set the port in the arquillian.xml. For that, the tests do

1. instead of calling super.setup() they do

  @BeforeEach
  public void setup() {
    super.setup();
  }

2. Many tests include additional logic in a setup method distributed among them:

 public void setup() {

    TestUtil.logTrace("setup method JAXRSSubClientIT");

    assertFalse((url==null), "[JAXRSSubClientIT] 'url' was not injected.");

    String hostname = url.getHost();
    int portnum = url.getPort();

    assertFalse(isNullOrEmpty(hostname), "[JAXRSSubClientIT] 'webServerHost' was not set.");
    _hostname = hostname.trim();
    assertFalse(isPortInvalid(portnum), "[JAXRSSubClientIT] 'webServerPort' was not set.");
    _port = portnum;
    TestUtil.logMsg("[JAXRSSubClientIT] Test setup OK");

3. The tests no longer contain information messages

TestUtil.logMsg("STARTING TEST : "+testInfo.getDisplayName());
TestUtil.logMsg("FINISHED TEST : "+testInfo.getDisplayName());

which might be intentional, or might be moved to the super-class, but the PR description does not say.

These are some very good points and is likely a reason we should wait until after 4.0 for this given our very short time frame. I do think the issues will be addressable, but I don't feel we should rush them.

[alwin-joseph]

2. Many tests include additional logic in a setup method distributed among them:

 public void setup() {

    TestUtil.logTrace("setup method JAXRSSubClientIT");

    assertFalse((url==null), "[JAXRSSubClientIT] 'url' was not injected.");

    String hostname = url.getHost();
    int portnum = url.getPort();

    assertFalse(isNullOrEmpty(hostname), "[JAXRSSubClientIT] 'webServerHost' was not set.");
    _hostname = hostname.trim();
    assertFalse(isPortInvalid(portnum), "[JAXRSSubClientIT] 'webServerPort' was not set.");
    _port = portnum;
    TestUtil.logMsg("[JAXRSSubClientIT] Test setup OK");

This does the same as the common setup method in ee.jakarta.tck.ws.rs.common.JAXRSCommonClient.

In some scenarios the test class files(*IT.java) get inherited from other test files (eg: ee.jakarta.tck.ws.rs.ee.rs.cookieparam.sub.JAXRSSubClientIT -> ee.jakarta.tck.ws.rs.ee.rs.cookieparam.JAXRSClientIT )
and they both have deployments; some deployments were named and others were "DEFAULT" which caused issues with the @OperateOnDeployment("<DeploymentName>").

I dont know if removing @OperateOnDeployment for injecting the URL will cause issues with any implementations, but that works for me. In this case the URL will be annotated with only @ArquillianResource.

3. The tests no longer contain information messages

TestUtil.logMsg("STARTING TEST : "+testInfo.getDisplayName());
TestUtil.logMsg("FINISHED TEST : "+testInfo.getDisplayName());

which might be intentional, or might be moved to the super-class, but the PR description does not say.

Yes, this is something that could be moved to super-class ee.jakarta.tck.ws.rs.common.JAXRSCommonClient, which was not intended to be in this PR, I can add it here.

These are some very good points and is likely a reason we should wait until after 4.0 for this given our very short time frame. I do think the issues will be addressable, but I don't feel we should rush them.

I agree we can address these issues after 4.0, my only concern is that to make large changes to the TCK we will have to wait till next major release. But considering there are real concerns with regards to this PR we can hold this off for later time.

[jansupol]

So it sounds like instead of copying the setup logic over to sub-classes, it would be better to change the DEFAULT deployment name to some unique one.

From what I understand from the conversation related to the current release, we want to start working on 5.0 somewhat soonish so that we are ready for EE12, as the transition to CDI is rather a big deal. We will have time to revisit this and address the concerns.

[spericas]

@alwin-joseph @jansupol @jamezp @jbescos @jim-krueger Is anyone against pushing this PR to 5.0?

[jamezp]

Pushing off to 5.0 seems fine with me.