threads / patch / 42209

patcht7300: mark test with SANITY

Subject: [PATCH] t7300: mark test with SANITY

## tl;dr

7 messages between May 3, 2016 and May 3, 2016. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Stefan Beller· May 3, 2016, 18:54 UTC · lore

As the test runs `chmod 0` on a file, we don't want to run that test as root.

Reported-by: Jan Keromnes <janx@linux.com>
Fix-proposed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7300-clean.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7300-clean.sh +1 −1
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 86ceb38..b89fd2a 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -495,7 +495,7 @@ test_expect_success 'should not clean submodules' '
 	test_path_is_missing to_clean
 '
 
-test_expect_success POSIXPERM 'should avoid cleaning possible submodules' '
+test_expect_success POSIXPERM,SANITY 'should avoid cleaning possible submodules' '
 	rm -fr to_clean possible_sub1 &&
 	mkdir to_clean possible_sub1 &&
 	test_when_finished "rm -rf possible_sub*" &&
-- 
2.8.0.37.gb114fff.dirty
Jeff King· May 3, 2016, 19:04 UTC · re: Stefan Beller · lore

Re: [PATCH] t7300: mark test with SANITY

On Tue, May 03, 2016 at 11:54:32AM -0700, Stefan Beller wrote:
> As the test runs `chmod 0` on a file, we don't want to run that test
> as root.

This somehow misses the root (no pun intended) of the issue, to me. Perhaps:

  We `chmod 0` a file and test a case where git is unable to read it.
  If the test is run as root, the permissions are ignored, and our
  simulated read failure does not happen.
The patch itself looks obviously correct. :)
-Peff
Junio C Hamano· May 3, 2016, 19:09 UTC · re: Jeff King · lore

Re: [PATCH] t7300: mark test with SANITY

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> On Tue, May 03, 2016 at 11:54:32AM -0700, Stefan Beller wrote:
>
>> As the test runs `chmod 0` on a file, we don't want to run that test
>> as root.
>
> This somehow misses the root (no pun intended) of the issue, to me.
> Perhaps:
>
>   We `chmod 0` a file and test a case where git is unable to read it.
>   If the test is run as root, the permissions are ignored, and our
>   simulated read failure does not happen.
>
> The patch itself looks obviously correct. :)
>
> -Peff
Yup, I had exactly the same reaction.
Thanks.
Junio C Hamano· May 3, 2016, 19:28 UTC · re: Jeff King · lore

Re: [PATCH] t7300: mark test with SANITY

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> On Tue, May 03, 2016 at 11:54:32AM -0700, Stefan Beller wrote:
>
>> As the test runs `chmod 0` on a file, we don't want to run that test
>> as root.
>
> This somehow misses the root (no pun intended) of the issue, to me.
> Perhaps:
>
>   We `chmod 0` a file and test a case where git is unable to read it.
>   If the test is run as root, the permissions are ignored, and our
>   simulated read failure does not happen.
>
> The patch itself looks obviously correct. :)
>
> -Peff

By the way, it is easy to make a mistake like this, not to notice it during a review, and to leave it unnoticed for a long time, especially because I do not think anybody active in the development community runs tests as 'root'.

Perhaps in a future update, Travis should learn a step to catch breakages like like this. Or perhaps it is not worth the effort?

Jeff King· May 3, 2016, 21:15 UTC · re: Junio C Hamano · lore

Re: [PATCH] t7300: mark test with SANITY

On Tue, May 03, 2016 at 12:28:12PM -0700, Junio C Hamano wrote:
> By the way, it is easy to make a mistake like this, not to notice it
> during a review, and to leave it unnoticed for a long time,
> especially because I do not think anybody active in the development
> community runs tests as 'root'.

I think the advice for running the tests as root has long been: don't. But I guess some people are in situations where it's hard to do otherwise.

I'm torn on whether it really matters or not. On the one hand, if a breakage that is purely in a test and not in git itself goes for a long time without anyone noticing, it's not really hurting anyone. On the other, "noticing" and "reporting" are two different things; there may be people who run "make test" and give up.

FWIW, I just did a "sudo make test" and the only other failures were around HTTP (which correctly punts with "you can't run http tests as root", but I set GIT_TEST_HTTPD=1, which turns skipping them into an error).

> Perhaps in a future update, Travis should learn a step to catch
> breakages like like this.  Or perhaps it is not worth the effort?

Maybe. I admit to not really using the Travis tests myself, as they are way too slow and cumbersome to debug compared to just running "make test". The primary value to me of centralized CI is:

  1. _If_ people are looking at PRs on GitHub, the test status is shown
     right there in the PR, without a reviewer having to wonder whether
     the submitter ran "make test". But since I don't ever look at PRs
     for Git, that's not helpful.
  2. Quicker testing on a variety of platforms that I don't have.
     We've known for years that the tip of master passes your "make
     test" under Linux, but other-platform breakage slips through, and
     we rely on people who use those platforms to report. But that may
     not happen in a timely way, and CI can let us know sooner.
     Right now we've taken a baby step there. CI auto-builds for various
     setups, but we still rely on people who care about those platforms
     to look at the results and surface the data to the list. If we
     tested individual topics picked up from the mailing list and sent
     out an email saying "by the way, this is broken on platform X",
     that would go a long way (and would help point 1, too). I think
     there was some discussion in the 0day thread elsewhere on the list,
     but I didn't follow it too closely.

Anyway, back to the original question. I do think "test as root" can be considered another platform, which makes it a good match for CI. But at the same time, I don't know that it has ever surfaced an actual bug in _Git_, and not just the test suite. So seeing those bugs quickly is a lot less interesting.

-Peff
Junio C Hamano· May 3, 2016, 21:19 UTC · re: Jeff King · lore

Re: [PATCH] t7300: mark test with SANITY

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> Maybe. I admit to not really using the Travis tests myself, as they are
> way too slow and cumbersome to debug compared to just running "make
> test".  The primary value to me of centralized CI is:
>
>   1. _If_ people are looking at PRs on GitHub, the test status is shown
>      right there in the PR, without a reviewer having to wonder whether
>      the submitter ran "make test". But since I don't ever look at PRs
>      for Git, that's not helpful.

What I was hoping was that bots like SubmitGit could look at that status.

Show 5 quoted lines
>   2. Quicker testing on a variety of platforms that I don't have.
>
>      ... I think
>      there was some discussion in the 0day thread elsewhere on the list,
>      but I didn't follow it too closely.
Yes, I'd love to see it happen some day.
Show 5 quoted lines
> Anyway, back to the original question. I do think "test as root" can be
> considered another platform, which makes it a good match for CI. But at
> the same time, I don't know that it has ever surfaced an actual bug in
> _Git_, and not just the test suite. So seeing those bugs quickly is a
> lot less interesting.
True.
Jeff King· May 3, 2016, 21:35 UTC · re: Junio C Hamano · lore

Re: [PATCH] t7300: mark test with SANITY

On Tue, May 03, 2016 at 02:19:24PM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > Maybe. I admit to not really using the Travis tests myself, as they are
> > way too slow and cumbersome to debug compared to just running "make
> > test".  The primary value to me of centralized CI is:
> >
> >   1. _If_ people are looking at PRs on GitHub, the test status is shown
> >      right there in the PR, without a reviewer having to wonder whether
> >      the submitter ran "make test". But since I don't ever look at PRs
> >      for Git, that's not helpful.
> 
> What I was hoping was that bots like SubmitGit could look at that
> status.

Yeah, I think that would be pretty trivial to do. It's already interacting with GitHub's API, and I think there's a simple call to query the test status (so it wouldn't even require SubmitGit talking to Travis directly).

I don't think that really solves the problem overall, though. SubmitGit is still a minority of patch submissions (and I wouldn't expect that to change, but maybe I'm just a curmudgeon).

-Peff

← back to recent threads