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

websocket test refactor with Junit and arquillian #1160

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

@gurunrao gurunrao force-pushed the tck-refactor-websocket-2 branch 2 times, most recently from af11cd8 to 5779ac7 Compare April 6, 2023 07:05
@olamy
Copy link
Contributor

olamy commented Apr 7, 2023

is it using arquilian?

@gurunrao gurunrao changed the title websocket test refactor with Junit. websocket test refactor with Junit and arquillian Apr 7, 2023
@gurunrao
Copy link
Contributor Author

gurunrao commented Apr 7, 2023

is it using arquilian?

Yes, it uses arquillian and junit5.

@olamy
Copy link
Contributor

olamy commented Apr 7, 2023

is it using arquilian?

Yes, it uses arquillian and junit5.

great. I will have a try next week with Jetty.

@gurunrao gurunrao force-pushed the tck-refactor-websocket-2 branch 4 times, most recently from 11515ae to a323f8b Compare April 14, 2023 03:01
@gurunrao gurunrao force-pushed the tck-refactor-websocket-2 branch 7 times, most recently from 2783063 to 3791851 Compare April 25, 2023 11:19
@gurunrao gurunrao marked this pull request as ready for review May 23, 2023 05:00
@olamy
Copy link
Contributor

olamy commented May 30, 2023

Please have a look at this PR https://github.com/gurunrao/jakartaee-tck/pull/8
The host:port should be coming from Arquillian. Some container such jetty just start some random so we cannot expect to have hardcoded port before starting the build

@olamy
Copy link
Contributor

olamy commented May 30, 2023

why renaming all classes to *IT?

@gurunrao
Copy link
Contributor Author

why renaming all classes to *IT?

fail-safe plugin default inclusion wildcards are as follows:
https://maven.apache.org/surefire/maven-failsafe-plugin/examples/inclusion-exclusion.html

@gurunrao
Copy link
Contributor Author

gurunrao commented May 30, 2023

Please have a look at this PR gurunrao#8 The host:port should be coming from Arquillian. Some container such jetty just start some random so we cannot expect to have hardcoded port before starting the build

The host:port were configured in jtest and harness TCK (https://github.com/jakartaee/platform-tck/blob/master/install/websocket/bin/ts.jte#L177), we intend to keep the functionality as before.

@olamy
Copy link
Contributor

olamy commented May 30, 2023

Please have a look at this PR gurunrao#8 The host:port should be coming from Arquillian. Some container such jetty just start some random so we cannot expect to have hardcoded port before starting the build

The host:port were configured in jtest and harness TCK (https://github.com/jakartaee/platform-tck/blob/master/install/websocket/bin/ts.jte#L177), we intend to keep the functionality as before.

Yes but now with Arquillian this works totally different and the change from the PR must be applied. I have tested with gf runner and this works. This has been done as this with servlet tck as this is simply using the hardcoded port return by Arquillian GH container.

If not changed there jetty or any arquillian container using random port will not being able to use this TCK code.

cc @starksm64

@gurunrao gurunrao dismissed olamy’s stale review May 31, 2023 03:24

the code is part of old tck.

@joakime
Copy link
Contributor

joakime commented Jun 8, 2023

The host:port were configured in jtest and harness TCK (https://github.com/jakartaee/platform-tck/blob/master/install/websocket/bin/ts.jte#L177), we intend to keep the functionality as before.

I don't like this.
The hardcoded port should not be the default or mandatory behavior.
All of the bad behaviors of jtest should be considered deprecated legacy behaviors.
Use junit and arquillian properly and don't hang onto the bad decisions from the past.

@gurunrao
Copy link
Contributor Author

gurunrao commented Jun 9, 2023

The host:port were configured in jtest and harness TCK (https://github.com/jakartaee/platform-tck/blob/master/install/websocket/bin/ts.jte#L177), we intend to keep the functionality as before.

I don't like this. The hardcoded port should not be the default or mandatory behavior. All of the bad behaviors of jtest should be considered deprecated legacy behaviors. Use junit and arquillian properly and don't hang onto the bad decisions from the past.

Port is configured using properties file and not hardcoded.

@gurunrao gurunrao closed this Jun 9, 2023
@gurunrao gurunrao reopened this Jun 9, 2023
@olamy
Copy link
Contributor

olamy commented Jun 9, 2023

The host:port were configured in jtest and harness TCK (https://github.com/jakartaee/platform-tck/blob/master/install/websocket/bin/ts.jte#L177), we intend to keep the functionality as before.

I don't like this. The hardcoded port should not be the default or mandatory behavior. All of the bad behaviors of jtest should be considered deprecated legacy behaviors. Use junit and arquillian properly and don't hang onto the bad decisions from the past.

Port is configured using properties file and not harcoded.

No, it is definitely hardcoded in the pom.
The port will be defined by Arquillian (e.g each Arquillian impl will have its own file and so it will be provided by the URL injected by Arqullian).
Please look at how every other TCKs (servlet and others) have been translated to Arquillian. They are all using this feature.
If you want to use hardcoded port fair enough but use them only in the glassfish runner part as I'm not if the arquillian glassfush can use random port.
Please look at this PR https://github.com/gurunrao/jakartaee-tck/pull/8. the change is very simple.
especillay this line https://github.com/gurunrao/jakartaee-tck/pull/8/files#diff-cbb04ed2421f4d932ce2e779fd6fb96ebaa87f01886848dbfb4c0fb36d13e946R123

cc @starksm64 @scottmarlow

@gurunrao
Copy link
Contributor Author

gurunrao commented Jun 9, 2023

The host:port were configured in jtest and harness TCK (https://github.com/jakartaee/platform-tck/blob/master/install/websocket/bin/ts.jte#L177), we intend to keep the functionality as before.

I don't like this. The hardcoded port should not be the default or mandatory behavior. All of the bad behaviors of jtest should be considered deprecated legacy behaviors. Use junit and arquillian properly and don't hang onto the bad decisions from the past.

Port is configured using properties file and not harcoded.

No, it is definitely hardcoded in the pom. The port will be defined by Arquillian (e.g each Arquillian impl will have its own file and so it will be provided by the URL injected by Arqullian). Please look at how every other TCKs (servlet and others) have been translated to Arquillian. They are all using this feature. If you want to use hardcoded port fair enough but use them only in the glassfish runner part as I'm not if the arquillian glassfush can use random port. Please look at this PR gurunrao#8. the change is very simple. especillay this line https://github.com/gurunrao/jakartaee-tck/pull/8/files#diff-cbb04ed2421f4d932ce2e779fd6fb96ebaa87f01886848dbfb4c0fb36d13e946R123

cc @starksm64 @scottmarlow

https://github.com/gurunrao/jakartaee-tck/pull/8 - doesn't take care of secure ports

@joakime
Copy link
Contributor

joakime commented Jun 9, 2023

gurunrao#8 - doesn't take care of secure ports

The example of gurunrao#8 shows how it works with a default deployment.

	@ArquillianResource
	@OperateOnDeployment("_DEFAULT_")
	public URL url;

Take the Servlet world, for example, if the need is for an Arquillian operation with a secure port, then it is accessed with a operational name.

@ArquillianResource
@OperateOnDeployment("webapp-https") URL urlHttps;

and used like ...

portnum = urlHttps.getPort();
p.setProperty("securedWebServicePort", Integer.toString(portnum));

Which would be needed in only one websocket test class ...
websocket/src/main/java/com/sun/ts/tests/websocket/platform/jakarta/websocket/server/handshakerequest/authenticatedssl/WSCClientIT.java

@markt-asf
Copy link
Contributor

Some feedback from testing this with Tomcat.

First the big plus in the improved speed. Tests execute much faster with the entire suite complete in just over 3 minutes.

It took a little while to figure out how to configure the TCK to run with Tomcat but now I have figured that it it will be easy to maintain going forward and to potentially feed into CI systems to run the TCK more regularly.

I am currently seeing a small number (three) of test failures. I need to look into these to figure out if it is my configuration of the test suite that is still not quite right or if something else is going on.

@markt-asf
Copy link
Contributor

There does appear to be a concurrency issue with the refactored tests.

A number of the tests define private static StringBuffer receivedMessageString which is then used to log messages at key points thoughout the test which are then validated at the end of the test. The Maven Failsafe plugin runs all the tests in a single JVM by default and that appeared to be causing issues with the validation of receivedMessageString.

I'm not sure what the "right" fix is but configuring the tests to run with <reuseForks>false</reuseForks> fixed those failures. The downside is that the tests run in just over 5 minutes rather than just over 3 minutes (which is still much faster than the old TCK). The "right" fix might be breaking up those tests into separate classes each with their own private static StringBuffer receivedMessageString so they don't interfere.

I'm still seeing a couple of timeout tests fail but at the moment I think this is a Tomcat configuration issue.

@markt-asf
Copy link
Contributor

It isn't a Tomcat issue. The root cause is the same as the other failures. The problem is that setTimeout2Test sees the message TCKTestServer second message after sleep generated by setTimeout1Test and fails. My quick fix was to use unique messages for the two timeout tests.

That has resolved the failures I was seeing but it looks like one of the timeout tests is now failing further on. I'm still investigating.

@markt-asf
Copy link
Contributor

Woot! The remaining issues were configuration (I'd forgotten a system property needed to ensure the timeouts fire promptly enough for the tests) and a Tomact bug caused by a recent refactoring. All 736 tests are now passing which is consistent with the old TCK tests that had 737 tests (including the signature test which hasn't been refactored yet).

@olamy
Copy link
Contributor

olamy commented Jun 27, 2023

@gurunrao please read my comment here #1182 (comment)
maybe we can have something which could make everybody happy?

@gurunrao
Copy link
Contributor Author

gurunrao commented Jul 11, 2023

gurunrao#8 - doesn't take care of secure ports

The example of gurunrao#8 shows how it works with a default deployment. Please refer latest code.

	@ArquillianResource
	@OperateOnDeployment("_DEFAULT_")
	public URL url;

Take the Servlet world, for example, if the need is for an Arquillian operation with a secure port, then it is accessed with a operational name.

@ArquillianResource
@OperateOnDeployment("webapp-https") URL urlHttps;

and used like ...

portnum = urlHttps.getPort();
p.setProperty("securedWebServicePort", Integer.toString(portnum));

Which would be needed in only one websocket test class ... websocket/src/main/java/com/sun/ts/tests/websocket/platform/jakarta/websocket/server/handshakerequest/authenticatedssl/WSCClientIT.java

I have made necessary changes as per comments, the ssl port auto-detection is not getting detected.

@gurunrao
Copy link
Contributor Author

gurunrao#8 - doesn't take care of secure ports

The example of gurunrao#8 shows how it works with a default deployment. Please refer latest code.

	@ArquillianResource
	@OperateOnDeployment("_DEFAULT_")
	public URL url;

Take the Servlet world, for example, if the need is for an Arquillian operation with a secure port, then it is accessed with a operational name.

@ArquillianResource
@OperateOnDeployment("webapp-https") URL urlHttps;

and used like ...

portnum = urlHttps.getPort();
p.setProperty("securedWebServicePort", Integer.toString(portnum));

Which would be needed in only one websocket test class ... websocket/src/main/java/com/sun/ts/tests/websocket/platform/jakarta/websocket/server/handshakerequest/authenticatedssl/WSCClientIT.java

I have made necessary changes as per comments, the ssl port auto-detection is not getting detected.

@olamy @joakime any suggestions on SSL port auto-detection working?

@olamy
Copy link
Contributor

olamy commented Jul 14, 2023

@gurunrao I'm on holidays. I can have a look next week.
This is how we do for Jetty https://github.com/jetty-project/servlet-tck-run/blob/jetty-12-ee10/src/test/resources/arquillian.xml

as you can see there is the default

<container qualifier="http" default="true">

and then the https one

<container qualifier="https">

https here it's just an id in the arquillian configuration.

And you reference this as is in your test.

@scottmarlow
Copy link
Contributor

We should consider asking on Websocket dev mailing list if the Standalone WebSocket TCK tests should move to the WebSocket spec. I see that Mark Thomas is here helping already but its not too early to discuss moving the WebSocket TCK work to the Specification.

We started that conversation recently on the Servlet mailing list.

@gurunrao
Copy link
Contributor Author

@gurunrao I'm on holidays. I can have a look next week. This is how we do for Jetty https://github.com/jetty-project/servlet-tck-run/blob/jetty-12-ee10/src/test/resources/arquillian.xml

as you can see there is the default

<container qualifier="http" default="true">

and then the https one

<container qualifier="https">

https here it's just an id in the arquillian configuration.

And you reference this as is in your test.

https://github.com/gurunrao/jakartaee-tck/blob/tck-refactor-websocket-2/glassfish-runner/websocket-platform-tck/src/test/resources/arquillian.xml#L10 has details on container.

@gurunrao
Copy link
Contributor Author

gurunrao commented Jul 31, 2023

if there are no new review comments,I am planning to merge the PR.

@gurunrao
Copy link
Contributor Author

gurunrao commented Jul 31, 2023

@gurunrao I'm on holidays. I can have a look next week. This is how we do for Jetty https://github.com/jetty-project/servlet-tck-run/blob/jetty-12-ee10/src/test/resources/arquillian.xml
as you can see there is the default

<container qualifier="http" default="true">

and then the https one

<container qualifier="https">

https here it's just an id in the arquillian configuration.
And you reference this as is in your test.

https://github.com/gurunrao/jakartaee-tck/blob/tck-refactor-websocket-2/glassfish-runner/websocket-platform-tck/src/test/resources/arquillian.xml#L10 has details on container.

I will not debug the auto detection of SSL further, Since we have system properties and @olamy suggested SSL port auto-detection might be issue with glassfish arquillian.

@gurunrao gurunrao merged commit 365d073 into jakartaee:tckrefactor Jul 31, 2023
1 check failed
@gurunrao gurunrao deleted the tck-refactor-websocket-2 branch April 17, 2024 03:18
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.

5 participants