{"thread":{"id":"42209","subject":"[PATCH] t7300: mark test with SANITY","startedAt":"2016-05-03T18:54:32Z","lastAt":"2016-05-03T21:35:56Z","messageCount":7,"participants":["Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"285357","messageId":"1462301672-20866-1-git-send-email-sbeller@google.com","threadId":"42209","inReplyTo":null,"subject":"[PATCH] t7300: mark test with SANITY","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-03T18:54:32Z","receivedAt":"2016-05-03T18:54:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As the test runs `chmod 0` on a file, we don't want to run that test\nas root.\n\nReported-by: Jan Keromnes \nFix-proposed-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n t/t7300-clean.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 86ceb38..b89fd2a 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -495,7 +495,7 @@ test_expect_success 'should not clean submodules' '\n \ttest_path_is_missing to_clean\n '\n \n-test_expect_success POSIXPERM 'should avoid cleaning possible submodules' '\n+test_expect_success POSIXPERM,SANITY 'should avoid cleaning possible submodules' '\n \trm -fr to_clean possible_sub1 &&\n \tmkdir to_clean possible_sub1 &&\n \ttest_when_finished \"rm -rf possible_sub*\" &&\n-- \n2.8.0.37.gb114fff.dirty\n"},{"id":"285360","messageId":"20160503190417.GD30530@sigill.intra.peff.net","threadId":"42209","inReplyTo":"1462301672-20866-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] t7300: mark test with SANITY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-03T19:04:17Z","receivedAt":"2016-05-03T19:04:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 03, 2016 at 11:54:32AM -0700, Stefan Beller wrote:\n\n> As the test runs `chmod 0` on a file, we don't want to run that test\n> as root.\n\nThis somehow misses the root (no pun intended) of the issue, to me.\nPerhaps:\n\n We `chmod 0` a file and test a case where git is unable to read it.\n If the test is run as root, the permissions are ignored, and our\n simulated read failure does not happen.\n\nThe patch itself looks obviously correct. :)\n\n-Peff\n"},{"id":"285362","messageId":"xmqqvb2vm00a.fsf@gitster.mtv.corp.google.com","threadId":"42209","inReplyTo":"20160503190417.GD30530@sigill.intra.peff.net","subject":"Re: [PATCH] t7300: mark test with SANITY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T19:09:41Z","receivedAt":"2016-05-03T19:09:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, May 03, 2016 at 11:54:32AM -0700, Stefan Beller wrote:\n>\n>> As the test runs `chmod 0` on a file, we don't want to run that test\n>> as root.\n>\n> This somehow misses the root (no pun intended) of the issue, to me.\n> Perhaps:\n>\n> We `chmod 0` a file and test a case where git is unable to read it.\n> If the test is run as root, the permissions are ignored, and our\n> simulated read failure does not happen.\n>\n> The patch itself looks obviously correct. :)\n>\n> -Peff\n\nYup, I had exactly the same reaction.\n\nThanks.\n"},{"id":"285365","messageId":"xmqqmvo7lz5f.fsf@gitster.mtv.corp.google.com","threadId":"42209","inReplyTo":"20160503190417.GD30530@sigill.intra.peff.net","subject":"Re: [PATCH] t7300: mark test with SANITY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T19:28:12Z","receivedAt":"2016-05-03T19:28:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, May 03, 2016 at 11:54:32AM -0700, Stefan Beller wrote:\n>\n>> As the test runs `chmod 0` on a file, we don't want to run that test\n>> as root.\n>\n> This somehow misses the root (no pun intended) of the issue, to me.\n> Perhaps:\n>\n> We `chmod 0` a file and test a case where git is unable to read it.\n> If the test is run as root, the permissions are ignored, and our\n> simulated read failure does not happen.\n>\n> The patch itself looks obviously correct. :)\n>\n> -Peff\n\nBy the way, it is easy to make a mistake like this, not to notice it\nduring a review, and to leave it unnoticed for a long time,\nespecially because I do not think anybody active in the development\ncommunity runs tests as 'root'.\n\nPerhaps in a future update, Travis should learn a step to catch\nbreakages like like this. Or perhaps it is not worth the effort?\n"},{"id":"285378","messageId":"20160503211528.GA32737@sigill.intra.peff.net","threadId":"42209","inReplyTo":"xmqqmvo7lz5f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t7300: mark test with SANITY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-03T21:15:29Z","receivedAt":"2016-05-03T21:15:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 03, 2016 at 12:28:12PM -0700, Junio C Hamano wrote:\n\n> By the way, it is easy to make a mistake like this, not to notice it\n> during a review, and to leave it unnoticed for a long time,\n> especially because I do not think anybody active in the development\n> community runs tests as 'root'.\n\nI think the advice for running the tests as root has long been: don't.\nBut I guess some people are in situations where it's hard to do\notherwise.\n\nI'm torn on whether it really matters or not. On the one hand, if a\nbreakage that is purely in a test and not in git itself goes for a long\ntime without anyone noticing, it's not really hurting anyone. On the\nother, \"noticing\" and \"reporting\" are two different things; there may be\npeople who run \"make test\" and give up.\n\nFWIW, I just did a \"sudo make test\" and the only other failures were\naround HTTP (which correctly punts with \"you can't run http tests as\nroot\", but I set GIT_TEST_HTTPD=1, which turns skipping them into an\nerror).\n\n> Perhaps in a future update, Travis should learn a step to catch\n> breakages like like this. Or perhaps it is not worth the effort?\n\nMaybe. I admit to not really using the Travis tests myself, as they are\nway too slow and cumbersome to debug compared to just running \"make\ntest\". The primary value to me of centralized CI is:\n\n 1. _If_ people are looking at PRs on GitHub, the test status is shown\n right there in the PR, without a reviewer having to wonder whether\n the submitter ran \"make test\". But since I don't ever look at PRs\n for Git, that's not helpful.\n\n 2. Quicker testing on a variety of platforms that I don't have.\n\n We've known for years that the tip of master passes your \"make\n test\" under Linux, but other-platform breakage slips through, and\n we rely on people who use those platforms to report. But that may\n not happen in a timely way, and CI can let us know sooner.\n\n Right now we've taken a baby step there. CI auto-builds for various\n setups, but we still rely on people who care about those platforms\n to look at the results and surface the data to the list. If we\n tested individual topics picked up from the mailing list and sent\n out an email saying \"by the way, this is broken on platform X\",\n that would go a long way (and would help point 1, too). I think\n there was some discussion in the 0day thread elsewhere on the list,\n but I didn't follow it too closely.\n\nAnyway, back to the original question. I do think \"test as root\" can be\nconsidered another platform, which makes it a good match for CI. But at\nthe same time, I don't know that it has ever surfaced an actual bug in\n_Git_, and not just the test suite. So seeing those bugs quickly is a\nlot less interesting.\n\n-Peff\n"},{"id":"285379","messageId":"xmqqk2jalu03.fsf@gitster.mtv.corp.google.com","threadId":"42209","inReplyTo":"20160503211528.GA32737@sigill.intra.peff.net","subject":"Re: [PATCH] t7300: mark test with SANITY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T21:19:24Z","receivedAt":"2016-05-03T21:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Maybe. I admit to not really using the Travis tests myself, as they are\n> way too slow and cumbersome to debug compared to just running \"make\n> test\". The primary value to me of centralized CI is:\n>\n> 1. _If_ people are looking at PRs on GitHub, the test status is shown\n> right there in the PR, without a reviewer having to wonder whether\n> the submitter ran \"make test\". But since I don't ever look at PRs\n> for Git, that's not helpful.\n\nWhat I was hoping was that bots like SubmitGit could look at that\nstatus.\n\n> 2. Quicker testing on a variety of platforms that I don't have.\n>\n> ... I think\n> there was some discussion in the 0day thread elsewhere on the list,\n> but I didn't follow it too closely.\n\nYes, I'd love to see it happen some day.\n\n> Anyway, back to the original question. I do think \"test as root\" can be\n> considered another platform, which makes it a good match for CI. But at\n> the same time, I don't know that it has ever surfaced an actual bug in\n> _Git_, and not just the test suite. So seeing those bugs quickly is a\n> lot less interesting.\n\nTrue.\n"},{"id":"285384","messageId":"20160503213556.GA25133@sigill.intra.peff.net","threadId":"42209","inReplyTo":"xmqqk2jalu03.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t7300: mark test with SANITY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-03T21:35:56Z","receivedAt":"2016-05-03T21:35:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 03, 2016 at 02:19:24PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Maybe. I admit to not really using the Travis tests myself, as they are\n> > way too slow and cumbersome to debug compared to just running \"make\n> > test\". The primary value to me of centralized CI is:\n> >\n> > 1. _If_ people are looking at PRs on GitHub, the test status is shown\n> > right there in the PR, without a reviewer having to wonder whether\n> > the submitter ran \"make test\". But since I don't ever look at PRs\n> > for Git, that's not helpful.\n> \n> What I was hoping was that bots like SubmitGit could look at that\n> status.\n\nYeah, I think that would be pretty trivial to do. It's already\ninteracting with GitHub's API, and I think there's a simple call to\nquery the test status (so it wouldn't even require SubmitGit talking to\nTravis directly).\n\nI don't think that really solves the problem overall, though. SubmitGit\nis still a minority of patch submissions (and I wouldn't expect that to\nchange, but maybe I'm just a curmudgeon).\n\n-Peff\n"}]}