Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

[Martin Ågren]

On 25 February 2018 at 04:48, Kaartic Sivaraam
 wrote:
> On Wednesday 21 February 2018 02:14 AM, Martin Ågren wrote:
>> To sum up: I probably won't be looking into Travis-ing such a blacklist
>> in the near future.
>>
>
> Just thinking out loud, how about having a white-list instead of a
> black-list and using it to run only those tests in the white list.
> Something like,
>
> t/white_list
> 
> t
> t0001
>
> To run
> --
>
> cd t/
> for test in $(cat white_list)
> do
> white_list_tests="$white_list_tests $test*"
> done
> make SANITIZE=leak $white_list_tests

Yeah, that would also work. An incomplete whitelist can't cause errors
for those running other tests, as an incomplete blacklist could. So
that's good. At some point, the whitelist would need to be turned into a
blacklist. At the very latest when the whitelist is the full set of
tests, in order to flip the default of new tests. ;-) Right now, I think
whitelists and blacklists are about equally useful.

Let's hope we're heading for a future where a blacklist gets more and
more feasible, whereas a whitelist would get longer and longer. ;-)

Martin

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

[Kaartic Sivaraam]

On Wednesday 21 February 2018 02:14 AM, Martin Ågren wrote:
> To sum up: I probably won't be looking into Travis-ing such a blacklist
> in the near future.
> 

Just thinking out loud, how about having a white-list instead of a
black-list and using it to run only those tests in the white list.
Something like,

t/white_list

t
t0001

To run
--

cd t/
for test in $(cat white_list)
do
white_list_tests="$white_list_tests $test*"
done
make SANITIZE=leak $white_list_tests



-- 
Kaartic

QUOTE
---

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

  - Joel Spolsky



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

[Jeff King]

On Wed, Feb 21, 2018 at 08:53:16AM -0800, Junio C Hamano wrote:

> Even though keeping track of list of known-leaky tests may not be so
> interesting, we can still salvage useful pieces from the discussion
> and make them available to developers, e.g.  something like
> 
> prove --dry --state=failed |
> perl -lne '/^(t[0-9]{4})-.*\.sh$/ and print $1' | sort >$@+
> if cmp >/dev/null $@ $@+; then rm $@+; else mv $@+ $@; fi
> 
> could be made into a target to stash away the list of failing tests
> after a test run?

Unfortunately there are some caveats in that snippet:

  1. You are using prove.

  2. You are using --state=save in the initial run.

I think we might be better off having the test scripts write to
test-results/*.counts even when run under a TAP harness, and then we can
have a consistent way to get the list of failed tests (we already have a
"make failed" that works this way).

-Peff

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

[Junio C Hamano]

Martin Ågren  writes:

> On 19 February 2018 at 22:29, Jeff King  wrote:
> ...
>> Or alternatively, we could just not bother with checking this into the
>> repository, and it becomes a local thing for people interested in
>> leak-testing. What's the value in having a shared known-leaky list,
>> especially if we don't expect most people to run it.
>
> This sums up my feeling about this.

Even though keeping track of list of known-leaky tests may not be so
interesting, we can still salvage useful pieces from the discussion
and make them available to developers, e.g.  something like

prove --dry --state=failed |
perl -lne '/^(t[0-9]{4})-.*\.sh$/ and print $1' | sort >$@+
if cmp >/dev/null $@ $@+; then rm $@+; else mv $@+ $@; fi

could be made into a target to stash away the list of failing tests
after a test run?

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

[Jeff King]

On Tue, Feb 20, 2018 at 09:44:51PM +0100, Martin Ågren wrote:

> For finding this sort of low-hanging fruit (windfall?) any local
> scripting and greping will do. I sort of hesitate wasting community CPU
> cycles on something which might be appreciated, in theory, but that
> doesn't have enough attention to move beyond "every now and then,
> something is fixed, but just as often, tests gain leaks and the
> blacklist gains entries".
> 
> To sum up: I probably won't be looking into Travis-ing such a blacklist
> in the near future.

Yeah, I think that's the right path for now. Hopefully we can get close
to zero leaks at some point, and then think about adding this kind of
infrastructure. Thanks for talking it through.

-Peff

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

[Martin Ågren]

On 19 February 2018 at 22:29, Jeff King  wrote:
> On Wed, Feb 14, 2018 at 10:56:37PM +0100, Martin Ågren wrote:
>
>> Here's what a list of known leaks might look like. It feels a bit
>> awkward to post a known-incomplete list (I don't run all tests). Duy
>> offered to pick up the ball if I gave up, maybe you could complete and
>> post this as your own? :-? Even if I (or others) can't reproduce the
>> complete list locally, regressions will be trivial to find, and newly
>> leak-free tests fairly easy to notice.
>
> I didn't think about that when I posted my scripts. In general, it's OK
> to me if you miss a script when you generate the "leaky" list. But if
> you skip it, you cannot say whether it is leaky or not, and should
> probably neither add nor remove it from the known-leaky list. So I think
> the second shell snippet needs to become a little more clever about
> skipped test scripts.
>
> Even that isn't 100% fool-proof, as some individual tests may be skipped
> or not skipped on various platforms. But it may be enough in practice
> (and eventually we'd have no known-leaky tests, of course ;) ).

All good points.

> Or alternatively, we could just not bother with checking this into the
> repository, and it becomes a local thing for people interested in
> leak-testing. What's the value in having a shared known-leaky list,
> especially if we don't expect most people to run it.

This sums up my feeling about this.

> I guess it lets us add a Travis job to do the leak-checking, which might
> get more coverage. So maybe if we do have an in-repo known-leaky, it
> should match some canonical Travis environment. That won't give us
> complete coverage, but at this point we're just trying to notice
> low-hanging fruit.

Right. A Travis job to get non-complete but consistent data would be
"nice" (TM), but at this point it would perhaps just result in
blacklist-churning. At some point, adding/removing entries in a
blacklist will have some signal-value and psychology to it, but right
now it might just be noise. (My patch adds an incomplete blacklist of
539 scripts...)

For finding this sort of low-hanging fruit (windfall?) any local
scripting and greping will do. I sort of hesitate wasting community CPU
cycles on something which might be appreciated, in theory, but that
doesn't have enough attention to move beyond "every now and then,
something is fixed, but just as often, tests gain leaks and the
blacklist gains entries".

To sum up: I probably won't be looking into Travis-ing such a blacklist
in the near future.

Martin

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

[Jeff King]

On Wed, Feb 14, 2018 at 10:56:37PM +0100, Martin Ågren wrote:

> Here's what a list of known leaks might look like. It feels a bit
> awkward to post a known-incomplete list (I don't run all tests). Duy
> offered to pick up the ball if I gave up, maybe you could complete and
> post this as your own? :-? Even if I (or others) can't reproduce the
> complete list locally, regressions will be trivial to find, and newly
> leak-free tests fairly easy to notice.

I didn't think about that when I posted my scripts. In general, it's OK
to me if you miss a script when you generate the "leaky" list. But if
you skip it, you cannot say whether it is leaky or not, and should
probably neither add nor remove it from the known-leaky list. So I think
the second shell snippet needs to become a little more clever about
skipped test scripts.

Even that isn't 100% fool-proof, as some individual tests may be skipped
or not skipped on various platforms. But it may be enough in practice
(and eventually we'd have no known-leaky tests, of course ;) ).

Or alternatively, we could just not bother with checking this into the
repository, and it becomes a local thing for people interested in
leak-testing. What's the value in having a shared known-leaky list,
especially if we don't expect most people to run it.

I guess it lets us add a Travis job to do the leak-checking, which might
get more coverage. So maybe if we do have an in-repo known-leaky, it
should match some canonical Travis environment. That won't give us
complete coverage, but at this point we're just trying to notice
low-hanging fruit.

-Peff

[PATCH] t/known-leaky: add list of known-leaky test scripts

[Martin Ågren]

Here's what a list of known leaks might look like. It feels a bit
awkward to post a known-incomplete list (I don't run all tests). Duy
offered to pick up the ball if I gave up, maybe you could complete and
post this as your own? :-? Even if I (or others) can't reproduce the
complete list locally, regressions will be trivial to find, and newly
leak-free tests fairly easy to notice.

I'm not sure if the shamelessly stolen shell snippets warrant their own
scripts, or how make targets overriding various variables would be
received. At least they're in the commit message.

-- >8 --
We have quite a lot of leaks in the code base. Using SANITIZE=leak, it
is easy to find them, and every now and then we simply stumble upon one.
Still, we can expect it to take some time to get to the point where
`make SANITIZE=leak test` succeeds.

Until that happens, it would be nice if we could at least try not to
regress in the sense that a test t which was at one point leak-free
turns leaky. Such a regression would indicate that leak-free code
turned leaky, that a new feature is leaky, or that we simply happen to
trigger an existing leak as part of a newly added/modified test.

To that end, introduce a list of known-leaky tests, i.e., a list of
t-values. Now this will be able to find such regressions:

make SANITIZE=leak test GIT_SKIP_TESTS="$(cat t/known-leaky)"

The list was generated, and can be updated, as follows:

# Assume we're using prove, which will keep running after failure,
# and will record the results for us to parse (using "--state=").
# Otherwise use "make -k" and grep in t/test-results.
GIT_TEST_OPTS=-i make SANITIZE=leak test
cd t
prove --dry --state=failed |
perl -lne '/^(t[0-9]{4})-.*\.sh$/ and print $1' |
sort >known-leaky

The list added in this commit might be incomplete, since I do not run
all tests (I'm missing svn, cvs, p4, Windows-only and
filesystem-dependent tests, as well as "writeable /"). The majority of
these do not primarily test our C code, but all of them might trigger
leaks, in which case they would belong in this list.

Suggested-by: Nguyễn Thái Ngọc Duy 
Helped-by: Jeff King 
Signed-off-by: Martin Ågren 
---
 t/known-leaky | 539 ++
 1 file changed, 539 insertions(+)
 create mode 100644 t/known-leaky

diff --git a/t/known-leaky b/t/known-leaky
new file mode 100644
index 00..80a0af4d09
--- /dev/null
+++ b/t/known-leaky
@@ -0,0 +1,539 @@
+t0002
+t0003
+t0006
+t0008
+t0009
+t0012
+t0020
+t0021
+t0023
+t0040
+t0050
+t0056
+t0060
+t0062
+t0064
+t0070
+t0090
+t0100
+t0101
+t0110
+t0203
+t0300
+t0301
+t0302
+t1001
+t1002
+t1004
+t1005
+t1006
+t1007
+t1008
+t1011
+t1012
+t1013
+t1020
+t1021
+t1050
+t1051
+t1090
+t1301
+t1302
+t1304
+t1306
+t1308
+t1350
+t1400
+t1401
+t1402
+t1403
+t1404
+t1405
+t1406
+t1407
+t1408
+t1409
+t1410
+t1411
+t1412
+t1413
+t1414
+t1430
+t1450
+t1500
+t1502
+t1505
+t1507
+t1508
+t1510
+t1511
+t1512
+t1514
+t1700
+t2007
+t2008
+t2009
+t2010
+t2011
+t2012
+t2013
+t2014
+t2015
+t2016
+t2017
+t2018
+t2019
+t2020
+t2021
+t2022
+t2023
+t2024
+t2025
+t2026
+t2027
+t2030
+t2103
+t2106
+t2200
+t2203
+t2204
+t3001
+t3004
+t3005
+t3007
+t3010
+t3020
+t3030
+t3031
+t3032
+t3033
+t3034
+t3040
+t3050
+t3060
+t3200
+t3201
+t3202
+t3203
+t3204
+t3205
+t3210
+t3301
+t3302
+t3303
+t3304
+t3306
+t3307
+t3308
+t3309
+t3310
+t3311
+t3400
+t3402
+t3403
+t3404
+t3405
+t3406
+t3407
+t3408
+t3409
+t3410
+t3411
+t3412
+t3413
+t3414
+t3415
+t3416
+t3417
+t3418
+t3419
+t3420
+t3421
+t3425
+t3426
+t3427
+t3428
+t3429
+t3500
+t3501
+t3502
+t3503
+t3504
+t3505
+t3506
+t3507
+t3508
+t3509
+t3510
+t3511
+t3512
+t3513
+t3600
+t3700
+t3701
+t3800
+t3900
+t3901
+t3903
+t3904
+t3905
+t3906
+t4001
+t4008
+t4010
+t4013
+t4014
+t4015
+t4016
+t4017
+t4018
+t4020
+t4021
+t4022
+t4023
+t4026
+t4027
+t4028
+t4030
+t4031
+t4036
+t4038
+t4039
+t4041
+t4042
+t4043
+t4044
+t4045
+t4047
+t4048
+t4049
+t4051
+t4052
+t4053
+t4055
+t4056
+t4057
+t4059
+t4060
+t4061
+t4064
+t4065
+t4103
+t4107
+t4108
+t4111
+t4114
+t4115
+t4117
+t4118
+t4120
+t4121
+t4122
+t4124
+t4125
+t4127
+t4131
+t4135
+t4137
+t4138
+t4150
+t4151
+t4152
+t4153
+t4200
+t4201
+t4202
+t4203
+t4204
+t4205
+t4206
+t4207
+t4208
+t4209
+t4210
+t4211
+t4212
+t4213
+t4252
+t4253
+t4254
+t4255
+t4300
+t5000
+t5001
+t5002
+t5003
+t5004
+t5100
+t5150
+t5300
+t5301
+t5302
+t5303
+t5304
+t5305
+t5306
+t5310
+t5311
+t5312
+t5313
+t5314
+t5315
+t5316
+t5317
+t5400
+t5401
+t5402
+t5403
+t5404
+t5405
+t5406
+t5407
+t5408
+t5500
+t5501
+t5502
+t5503
+t5504
+t5505
+t5506
+t5509
+t5510
+t5512
+t5513
+t5514
+t5515
+t5516
+t5517
+t5518
+t5519
+t5520
+t5521
+t5522
+t5523
+t5524
+t5525
+t5526
+t5527
+t5528
+t5531
+t5532
+t5533
+t5534
+t5535
+t5536
+t5537
+t5538
+t5539
+t5540
+t5541
+t5542
+t5543
+t5544
+t5545
+t5546
+t5547
+t5550
+t5551
+t5560
+t5561
+t5570
+t5571
+t5572
+t5573
+t5600
+t5601
+t5603
+t5604
+t5605