[Patch 0/5] Add exclusions for tests requiring cvs where cvs is not installed

[Patch 0/5] Add exclusions for tests requiring cvs where cvs is not installed

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

t9600 to t9604 currently depend on cvs to function correctly, otherwise
all of those tests fail. This patch follows an existing pattern of
from the t9400 series by attempting to run cvs without arguments,
which succeeds if installed, and skipping the test if the command
fails.

Randall S. Becker (5):
  t9600-cvsimport: exclude test if cvs is not installed
  t9601-cvsimport-vendor-branch: exclude test if cvs is not installed
  t9602-cvsimport-branches-tags: exclude test if cvs is not installed
  t9603-cvsimport-patchsets: exclude test if cvs is not installed
  t9604-cvsimport-timestamps: exclude test if cvs is not installed

 t/t9600-cvsimport.sh               | 7 +++++++
 t/t9601-cvsimport-vendor-branch.sh | 8 ++++++++
 t/t9602-cvsimport-branches-tags.sh | 7 +++++++
 t/t9603-cvsimport-patchsets.sh     | 7 +++++++
 t/t9604-cvsimport-timestamps.sh    | 7 +++++++
 5 files changed, 36 insertions(+)

-- 
2.22.0

[Patch 1/5] t9600-cvsimport: exclude test if cvs is not installed

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

The t9600-cvsimport test requires the cvs package to be installed on
the system on which the test is being run. The test will fail if cvs
is not installed. The patch checks that cvs is installed by running
the object without arguments, which should complete successfully if
available.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 t/t9600-cvsimport.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
index 251fdd66c4..d6bf38918b 100755
--- a/t/t9600-cvsimport.sh
+++ b/t/t9600-cvsimport.sh
@@ -3,6 +3,13 @@
 test_description='git cvsimport basic tests'
 . ./lib-cvs.sh
 
+cvs >/dev/null 2>&1
+if test $? -ne 1
+then
+	skip_all='skipping git-cvsimport tests, cvs not found'
+	test_done
+fi
+
 if ! test_have_prereq NOT_ROOT; then
 	skip_all='When cvs is compiled with CVS_BADROOT commits as root fail'
 	test_done
-- 
2.22.0

[Patch 2/5] t9601-cvsimport-vendor-branch: exclude test if cvs is not installed

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

The t9601-cvsimport-vendor-branch test requires the cvs package to
be installed on the system on which the test is being run. The test
will fail if cvs is not installed. The patch checks that cvs is
installed by running the object without arguments, which should
complete successfully if available.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 t/t9601-cvsimport-vendor-branch.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t9601-cvsimport-vendor-branch.sh b/t/t9601-cvsimport-vendor-branch.sh
index 827d39f5bf..a473f07d2d 100755
--- a/t/t9601-cvsimport-vendor-branch.sh
+++ b/t/t9601-cvsimport-vendor-branch.sh
@@ -32,8 +32,16 @@
 #       tag has been removed.
 
 test_description='git cvsimport handling of vendor branches'
+
 . ./lib-cvs.sh
 
+cvs >/dev/null 2>&1
+if test $? -ne 1
+then
+	skip_all='skipping git-cvsimport tests, cvs not found'
+	test_done
+fi
+
 setup_cvs_test_repository t9601
 
 test_expect_success PERL 'import a module with a vendor branch' '
-- 
2.22.0

[Patch 3/5] t9602-cvsimport-branches-tags: exclude test if cvs is not installed

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

The t9602-cvsimport-branches-tags test requires the cvs package to
be installed on the system on which the test is being run. The test
will fail if cvs is not installed. The patch checks that cvs is
installed by running the object without arguments, which should
complete successfully if available.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 t/t9602-cvsimport-branches-tags.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t9602-cvsimport-branches-tags.sh b/t/t9602-cvsimport-branches-tags.sh
index e1db323f54..52e8507725 100755
--- a/t/t9602-cvsimport-branches-tags.sh
+++ b/t/t9602-cvsimport-branches-tags.sh
@@ -6,6 +6,13 @@
 test_description='git cvsimport handling of branches and tags'
 . ./lib-cvs.sh
 
+cvs >/dev/null 2>&1
+if test $? -ne 1
+then
+	skip_all='skipping git-cvsimport tests, cvs not found'
+	test_done
+fi
+
 setup_cvs_test_repository t9602
 
 test_expect_success PERL 'import module' '
-- 
2.22.0

[Patch 4/5] t9603-cvsimport-patchsets: exclude test if cvs is not installed

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

The t9603-cvsimport-patchsets test requires the cvs package to
be installed on the system on which the test is being run. The test
will fail if cvs is not installed. The patch checks that cvs is
installed by running the object without arguments, which should
complete successfully if available.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 t/t9603-cvsimport-patchsets.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t9603-cvsimport-patchsets.sh b/t/t9603-cvsimport-patchsets.sh
index 3e64b11eac..354cd66400 100755
--- a/t/t9603-cvsimport-patchsets.sh
+++ b/t/t9603-cvsimport-patchsets.sh
@@ -14,6 +14,13 @@
 test_description='git cvsimport testing for correct patchset estimation'
 . ./lib-cvs.sh
 
+cvs >/dev/null 2>&1
+if test $? -ne 1
+then
+	skip_all='skipping git-cvsimport tests, cvs not found'
+	test_done
+fi
+
 setup_cvs_test_repository t9603
 
 test_expect_failure PERL 'import with criss cross times on revisions' '
-- 
2.22.0

[Patch 5/5] t9604-cvsimport-timestamps: exclude test if cvs is not installed

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

The t9604-cvsimport-timestamps test requires the cvs package to
be installed on the system on which the test is being run. The test
will fail if cvs is not installed. The patch checks that cvs is
installed by running the object without arguments, which should
complete successfully if available.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 t/t9604-cvsimport-timestamps.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t9604-cvsimport-timestamps.sh b/t/t9604-cvsimport-timestamps.sh
index 2ff4aa932d..1fbbd179c1 100755
--- a/t/t9604-cvsimport-timestamps.sh
+++ b/t/t9604-cvsimport-timestamps.sh
@@ -3,6 +3,13 @@
 test_description='git cvsimport timestamps'
 . ./lib-cvs.sh
 
+cvs >/dev/null 2>&1
+if test $? -ne 1
+then
+	skip_all='skipping git-cvsimport tests, cvs not found'
+	test_done
+fi
+
 setup_cvs_test_repository t9604
 
 test_expect_success PERL 'check timestamps are UTC (TZ=CST6CDT)' '
-- 
2.22.0

Re: [Patch 0/5] Add exclusions for tests requiring cvs where cvs is not installed

[Jeff King]

On Thu, Jun 13, 2019 at 02:53:08PM -0400, randall.s.becker@rogers.com wrote:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> t9600 to t9604 currently depend on cvs to function correctly, otherwise
> all of those tests fail. This patch follows an existing pattern of
> from the t9400 series by attempting to run cvs without arguments,
> which succeeds if installed, and skipping the test if the command
> fails.

Hrm. I don't have cvs installed, and those tests are properly skipped
for me. That's because they include lib-cvs.sh, which has:

  if ! type cvs >/dev/null 2>&1
  then
          skip_all='skipping cvsimport tests, cvs not found'
          test_done
  fi

Why doesn't that work for you? Does the "type" check not work (e.g., you
have something called "cvs" but it does not behave as we expect)? If so,
then it sounds like we just need to harmonize that with the other
checks.

It also sounds like the t9400 tests could be using lib-cvs to avoid
duplicating logic, though it might need some refactoring (they don't
need cvsps, for example).

-Peff

RE: [Patch 0/5] Add exclusions for tests requiring cvs where cvs is not installed

[Randall S. Becker]

On June 13, 2019 3:07 PM, Peff wrote:
> On Thu, Jun 13, 2019 at 02:53:08PM -0400, randall.s.becker@rogers.com
> wrote:
> 
> > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> >
> > t9600 to t9604 currently depend on cvs to function correctly,
> > otherwise all of those tests fail. This patch follows an existing
> > pattern of from the t9400 series by attempting to run cvs without
> > arguments, which succeeds if installed, and skipping the test if the
> > command fails.
> 
> Hrm. I don't have cvs installed, and those tests are properly skipped for me.
> That's because they include lib-cvs.sh, which has:
> 
>   if ! type cvs >/dev/null 2>&1
>   then
>           skip_all='skipping cvsimport tests, cvs not found'
>           test_done
>   fi
> 
> Why doesn't that work for you? Does the "type" check not work (e.g., you
> have something called "cvs" but it does not behave as we expect)? If so, then
> it sounds like we just need to harmonize that with the other checks.
> 
> It also sounds like the t9400 tests could be using lib-cvs to avoid duplicating
> logic, though it might need some refactoring (they don't need cvsps, for
> example).

The t9400 tests use the same technique as I used - and I mistakenly trusted it. The type check does not fail.

if ! type cvs >/dev/null 2>&1
then
	echo "oops"
fi

does not print "oops". type is reporting $?=0 and a legitimate file in /usr/local/bin/cvs. Confusingly, t9400 skips, but type reports a valid path. I think the test done in the t9400 series is not correct.

cvs >/dev/null 2>&1 on the platform causes $?=255, while a blah >/dev/null 2>&1 reports $?=127.

There is something else going on causing the cvs-related tests to fail that this patch might be hiding. We do have cvsps so I'm now much more confused by the whole thing.

Let's drop this patch for now. I was premature on this patch and need to dig deeper as to what is going on.

Randall

RE: [Patch 0/5] Add exclusions for tests requiring cvs where cvs is not installed

[Randall S. Becker]

On June 13, 2019 3:31 PM, I wrote:
> On June 13, 2019 3:07 PM, Peff wrote:
> > On Thu, Jun 13, 2019 at 02:53:08PM -0400, randall.s.becker@rogers.com
> > wrote:
> >
> > > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> > >
> > > t9600 to t9604 currently depend on cvs to function correctly,
> > > otherwise all of those tests fail. This patch follows an existing
> > > pattern of from the t9400 series by attempting to run cvs without
> > > arguments, which succeeds if installed, and skipping the test if the
> > > command fails.
> >
> > Hrm. I don't have cvs installed, and those tests are properly skipped for me.
> > That's because they include lib-cvs.sh, which has:
> >
> >   if ! type cvs >/dev/null 2>&1
> >   then
> >           skip_all='skipping cvsimport tests, cvs not found'
> >           test_done
> >   fi
> >
> > Why doesn't that work for you? Does the "type" check not work (e.g.,
> > you have something called "cvs" but it does not behave as we expect)?
> > If so, then it sounds like we just need to harmonize that with the other
> checks.
> >
> > It also sounds like the t9400 tests could be using lib-cvs to avoid
> > duplicating logic, though it might need some refactoring (they don't
> > need cvsps, for example).
> 
> The t9400 tests use the same technique as I used - and I mistakenly trusted it.
> The type check does not fail.
> 
> if ! type cvs >/dev/null 2>&1
> then
> 	echo "oops"
> fi
> 
> does not print "oops". type is reporting $?=0 and a legitimate file in
> /usr/local/bin/cvs. Confusingly, t9400 skips, but type reports a valid path. I
> think the test done in the t9400 series is not correct.
> 
> cvs >/dev/null 2>&1 on the platform causes $?=255, while a blah >/dev/null
> 2>&1 reports $?=127.
> 
> There is something else going on causing the cvs-related tests to fail that this
> patch might be hiding. We do have cvsps so I'm now much more confused by
> the whole thing.
> 
> Let's drop this patch for now. I was premature on this patch and need to dig
> deeper as to what is going on.

We do not need the patch. The situation was caused by an old version of CVS (pre 1.11)  that was causing t9600... to fail. The message was buried under --verbose. I ported CVS 1.11.23 and CVS tests are now working. My bad.

Cheers,
Randall