Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> Because I took the time to scan backward through this test script, I
> understand that `core.commitGraph` is already `true` by the time this
> test is reached, thus the new test title is accurate (for now). However,
> it would be a bit easier to reason about this and make it more robust by
> having the test itself guarantee that it is set to `true` by invoking
> `git config core.commitGraph true`.
Reading this and..
> I find it somewhat confusing to reason about the behavior of
> test_when_finished() when it is invoked like this before the `cd`. It's
> true that the end-of-test `git config core.commitGraph true` will be
> invoked within `full` as desired (except in the very unusual case of the
> `cd` failing), so it's probably correct, but it requires extra thought
> to come to that conclusion. Switching the order of these two lines might
> lower the cognitive load a bit.
I'll make both of these changes. Test readability is important and
people shouldn't be wasting time thinking about test correctness.
> test_config() unsets the config variable when the test completes, so I'm
> wondering if its use is in fact desirable here. I ask because, from a
> quick scan through the file, it appears that many tests in this script
> assume that `core.commitGraph` is `true`, so it's not clear that
> unsetting it at the end of this test is a good idea in general.
This is a good point. I made the original change in response to Taylor's
comment, but I think we both didn't consider that this script assumes
`core.commitGraph` is `true`. The rest of the tests pass, but only
because the default value is true and I'd rather not have tests rely on
a happy accident.
> Taking Taylor's comment[1] on v1 patch [2/3] into account, I wonder if
> it would be simpler for these all to be combined into a single test.
Interesting thought. The marginal benefit here is much lower than in
patch [2/3]. The difference is that corrupt_midx_and_verify() uses an
additional $COMMAND to perform verification, but
corrupt_graph_and_verify() does not. The result is that
corrupt_midx_and_verify() is subtle and confusing when used in separate
tests, but corrupt_graph_and_verify() is not so bad.
> A downside of combining the tests like this is that it makes it a bit
> more cumbersome to diagnose a failure (because there is more code and
> more cases to wade through).
Yes, and we also lose the test labels that would guide the reader. It
might not be that obvious why the tests for a boolean value has 3
cases {true,false,unset}.
Taking churn into account, I'm inclined to keep the tests separate.
