Re: [1/2] maven-wagon git commit: [WAGON-474] Upgrade and revise all tests for Jetty 8

[Olivier Lamy]

Hi
What about at least Jetty 9.2? (8.x is not anymore supported)


On 26 December 2016 at 10:58,  wrote:

> Repository: maven-wagon
> Updated Branches:
>   refs/heads/jetty-8 [created] e707a2691
>
>
> [WAGON-474] Upgrade and revise all tests for Jetty 8
>
> * Upgrade all test code to Jetty 8.1.22 and Servlet 3.0
> * Unify variable names in redirect usecases to realServer and
>   redirectServer
> * RedirectHandler: redirect code is passed but completely ignored because
>   sendRedirect() always sends 302
> * Chronologically sort checkHandlerResult() calls
> ** Set redirect code (See Other (303)) as requested
> ** Update checkHandlerResult() for requested status codes rather sent
>chosen by server (mismatched previously)
> * testPreemptiveAuthentication*(): properly check for OK for GET and
>   CREATED for PUT instead of OK only for both
> * WebDavWagonTest: replace status code literal for
>   HttpServletResponse.SC_* for better readability
> * testRedirect*(): add more checkHandlerResults
>
>
> Project: http://git-wip-us.apache.org/repos/asf/maven-wagon/repo
> Commit: http://git-wip-us.apache.org/repos/asf/maven-wagon/commit/2e1c807e
> Tree: http://git-wip-us.apache.org/repos/asf/maven-wagon/tree/2e1c807e
> Diff: http://git-wip-us.apache.org/repos/asf/maven-wagon/diff/2e1c807e
>
> Branch: refs/heads/jetty-8
> Commit: 2e1c807e5345a1999376771f8b378b8cd3328fee
> Parents: b451e41
> Author: Michael Osipov 
> Authored: Mon Dec 26 00:55:09 2016 +0100
> Committer: Michael Osipov 
> Committed: Mon Dec 26 00:55:09 2016 +0100
>
> --
>  pom.xml |  14 +-
>  wagon-provider-test/pom.xml |   8 +-
>  .../maven/wagon/http/HttpWagonTestCase.java | 273 ++-
>  wagon-providers/wagon-http-lightweight/pom.xml  |   4 +-
>  .../http/LightweightHttpsWagonTest.java |   6 +-
>  wagon-providers/wagon-http/pom.xml  |   8 +-
>  .../http/HttpWagonHttpServerTestCase.java   |  14 +-
>  .../http/HttpWagonReasonPhraseTest.java |   2 +-
>  .../providers/http/HttpWagonTimeoutTest.java|   2 +-
>  .../http/HttpsWagonPreemptiveTest.java  |   6 +-
>  .../wagon/providers/http/HttpsWagonTest.java|   6 +-
>  .../providers/http/HugeFileDownloadTest.java|  14 +-
>  wagon-providers/wagon-ssh/pom.xml   |   8 +-
>  .../ssh/jsch/ScpWagonWithProxyTest.java |  16 +-
>  wagon-providers/wagon-webdav-jackrabbit/pom.xml |   8 +-
>  .../wagon/providers/webdav/WebDavWagonTest.java |  73 ++---
>  .../providers/webdav/WebDavsWagonTest.java  |   6 +-
>  wagon-tcks/wagon-tck-http/pom.xml   |  12 +-
>  .../wagon/tck/http/fixture/ServerFixture.java   |  52 ++--
>  19 files changed, 278 insertions(+), 254 deletions(-)
> --
>
>
> http://git-wip-us.apache.org/repos/asf/maven-wagon/blob/2e1c807e/pom.xml
> --
> diff --git a/pom.xml b/pom.xml
> index e127da4..a90a1ba 100644
> --- a/pom.xml
> +++ b/pom.xml
> @@ -313,14 +313,14 @@ under the License.
>
>
>
> -org.mortbay.jetty
> -jetty
> -6.1.26
> +org.eclipse.jetty.aggregate
> +jetty-all
> +8.1.22.v20160922
>
>
> -org.mortbay.jetty
> -servlet-api
> -2.5-20081211
> +javax.servlet
> +javax.servlet-api
> +3.0.1
>  
>  
>
> @@ -565,5 +565,5 @@ under the License.
>
>  
>
> -
> +
>  
>
> http://git-wip-us.apache.org/repos/asf/maven-wagon/blob/
> 2e1c807e/wagon-provider-test/pom.xml
> --
> diff --git a/wagon-provider-test/pom.xml b/wagon-provider-test/pom.xml
> index b19e7ac..9b0f4e5 100644
> --- a/wagon-provider-test/pom.xml
> +++ b/wagon-provider-test/pom.xml
> @@ -51,12 +51,12 @@ under the License.
>compile
>  
>  
> -  org.mortbay.jetty
> -  jetty
> +  org.eclipse.jetty.aggregate
> +  jetty-all
>  
>  
> -  org.mortbay.jetty
> -  servlet-api
> +  javax.servlet
> +  javax.servlet-api
>  
>  
>org.slf4j
>
> http://git-wip-us.apache.org/repos/asf/maven-wagon/blob/
> 2e1c807e/wagon-provider-test/src/main/java/org/apache/maven/wagon/http/
> HttpWagonTestCase.java
> --
> diff --git a/wagon-provider-test/src/main/java/org/apache/maven/
> wagon/http/HttpWagonTestCase.java b/wagon-provider-test/src/
> main/java/org/apache/maven/wagon/http/HttpWagonTestCase.java
> index dfc499e..cf479fd 100644
> --- a/wagon-provider-test/src/main/java/org/apache/maven/
> wagon/http/HttpWagonTestCase.java
> +++ b/wagon-provider-test/src/main/java/org/apache/maven/
> 

Re: [VOTE] Releasing Wagon 2.11

[Dan Tran]

i still see the same timeout error on the jetty8 branch. No issue at master

[ERROR] Failed to execute goal
org.apache.maven.plugins:maven-surefire-plugin:2.18.1:test
(default-test) on project wagon-http: There was a timeout or other
error in the fork -> [Help
1]org.apache.maven.lifecycle.LifecycleExecutionException: Failed to
execute goal org.apache.maven.plugins:maven-surefire-plugin:2.18.1:test
(default-test) on project wagon-http: There was a timeout or other
error in the fork
at 
org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:212)



It is your call now. since I cant release it

-Dan

On Tue, Dec 27, 2016 at 5:05 PM, Michael Osipov  wrote:

> Hi Dan,
>
> Am 2016-12-25 um 19:51 schrieb Dan Tran:
>
>> Just want to confirm,  your FreeBSD and build.apache.org's ubuntu see the
>> same test failure??
>>
>> Do you think we should cancel the vote and wait for your fix?
>>
>
> I was finally able to complete my coding and testings. After several hours
> of testing and debugging, I've found the reasons why several tests were
> failing sporadically, some were bad test conditions (testSecuredGet(),
> testSecuredGetToStream()) and some rooted in bugs in HttpWagon. I'll go in
> detail:
>
> 1. Flaky testSecuredGet(ToStream):
> This failed once in a while due to race conditions between Jetty's qtp
> threads and the main thread running the tests and the assertions. Jetty did
> not fully complete the process chain while Surefire continued to verify the
> result. This yielded to: expected <2> requests found <1> and so forth. I
> added a simple sleep() and it did work. I avoided to have complex sync code
> with CountDownLatch or alike. Some logging added to the code shows the
> following in Surefire's XML output:
>
> 
>>
>
> now the logging:
>
> > [main] DEBUG org.apache.http.impl.conn.DefaultManagedHttpClientConnection
>> - http-outgoing-86: set socket timeout to 180
>> [main] DEBUG org.apache.http.impl.execchain.MainClientExec - Executing
>> request GET /test-secured-resource HTTP/1.1
>> [main] DEBUG org.apache.http.impl.execchain.MainClientExec - Target auth
>> state: CHALLENGED
>> [main] DEBUG org.apache.http.impl.auth.HttpAuthenticator - Generating
>> response to an authentication challenge using basic scheme
>> [main] DEBUG org.apache.http.impl.execchain.MainClientExec - Proxy auth
>> state: UNCHALLENGED
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 >> GET
>> /test-secured-resource HTTP/1.1
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 >> Cache-control:
>> no-cache
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 >> Cache-store:
>> no-store
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 >> Pragma:
>> no-cache
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 >> Expires: 0
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 >>
>> Accept-Encoding: gzip
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 >> Host:
>> localhost:45658
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 >> Connection:
>> Keep-Alive
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 >> User-Agent:
>> Apache-HttpClient/4.5.2 (Java/1.7.0_111)
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 >> Authorization:
>> Basic dXNlcjpzZWNyZXQ=
>> org.apache.maven.wagon.http.HttpWagonTestCase$TestSecurityHandler@9187167
>> (count 1): handled GET /test-secured-resource with status 401
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 << HTTP/1.1 200 OK
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 << Accept-Ranges:
>> bytes
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 <<
>> Content-Length: 10
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 << Last-Modified:
>> Mon, 26 Dec 2016 23:07:55 GMT
>> [main] DEBUG org.apache.http.headers - http-outgoing-86 << Server:
>> Jetty(8.1.22.v20160922)
>> [main] DEBUG org.apache.http.impl.execchain.MainClientExec - Connection
>> can be kept alive indefinitely
>> [main] DEBUG org.apache.http.impl.auth.HttpAuthenticator -
>> Authentication succeeded
>> [main] DEBUG org.apache.http.impl.client.TargetAuthenticationStrategy -
>> Caching 'basic' auth scheme for http://localhost:45658
>> [main] DEBUG org.apache.http.impl.conn.PoolingHttpClientConnectionManager
>> - Connection [id: 86][route: {}->http://localhost:45658] can be kept
>> alive indefinitely
>> [main] DEBUG org.apache.http.impl.conn.PoolingHttpClientConnectionManager
>> - Connection released: [id: 86][route: {}->http://localhost:45658][total
>> kept alive: 40; route allocated: 1 of 20; total allocated: 40 of 40]
>> org.apache.maven.wagon.http.HttpWagonTestCase$TestSecurityHandler@9187167
>> (1)
>> org.apache.maven.wagon.http.HttpWagonTestCase$TestSecurityHandler@9187167
>> (count 2): handled GET /test-secured-resource with status 200
>> [main] INFO org.eclipse.jetty.server.handler.ContextHandler - stopped
>> 

Re: [VOTE] Releasing Wagon 2.11

[Michael Osipov]

Hi Dan,

Am 2016-12-25 um 19:51 schrieb Dan Tran:

Just want to confirm,  your FreeBSD and build.apache.org's ubuntu see the
same test failure??

Do you think we should cancel the vote and wait for your fix?


I was finally able to complete my coding and testings. After several 
hours of testing and debugging, I've found the reasons why several tests 
were failing sporadically, some were bad test conditions 
(testSecuredGet(), testSecuredGetToStream()) and some rooted in bugs in 
HttpWagon. I'll go in detail:


1. Flaky testSecuredGet(ToStream):
This failed once in a while due to race conditions between Jetty's qtp 
threads and the main thread running the tests and the assertions. Jetty 
did not fully complete the process chain while Surefire continued to 
verify the result. This yielded to: expected <2> requests found <1> and 
so forth. I added a simple sleep() and it did work. I avoided to have 
complex sync code with CountDownLatch or alike. Some logging added to 
the code shows the following in Surefire's XML output:






now the logging:





now take a look at TestSecurityHandler output. I added it after 
super.handle() called by Jetty. As you can see, 
"org.apache.maven.wagon.http.HttpWagonTestCase$TestSecurityHandler@9187167 
(1)" is printed where the assertions kick in, after Wagon has completed, 
but the second request is handled afterwards. A traditional race condition.


2. Bugs found:
Several auth tests fail for no reason too once in a while. Logging 
showed me that HttpClient simply did not send any Basic data, regardless 
if preemptive or after a roundtrip. The basic problem was that 
non-threadsafe classes have been used in a non-threadsafe manner as well 
as auth caches have not beed released properly, and preemptive auth was 
configured incorrectly. After fixing the issues one by one, the tests 
never failed again.


I have tested the code on three platforms and six JDK versions tens of 
times, they never failed again. After we have resolved your 
HugeDowloadTest issue and no one else objects, I'd like to merge this 
into master and like you to reroll 2.11 with those changes.


Meanwhile, I'll test this branch with Maven Core IT Suite too.

Michael

-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

Re: [1/2] maven-wagon git commit: [WAGON-474] Upgrade and revise all tests for Jetty 8

[Michael Osipov]

Am 2016-12-26 um 02:56 schrieb Dan Tran:

redhat7/openjdk 7 ( latest)

this test fails on timeout.  I have seen this issue before not lately. Used
to work on  2.9, 2 10, and 2.11)


Please add the following to your Surefire config:

  
true
  
-MM-dd'T'HH:mm:ss.SSS
  
debug
  
error
  
debug
  
debug
  
debug
  
debug
  
debug
  
error


and we will see what the HttpClient does and why it is is failing. I ran 
preliminary tests on FC25 with OpenJDK 8, no issues so far.



-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

Re: svn commit: r1775971 - /maven/plugins/trunk/maven-pmd-plugin/pom.xml

[Robert Scholte]

I will have to analyze that. [1]

When working on Jigsaw I had to make clear what's the difference between  
the two of them:
- both are required on the classpath during compilation, but at runtime  
the provided should already be there ( e.g. servlet-api), optional can be  
there (springboot dependency)


there it is translated like this:
module M {
  requires javax.servlet-api; //it is required both at compile time and at  
runtime, even though it is the container which provides this module
  requires static some.springboot.dep; //required during compiletime,  
unused or discoverable during runtime

}

and about those compile-time annotations like Plugin Annotations: those  
are actually compile-time only.


[1]  
https://cwiki.apache.org/confluence/display/MAVEN/POM+Model+Version+5.0.0


On Tue, 27 Dec 2016 20:07:06 +0100, Christian Schulte   
wrote:



Am 12/27/16 um 14:48 schrieb Robert Scholte:

IMO scopes weren't designed to make transitive dependencies disappear.


Does it mean you would agree that there should be no "provided" scope?
Instead "compile+optional" or "runtime+optional" or "test+optional"
should be used, where "optional" is just another name for "transitive"?
So in model version 5.0.0 we really should rename "optional" to
"transitive"? Would you also agree that the "test" scope should be
transitive by default in model version 5.0.0? So in model version 5.0.0
all scopes are transitive by default and a dependency can be flagged
non-transitive by setting the "transitive" attribute to "false" which
defaults to "true" and there are no optional dependencies any more.
Maybe the dependency element needs a way to carry freetext comments on
them as well so that things can be documented (I flagged this not
transitive, because..., I pulled this in to upgrade transitive
dependency XYZ of ABC..., etc.).


-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org


-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

Re: svn commit: r1775971 - /maven/plugins/trunk/maven-pmd-plugin/pom.xml

[Christian Schulte]

Am 12/27/16 um 14:48 schrieb Robert Scholte:
> IMO scopes weren't designed to make transitive dependencies disappear.

Does it mean you would agree that there should be no "provided" scope?
Instead "compile+optional" or "runtime+optional" or "test+optional"
should be used, where "optional" is just another name for "transitive"?
So in model version 5.0.0 we really should rename "optional" to
"transitive"? Would you also agree that the "test" scope should be
transitive by default in model version 5.0.0? So in model version 5.0.0
all scopes are transitive by default and a dependency can be flagged
non-transitive by setting the "transitive" attribute to "false" which
defaults to "true" and there are no optional dependencies any more.
Maybe the dependency element needs a way to carry freetext comments on
them as well so that things can be documented (I flagged this not
transitive, because..., I pulled this in to upgrade transitive
dependency XYZ of ABC..., etc.).


-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

Re: svn commit: r1775971 - /maven/plugins/trunk/maven-pmd-plugin/pom.xml

[Robert Scholte]

On Mon, 26 Dec 2016 17:07:14 +0100, Christian Schulte   
wrote:



Am 12/26/16 um 11:36 schrieb Robert Scholte:

This is becoming a bug versus feature discussion.


It shouldn't.


Up until now you've made
changes which might change the resolution because you've marked them as  
a

bug which must be fixed. However, what is 'the truth': the documentation
or the code? The answer is: in the end it is the code. And if you want  
to
have them in sync, you sometimes need to adjust the documentation  
instead

of the code, because the code has a behavior one is used to.


Have you read the Javadoc and the code? If you would have done that, you
would notice that everything behaves consistently and as documented
*but* one class which is fixed now. If it would be *all* classes, there
would be no question the code is behaving the way it should.

MRESOLVER-8

This *only* affects plugin and extension resolution by stopping to
discard any test and provided *direct* dependencies of a plugin the same
way optional *direct* dependencies are not discarded and the same way
the dependency manager is not managing *direct* dependencies. It does
not affect project resolution in any way. That's what we are really
lucky about. If we don't want Maven to behave that way (with plugin and
extension resolution fixed) it's the Maven codebase to adjust - not the
resolver. That's just an API used by Maven and should just be consistent
and correct. The original author really could have left a few unit tests
in that area. We would not discuss anything, if he would have done that.
He would have noticed things himself or would have left a comment at
least. The issue above together with

MRESOLVER-9
MRESOLVER-10

is really "just" bugfixes. What we learn from that is that we should
"commit" any resolution result during deployment so that bugs like these
can be fixed without influencing the resolution performed for a deployed
project. That's the PDT file we are going to deploy in Maven X.

Since we're talking about changes in resolution, I also expose this  
topic.

To me it is not a bug nor a feature, but it is a design flaw. And the
issue is often not exposed, because the dependencies used for testing  
are

not conflicting the compile dependencies. As long as the compilations
works and all the tests run, users often don't look in detail at the
dependency.
The fact right now is that if I add/change a test-scoped dependency, it
could happen that the project won't run due to a missing transitive
dependency.
We are very, very lucky this doesn't happen that often.


This is what would stop if we would just fix those bugs. We are running
into those bugs ourselves. Take a look at the PMD plugin POM again. What
would you have done, if the test dependencies I moved to compile scope
would be required for compilation of that project?


With the current behavior I wouldn't include these dependencies at all,  
since they're already available as compile-scoped transitive dependency.  
When due to an upgrade of a dependency commons-io is not used anymore,  
tests will fail to compile. And that's the moment to add this dependency.
IMO changing dependencies should never cause compilation failures caused  
by transitive dependencies, but right now there's simply no better  
solution. We say that it is a best practice to always define the direct  
dependencies, but in this case we can't do that because we cannot give  
these dependencies the test-scope.



This is already
yelling for an enforcer rule or something like that. "Are all classes
used by the classes on the compilation classpath available during
compilation?" Currently it's a result of trial and error. Really. If we
let this go on, we need to be even more lucky in a few years. If we say
plugins and extensions just are not resolved the same way as projects
(how it has been for a long time), this will make the following IT start
to fail, when done consistently.



So we would need to adjust the Maven codebase to keep it behaving as
before. The resolver is not the correct codebase for this. I could do
that easily although it's inconcistent with itself that way. If you take
a look at what updates needed to be performed to various plugin POMs,
those are really all bugs in the POMs. Either we fix them, or we make
plugin resolution differ from project resolution (non-transitive
*direct* dependencies only override main scope dependencies during
building but are ignored when building the runtime classpath). Just say
so and it'll be done. My personal opinion is that having a different
runtime classpath than what was used during building is a bad idea and
we are running into issues due to this ourselves which proves this  
correct.


I agree that compile and runtime classpath should be the same (apart from  
provided/runtime scoped deps of course). Just like test-compile