[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty
getLocalPort() returns -1 resulting in build failures
URL:
https://github.com/apache/maven-integration-testing/pull/46#discussion_r315005991
##
File path:
core-it-suite/src/test/java/org/apache/maven/it/MavenITmng2305MultipleProxiesTest.java
##
@@ -71,20 +78,16 @@ public void testit()
String keyPwd = "key-passwd";
Server server = new Server( 0 );
-server.addConnector( newHttpsConnector( storePath, storePwd, keyPwd )
);
+addHttpsConnector( server, storePath, storePwd, keyPwd );
server.setHandler( new RepoHandler() );
server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
{
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
}
-int httpPort = server.getConnectors()[0].getLocalPort();
+int httpPort = ( (NetworkConnector) server.getConnectors()[0]
).getLocalPort();
System.out.println( "Bound server socket to HTTP port " + httpPort );
-int httpsPort = server.getConnectors()[1].getLocalPort();
+int httpsPort = ( (NetworkConnector) server.getConnectors()[1]
).getLocalPort();
Review comment:
@joakime
I am convinced your advice is right. Can you improve these tests and publish
a new PR?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty
getLocalPort() returns -1 resulting in build failures
URL:
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314309460
##
File path:
core-it-suite/src/test/java/org/apache/maven/it/MavenITmng2305MultipleProxiesTest.java
##
@@ -71,20 +78,16 @@ public void testit()
String keyPwd = "key-passwd";
Server server = new Server( 0 );
-server.addConnector( newHttpsConnector( storePath, storePwd, keyPwd )
);
+addHttpsConnector( server, storePath, storePwd, keyPwd );
server.setHandler( new RepoHandler() );
server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
{
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
}
-int httpPort = server.getConnectors()[0].getLocalPort();
+int httpPort = ( (NetworkConnector) server.getConnectors()[0]
).getLocalPort();
System.out.println( "Bound server socket to HTTP port " + httpPort );
-int httpsPort = server.getConnectors()[1].getLocalPort();
+int httpsPort = ( (NetworkConnector) server.getConnectors()[1]
).getLocalPort();
Review comment:
The thing is that we do not set the port, via
`HttpConfiguration.setSecurePort(int)`, because we do not know it. We can only
read both ports as local ports.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures URL: https://github.com/apache/maven-integration-testing/pull/46#discussion_r314307488 ## File path: core-it-suite/src/test/java/org/apache/maven/it/MavenITmng0553SettingsAuthzEncryptionTest.java ## @@ -86,18 +93,13 @@ protected void setUp() handlerList.addHandler( repoHandler ); handlerList.addHandler( new DefaultHandler() ); -server = new Server( 0 ); server.setHandler( handlerList ); server.start(); -while ( !server.isRunning() || !server.isStarted() ) +if ( server.isFailed() ) Review comment: I think the purpose of this is to check server's status in a parallel thread. ok, we can avoid the IF statement. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty
getLocalPort() returns -1 resulting in build failures
URL:
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314301738
##
File path:
core-it-suite/src/test/java/org/apache/maven/it/MavenIT0146InstallerSnapshotNaming.java
##
@@ -59,15 +60,11 @@ protected void setUp()
server = new Server( 0 );
server.setHandler( handlers );
server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
{
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
}
-port = server.getConnectors()[0].getLocalPort();
+port = ( (NetworkConnector) server.getConnectors()[0] ).getLocalPort();
Review comment:
I did not want to make the code too much different from what we had before.
I am glad that the version 9.0.4 works. After I changed the version to 9.2.28 I
got HTTP 405 and really did not want to get new arguments from the devs saying
that there is something which broke new version.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty
getLocalPort() returns -1 resulting in build failures
URL:
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314304344
##
File path:
core-it-suite/src/test/java/org/apache/maven/it/MavenIT0146InstallerSnapshotNaming.java
##
@@ -59,15 +60,11 @@ protected void setUp()
server = new Server( 0 );
server.setHandler( handlers );
server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
{
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
}
-port = server.getConnectors()[0].getLocalPort();
+port = ( (NetworkConnector) server.getConnectors()[0] ).getLocalPort();
Review comment:
@joakime
Do you see more occurrences of this? Notice that we also have two connectors
in few tests using HTTPS.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty
getLocalPort() returns -1 resulting in build failures
URL:
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314304344
##
File path:
core-it-suite/src/test/java/org/apache/maven/it/MavenIT0146InstallerSnapshotNaming.java
##
@@ -59,15 +60,11 @@ protected void setUp()
server = new Server( 0 );
server.setHandler( handlers );
server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
{
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
}
-port = server.getConnectors()[0].getLocalPort();
+port = ( (NetworkConnector) server.getConnectors()[0] ).getLocalPort();
Review comment:
@joakime
Do you see more occurences of this? Notice that we also have two connectors
in few tests using HTTPS.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty
getLocalPort() returns -1 resulting in build failures
URL:
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314302888
##
File path:
core-it-suite/src/test/java/org/apache/maven/it/MavenIT0146InstallerSnapshotNaming.java
##
@@ -59,15 +60,11 @@ protected void setUp()
server = new Server( 0 );
server.setHandler( handlers );
server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
{
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
}
-port = server.getConnectors()[0].getLocalPort();
+port = ( (NetworkConnector) server.getConnectors()[0] ).getLocalPort();
Review comment:
So I would make a new commit as an improvement of this line.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty
getLocalPort() returns -1 resulting in build failures
URL:
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314302466
##
File path:
core-it-suite/src/test/java/org/apache/maven/it/MavenIT0146InstallerSnapshotNaming.java
##
@@ -59,15 +60,11 @@ protected void setUp()
server = new Server( 0 );
server.setHandler( handlers );
server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
{
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
}
-port = server.getConnectors()[0].getLocalPort();
+port = ( (NetworkConnector) server.getConnectors()[0] ).getLocalPort();
Review comment:
We can improve as always but I spent 3 days with these 38 tests and for me
it is always good to have several smaller changes than one big change. With
small changes/commits we can always test older commits if we find some issue.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty
getLocalPort() returns -1 resulting in build failures
URL:
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314302466
##
File path:
core-it-suite/src/test/java/org/apache/maven/it/MavenIT0146InstallerSnapshotNaming.java
##
@@ -59,15 +60,11 @@ protected void setUp()
server = new Server( 0 );
server.setHandler( handlers );
server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
{
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
}
-port = server.getConnectors()[0].getLocalPort();
+port = ( (NetworkConnector) server.getConnectors()[0] ).getLocalPort();
Review comment:
We can improve as always but I spent 3 days with these 38 tests and for me
it is always good to have several smaller changes than one big change. With
small changes/commits we can always find test older commits if we find some
issue.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty
getLocalPort() returns -1 resulting in build failures
URL:
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314301738
##
File path:
core-it-suite/src/test/java/org/apache/maven/it/MavenIT0146InstallerSnapshotNaming.java
##
@@ -59,15 +60,11 @@ protected void setUp()
server = new Server( 0 );
server.setHandler( handlers );
server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
{
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
}
-port = server.getConnectors()[0].getLocalPort();
+port = ( (NetworkConnector) server.getConnectors()[0] ).getLocalPort();
Review comment:
I did not want to make the code too much different from what we had before.I
was glad that the version 9.0.4 works. After I changed the version to 9.2.28 I
got HTTP 405 and really did not want to get new arguments from the devs saying
that there is something which broke new version.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-integration-testing] Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures
Tibor17 commented on a change in pull request #46: [MNG-6731] Jetty
getLocalPort() returns -1 resulting in build failures
URL:
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314300317
##
File path:
core-it-suite/src/test/java/org/apache/maven/it/MavenITmng0553SettingsAuthzEncryptionTest.java
##
@@ -86,18 +93,13 @@ protected void setUp()
handlerList.addHandler( repoHandler );
handlerList.addHandler( new DefaultHandler() );
-server = new Server( 0 );
server.setHandler( handlerList );
server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
Review comment:
because this has a purpose. I want to catch all errors. Not only IOException.
```
@Override
public final void start() throws Exception
{
synchronized (_lock)
{
try
{
if (_state == __STARTED || _state == __STARTING)
return;
setStarting();
doStart();
setStarted();
}
catch (Throwable e)
{
setFailed(e);
throw e;
}
}
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
