Re: [OUTREACHY][PATCH v1] t7006: Use test_path_is_* functions in test script

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 20, 2020, 12:19 UTC
Message-ID: <0c61c3cc-b6b6-84dc-7e3f-99a41f676e7f@gmail.com>
In-Reply-To: <O2ACwMmJ8_oj_OrMPF4bGf-9nctJiXbFxJ6yzlWWCob0Zr25aSCOfcCc8pGNukbReQQJi_6igxiEyQT1hWp3ccbY1pJr62Y4eNk097tInP8=@protonmail.com>

Hi Joey

On 20/10/2020 08:24, Joey S wrote:

Show 39 quoted lines

> Hi everyone,
> 
> Thank you very much for the input and feedback, it's much appreciated.
> 
>> All this text above is useful context for reviewers but appears as part
>> of the commit message which is not what you want. If you add notes after
>> the `---` line below then they will not end up in the commit message.
>>
> Understood, thank you.
> 
>>> Modernized the test by replacing 'test -e' instances with
>>> test_path_is_file helper functions.
>>
>> s/Modernized/Modernize/
> Will do in the amended commit next.
> 
>>> -   ! test_path_is_file paginated.out
>>
>> It would be better to replace`! test -e` this with
>> `test_path_is_missing` as the modified test will pass if paginated.out
>> exists but is not a file. `test_path_is_missing` will print an
>> appropriate diagnostic message as well.
> 
> Thank you for the explanation : )
> 
> After replacing `! test -e` with `! test_path_is_missing paginated.out` however, the changed test cases are failing;
> ```
> $ cd t/ && prove t7006-pager.sht7006-pager.sh .. Dubious, test returned 1 (wstat 256, 0x100)
> Failed 3/101 subtests
> 
> Test Summary Report
> -------------------
> t7006-pager.sh (Wstat: 256 Tests: 101 Failed: 3)
>    Failed tests:  7-9
>    Non-zero exit status: 1
> Files=1, Tests=101,  5 wallclock secs ( 0.03 usr  0.00 sys +  3.49 cusr  0.65 csys =  4.17 CPU)
> Result: FAIL
> ```
> Is this the behavior I should be expecting?

No it's not! As one aspect of this process is to help candidates improve their understanding I'll give you a hint rather than the whole answer. `test -e <path>` checks whether <path> exists and exits 0 if it does and the shell treats an exit code of 0 as success. `!` inverts the success/failure of the command that follows it. Using that and looking at the definition of test_file_is_missing in t/test-lib-functions.sh see if you can fix the conversion so that the tests pass. If you get stuck do let me know and I'll try and help some more.

Best Wishes

Phillip

Show 54 quoted lines

>> ...Alternatively, this would fit just fine in a cover letter. Usually
>> cover letters are not necessary for single patches (where the patch
>> message itself conveys the full message, or a little bit of additional
>> context below the triple-dash line is all that's necessary to clarify
>> the intent). But, if you want to introduce yourself, a 0/1 cover letter
>> is fine, too.
> 
> Will keep this in mind, thank you Taylor.
> 
>>> One thing missed by other commenters: the Developer's Certificate of
>>> Origin line - which is what this indicates - should have your "full
>>> name".
>>
>> ... and it must match the authorship.
> 
> Changed, thank you both for catching that.
> 
> Thank you all,
> Joey
> 
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Monday, October 19, 2020 7:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
>> Emily Shaffer emilyshaffer@google.com writes:
>>
>>> On Mon, Oct 19, 2020 at 04:26:07AM +0000, Joey S wrote:
>>>
>>>> Hi everyone,
>>>> Hi Joey and welcome.
>>>
>>>> Signed-off-by: JoeyS jgsal@yahoo.com
>>>
>>> One thing missed by other commenters: the Developer's Certificate of
>>> Origin line - which is what this indicates - should have your "full
>>> name".
>>
>> ... and it must match the authorship.
>>
>>> So in my case, I sign my patches 'Emily Shaffer
>>> emilyshaffer@google.com'. If I'm wrong that's fine, but JoeyS sounds
>>> like a name and initial rather than a full name.
>>
>> Thanks for pointing it out.
>>
>> If somebody from the "mentoring" group is taking a tally, it might
>> not be a bad idea to identify which style and procedure rules are
>> often failed to be followed by new contributors so that we can
>> figure out ways to make them stand out in our documentation set
>> (e.g. Documentation/SubmittingPatches but maybe a separate cheat
>> sheet might be worth having).
>>
>> Thanks.
> 
>