| Message ID | 20190613185313.16120-1-randall.s.becker@rogers.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add exclusions for tests requiring cvs where cvs is not installed | expand |
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
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
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
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(+)