Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> Negate git with "test_must_fail", not "!", this would e.g. hide
> segfaults. See t/README's discussion about it.
>
>> + test_cmp expect actual
>
> Looks like this should be:
>
> test_must_fail git ... >out &&
> test_must_be_empty out
Nice! I don't know why I didn't look for t/README, but I also found
test_expect_code, which seems to be even more specific as to what is
being expected. I assume it has the same segfault detection.
This has now been incorporated in my branch; I'll submit it in v3 later
today.
>> +test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '
>> + test_config core.editor foo &&
>> + (
>> + sane_unset GIT_EDITOR &&
>> + sane_unset VISUAL &&
>> + sane_unset EDITOR &&
>> + echo foo >expect &&
>> + EDITOR=bar git var GIT_EDITOR >actual &&
>> + test_cmp expect actual
>> + )
>
> Perhaps these can all be factored into a helper to hide this repetition
> in a function, but maybe not. E.g:
>
> test_git_var () {
> cat >expect &&
> (
> [...common part of subshell ...]
> "$@" >actual &&
> test_cmp expect actual
> )
> }
>
> (untested)
In all honesty, I think too much abstraction would do more harm than
good here. I definitely share the instinct to factor out the common
pieces, but in other codebases I've worked in, that tends to stifle
future changes in the tests themselves.
That said, I can't realistically imagine a world where a
'sane_unset_all_editors' would stifle code changes -- and I think that
accounts for the lion's share of the repetition. I've incorporated such
a helper in my branch now.
If you're not convinced there should be further abstraction, I'd rather
leave things 'stupid simple' -- but if you think this would block merge,
I'd be happy to take a crack at further factoring out what I can.
--
Sean Allred
