{"thread":{"id":"50414","subject":"[PATCH 0/1] git-p4: remove ticket expiration test","startedAt":"2019-02-06T15:12:58Z","lastAt":"2019-02-08T10:15:26Z","messageCount":6,"participants":["Luke Diamand","SZEDER Gábor","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"368591","messageId":"20190206151153.20813-1-luke@diamand.org","threadId":"50414","inReplyTo":null,"subject":"[PATCH 0/1] git-p4: remove ticket expiration test","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2019-02-06T15:11:52Z","receivedAt":"2019-02-06T15:12:58Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"As per thread here, this removes the git-p4 ticket expiration\ntest, since it isn't really that useful.\n\nhttps://marc.info/?l=git&m=154946136416003&w=2\n\nLuke Diamand (1):\n git-p4: remove ticket expiry test\n\n t/t9833-errors.sh | 27 ---------------------------\n 1 file changed, 27 deletions(-)\n\n-- \n2.20.1.611.gfbb209baf1\n\n"},{"id":"368592","messageId":"20190206151153.20813-2-luke@diamand.org","threadId":"50414","inReplyTo":"20190206151153.20813-1-luke@diamand.org","subject":"[PATCH] git-p4: remove ticket expiry test","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2019-02-06T15:11:53Z","receivedAt":"2019-02-06T15:13:00Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"The git-p4 login ticket expiry test causes unreliable test\nruns. Since the handling of ticket expiry in git-p4 is far\nfrom polished anyway, let's remove it for now.\n\nA better way to actually run the test is to create a python\n\"fake\" version of \"p4\" which returns whatever expiry results\nthe test requires.\n\nIdeally git-p4 would look at the expiry time before starting\nany long operations, and cleanup gracefully if there is not\nenough time left. But that's quite hard to do.\n\nSigned-off-by: Luke Diamand \n---\n t/t9833-errors.sh | 27 ---------------------------\n 1 file changed, 27 deletions(-)\n\ndiff --git a/t/t9833-errors.sh b/t/t9833-errors.sh\nindex 277d347012..47b312e1c9 100755\n--- a/t/t9833-errors.sh\n+++ b/t/t9833-errors.sh\n@@ -45,33 +45,6 @@ test_expect_success 'ticket logged out' '\n \t)\n '\n \n-test_expect_success 'create group with short ticket expiry' '\n-\tP4TICKETS=\"$cli/tickets\" &&\n-\techo \"newpassword\" | p4 login &&\n-\tp4_add_user short_expiry_user &&\n-\tp4 -u short_expiry_user passwd -P password &&\n-\tp4 group -i <<-EOF &&\n-\tGroup: testgroup\n-\tTimeout: 3\n-\tUsers: short_expiry_user\n-\tEOF\n-\n-\tp4 users | grep short_expiry_user\n-'\n-\n-test_expect_success 'git operation with expired ticket' '\n-\tP4TICKETS=\"$cli/tickets\" &&\n-\tP4USER=short_expiry_user &&\n-\techo \"password\" | p4 login &&\n-\t(\n-\t\tcd \"$git\" &&\n-\t\tgit p4 sync &&\n-\t\tsleep 5 &&\n-\t\ttest_must_fail git p4 sync 2>errmsg &&\n-\t\tgrep \"failure accessing depot\" errmsg\n-\t)\n-'\n-\n test_expect_success 'kill p4d' '\n \tkill_p4d\n '\n-- \n2.20.1.611.gfbb209baf1\n\n"},{"id":"368594","messageId":"20190206162628.GL10587@szeder.dev","threadId":"50414","inReplyTo":"20190206151153.20813-2-luke@diamand.org","subject":"Re: [PATCH] git-p4: remove ticket expiry test","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-06T16:26:28Z","receivedAt":"2019-02-06T16:26:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Feb 06, 2019 at 03:11:53PM +0000, Luke Diamand wrote:\n> The git-p4 login ticket expiry test causes unreliable test\n> runs. Since the handling of ticket expiry in git-p4 is far\n> from polished anyway, let's remove it for now.\n\nI can't judge how far it is from polished, and whether it's worth\nhaving this test or should be removed in the meantime, but I would\nlike to clarify this part from my previous email that you might have\ninterpreted as a suggestion to remove these tests (it confused even\nmyself on second read):\n\n > I wonder why that failing 'git p4 sync' is\n > necessary in the first place, and whether it's really necessary to\n > test ticket expiration\n\nI was not wondering whether it's necessary to test ticket expiration\n_in general_. What I really meant was whether that first 'git p4\nsync' was really necessary in that test 'git operation with expired\nticket'. After all, what we want to see in this test is that 'git p4\nsync' fails with a specific error message when the ticket expired, and\nwhen flakiness hits, then this first 'git p4 sync' does fail with the\nexpected error message.\n\n\n> A better way to actually run the test is to create a python\n> \"fake\" version of \"p4\" which returns whatever expiry results\n> the test requires.\n> \n> Ideally git-p4 would look at the expiry time before starting\n> any long operations, and cleanup gracefully if there is not\n> enough time left. But that's quite hard to do.\n> \n> Signed-off-by: Luke Diamand \n> ---\n> t/t9833-errors.sh | 27 ---------------------------\n> 1 file changed, 27 deletions(-)\n> \n> diff --git a/t/t9833-errors.sh b/t/t9833-errors.sh\n> index 277d347012..47b312e1c9 100755\n> --- a/t/t9833-errors.sh\n> +++ b/t/t9833-errors.sh\n> @@ -45,33 +45,6 @@ test_expect_success 'ticket logged out' '\n> \t)\n> '\n> \n> -test_expect_success 'create group with short ticket expiry' '\n> -\tP4TICKETS=\"$cli/tickets\" &&\n> -\techo \"newpassword\" | p4 login &&\n> -\tp4_add_user short_expiry_user &&\n> -\tp4 -u short_expiry_user passwd -P password &&\n> -\tp4 group -i <<-EOF &&\n> -\tGroup: testgroup\n> -\tTimeout: 3\n> -\tUsers: short_expiry_user\n> -\tEOF\n> -\n> -\tp4 users | grep short_expiry_user\n> -'\n> -\n> -test_expect_success 'git operation with expired ticket' '\n> -\tP4TICKETS=\"$cli/tickets\" &&\n> -\tP4USER=short_expiry_user &&\n> -\techo \"password\" | p4 login &&\n> -\t(\n> -\t\tcd \"$git\" &&\n> -\t\tgit p4 sync &&\n> -\t\tsleep 5 &&\n> -\t\ttest_must_fail git p4 sync 2>errmsg &&\n> -\t\tgrep \"failure accessing depot\" errmsg\n> -\t)\n> -'\n> -\n> test_expect_success 'kill p4d' '\n> \tkill_p4d\n> '\n> -- \n> 2.20.1.611.gfbb209baf1\n> \n"},{"id":"368706","messageId":"nycvar.QRO.7.76.6.1902071343210.41@tvgsbejvaqbjf.bet","threadId":"50414","inReplyTo":"20190206151153.20813-1-luke@diamand.org","subject":"Re: [PATCH 0/1] git-p4: remove ticket expiration test","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-07T12:45:18Z","receivedAt":"2019-02-07T12:45:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Luke,\n\nOn Wed, 6 Feb 2019, Luke Diamand wrote:\n\n> As per thread here, this removes the git-p4 ticket expiration\n> test, since it isn't really that useful.\n> \n> https://marc.info/?l=git&m=154946136416003&w=2\n\nThank you for the prompt patch!\n\nHowever, like Gábor, my feeling is that we would want that test case in a\nnon-flakey form, if possible. If you think that that is only possible with\na mocked p4, so be it, let's remove the test case (because the mocked one\nwill likely look quite a bit different). But if there are easier ways to\nwork around the timing issues (such as dropping the first `sync`), then\nI'd prefer to have the safety of a regression test.\n\nThanks,\nDscho\n\n> Luke Diamand (1):\n> git-p4: remove ticket expiry test\n> \n> t/t9833-errors.sh | 27 ---------------------------\n> 1 file changed, 27 deletions(-)\n> \n> -- \n> 2.20.1.611.gfbb209baf1\n> \n> "},{"id":"368752","messageId":"20190207232552.4246fec6f3057aea05211141@diamand.org","threadId":"50414","inReplyTo":"nycvar.QRO.7.76.6.1902071343210.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/1] git-p4: remove ticket expiration test","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2019-02-07T23:25:52Z","receivedAt":"2019-02-07T23:25:57Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Thu, 7 Feb 2019 13:45:18 +0100 (STD)\nJohannes Schindelin wrote:\n\n> Hi Luke,\n> \n> On Wed, 6 Feb 2019, Luke Diamand wrote:\n> \n> > As per thread here, this removes the git-p4 ticket expiration\n> > test, since it isn't really that useful.\n> > \n> > https://marc.info/?l=git&m=154946136416003&w=2\n> \n> Thank you for the prompt patch!\n> \n> However, like Gábor, my feeling is that we would want that test case in a\n> non-flakey form, if possible. If you think that that is only possible with\n> a mocked p4, so be it, let's remove the test case (because the mocked one\n> will likely look quite a bit different). But if there are easier ways to\n> work around the timing issues (such as dropping the first `sync`), then\n> I'd prefer to have the safety of a regression test.\n\nI've got a mocked-up p4 wrapper which returns whatever expiration time the test needs. I'll submit it tomorrow.\n\nIt's just a few lines of python script to generate the marshalled data, so it's not very complicated.\n\n> \n> Thanks,\n> Dscho\n> \n> > Luke Diamand (1):\n> > git-p4: remove ticket expiry test\n> > \n> > t/t9833-errors.sh | 27 ---------------------------\n> > 1 file changed, 27 deletions(-)\n> > \n> > -- \n> > 2.20.1.611.gfbb209baf1\n> > \n> > \n\n\n-- \nLuke Diamand \n"},{"id":"368814","messageId":"nycvar.QRO.7.76.6.1902081114470.41@tvgsbejvaqbjf.bet","threadId":"50414","inReplyTo":"20190207232552.4246fec6f3057aea05211141@diamand.org","subject":"Re: [PATCH 0/1] git-p4: remove ticket expiration test","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-08T10:15:16Z","receivedAt":"2019-02-08T10:15:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Luke,\n\nOn Thu, 7 Feb 2019, Luke Diamand wrote:\n\n> I've got a mocked-up p4 wrapper which returns whatever expiration time\n> the test needs. I'll submit it tomorrow.\n\nGreat!\nDscho\n"}]}