[PATCH] t7300: mark test with SANITY

[PATCH] t7300: mark test with SANITY

[Stefan Beller]

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(-)

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

Re: [PATCH] t7300: mark test with SANITY

[Jeff King]

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

Re: [PATCH] t7300: mark test with SANITY

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Re: [PATCH] t7300: mark test with SANITY

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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?

Re: [PATCH] t7300: mark test with SANITY

[Jeff King]

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

Re: [PATCH] t7300: mark test with SANITY

[Junio C Hamano]

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.

>   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.

> 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.

Re: [PATCH] t7300: mark test with SANITY

[Jeff King]

On Tue, May 03, 2016 at 02:19:24PM -0700, Junio C Hamano wrote:

> 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