Re: [PATCH v3 2/3] builtins: kill: Add `kill ''` regression test with explicit sigspec

[Daniel Shahaf <d.s@daniel.shahaf.name>]

Chris Down wrote on Tue, 18 Feb 2020 10:26 -0500:
> Daniel Shahaf writes:
> >Chris Down wrote on Mon, 17 Feb 2020 10:12 -0500:  
> >> The version without a sigspec can't be added yet because it would still
> >> kill the test runner even in expected-to-fail mode, see workers/45449
> >> for discussion. For the same reason, we use a signal which is non-fatal
> >> by default and unlikely to be sent by someone else, SIGURG, to do the
> >> expected-to-fail case prior to the fix.  
> >
> >Do you consider the rationale for using SIGURG important enough to be
> >added in a code comment?  
> 
> Hmm, when I was considering whether to write it in the commit message or in the 
> file, my rationale was that after 3/3 we're still going to end up killing the 
> whole process group if it regresses, so likely the only people who'd run into 
> this distinction are those doing `git bisect`. As such the commit message 
> seemed the best place to me. I'm neutral though :-)

I understand, but I'm not convinced:

1. zsh's test suite aborts a test file as soon as any test group
fails.  Thus, if the bug regresses, the URG test will run but the TERM
test won't.

2. The URG test may be copied and adapted.

3. The reasons for picking URG are of interest to anyone who reads the
code (for whatever reason), even if few people will do so.

> >I've pushed this series to master with prose changes only.  
> 
> Thank you, and thanks for your diligent review!

You're welcome!

Cheers,

Daniel