From 074859ea1c191598515a9c15d81fea72bc1567cd Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sat, 19 Apr 2014 22:52:47 +0200 Subject: [PATCH] Acceptance-tests: Fix 'verify: false' bug Sporadic unit tests failures are caused by long withstanding bug in Jcraft SSH library. Despite announcements in the release notes for the versions 0.1.50 and 0.1.51 of this library, claiming that this bug was fixed, that bug still exists and can be easily reproduced. This change introduces an unit test, that reliably reproduces the 'verify: false' bug in one test method (that is running against established SSH port number, so that random port assignments can be eliminated as the reason for the sporadic unit test failures). The workaround introduced in this change is to conditionally use bouncycastle library. Per default bouncycastle library is used. To run unit tests without it, set NO_BOUNCYCASTLE library to any value: NO_BOUNCYCASTLE=1 buck test --all Change-Id: I9d5fdf992082b3658542d84ed9551ac2bf146414 --- Documentation/dev-buck.txt | 25 +++++-- .../acceptance/ssh/JschVerifyFalseBugIT.java | 67 +++++++++++++++++++ gerrit-acceptance-tests/tests.defs | 9 +++ 3 files changed, 97 insertions(+), 4 deletions(-) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/JschVerifyFalseBugIT.java diff --git a/Documentation/dev-buck.txt b/Documentation/dev-buck.txt index 33c4e9f52c..8a45bb43c8 100644 --- a/Documentation/dev-buck.txt +++ b/Documentation/dev-buck.txt @@ -480,13 +480,22 @@ heap size: == Sporadic failures of unit tests -In case unit tests are failing for non obvious reasons, failed tests may need -to be repeated: +Due to implementation of ephemeral port assignment for HTTP and SSH daemons, +(the reason is explained in depth in this thread: +https://groups.google.com/forum/#!topic/repo-discuss/db14RJ59ubs), +unit tests can randomly fail with "BindException: Address already in use". +In this case, failed tests need to be repeated: ---- FAIL 32,0s 7 Passed 1 Failed com.google.gerrit.acceptance.rest.group.AddRemoveGroupMembersIT -FAILURE includeRemoveGroup: verify: false -com.jcraft.jsch.JSchException: verify: false +FAILURE includeRemoveGroup: Cannot bind to localhost:40955 + at com.google.gerrit.sshd.SshDaemon.start(SshDaemon.java:281) + at com.google.gerrit.lifecycle.LifecycleManager.start(LifecycleManager.java:74) + at com.google.gerrit.pgm.Daemon.start(Daemon.java:288) + at com.google.gerrit.acceptance.GerritServer.start(GerritServer.java:81) +[...] +Caused by: java.net.BindException: Address already in use + at sun.nio.ch.Net.bind0(Native Method) ---- Because the test execution results are cached by Buck, they must be removed @@ -516,6 +525,14 @@ An alternative approach is to use a Buck feature: ---- When this option is used, cache is disabled per design and doesn't need to be deleted. +Note: when -f option is used, the whole unit test cache is dropped. As a consequence, +repeating the + +---- +buck test --all +---- + +would re-execute all tests again. GERRIT ------ diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/JschVerifyFalseBugIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/JschVerifyFalseBugIT.java new file mode 100644 index 0000000000..9bbc12597a --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/JschVerifyFalseBugIT.java @@ -0,0 +1,67 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.ssh; + +import static com.google.gerrit.acceptance.GitUtil.cloneProject; +import static com.google.gerrit.acceptance.GitUtil.createProject; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.NoHttpd; + +import org.junit.Assert; +import org.junit.Ignore; +import org.junit.Test; + +import java.util.Collections; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; + +@NoHttpd +// To see this test failing with 'verify: false', at least in the Jcsh 0.1.51 +// remove bouncycastle libs from the classpath, and run: +// buck test //gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh:JschVerifyFalseBugIT +public class JschVerifyFalseBugIT extends AbstractDaemonTest { + + @Test + @Ignore// we know it works now, so let's not clone a project 500 times ;-) + public void test() throws Exception { + test(5); + } + + private void test(int threads) throws InterruptedException, + ExecutionException { + Callable task = new Callable() { + @Override + public Void call() throws Exception { + for (int i = 1; i < 100; i++) { + String p = "p" + i; + createProject(sshSession, p); + cloneProject(sshSession.getUrl() + "/" + p); + } + return null; + } + }; + List> nCopies = Collections.nCopies(threads, task); + List> futures = Executors.newFixedThreadPool(threads) + .invokeAll(nCopies); + for (Future future : futures) { + future.get(); + } + Assert.assertEquals(threads, futures.size()); + } +} diff --git a/gerrit-acceptance-tests/tests.defs b/gerrit-acceptance-tests/tests.defs index 6c5a08766c..13d934dd80 100644 --- a/gerrit-acceptance-tests/tests.defs +++ b/gerrit-acceptance-tests/tests.defs @@ -1,8 +1,17 @@ +# these need as workaround for the 'verify: false' bug in Jcraft SSH library +BOUNCYCASTLE = [ + '//lib/bouncycastle:bcpkix', + '//lib/bouncycastle:bcpg', +] + def acceptance_tests( srcs, deps = [], source_under_test = [], vm_args = ['-Xmx256m']): + from os import environ + if not environ.get('NO_BOUNCYCASTLE'): + deps = BOUNCYCASTLE + deps for j in srcs: java_test( name = j[:-len('.java')],