Hello Eric,
On 2024-03-18 03:48, Eric Sunshine wrote:
On Sun, Mar 17, 2024 at 12:50 AM Dragan Simic <dsimic@xxxxxxxxxxx>
wrote:
On 2024-03-17 05:21, Eric Sunshine wrote:
> On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@xxxxxxxxxxx>
> wrote:
>> + x_to_tab >.git/config <<-\EOF
>> + [section]
>> + Xsolid = rock
>> + Xsparse = big XX blue
>> + ...
>> + EOF
>> +'
>
> The <<- operator strips all leading TAB characters, so the extra
> indentation you've placed inside the "[section]" section is stripped
> off. Thus, what you have above is the same as:
>
> x_to_tab >.git/config <<-\EOF
> [section]
> Xsolid = rock
> ...
> EOF
Yes, I was already aware that such indentation ends up wasted,
but having it makes the test a bit more readable. At least in
my opinion, but if you find it better not to have such whitespace,
for the sake of consistency, I'll happily remove this indentation
in the v3.
Readability wasn't my reason for bringing this up. As a reviewer,
every time a question pops into my mind as I'm reading the code, that
indicates that something about the code is unclear or that the commit
message doesn't properly explain why it was done in this way. People
coming across this code in the future may have the same questions but
they won't have the benefit of being able to easily ask you why it was
done this way.
I see. How about including a small comment in the t1300 that would
explain the additional indentation?
As a note, there are already more tests in the t1300 that contain such
indentation, so maybe we shoulddo something with those existing tests
as well; the above-proposed comment, which would be placed at the very
beginning of t1300, may provide a satisfactory explanation for all the
tests in t1300 that contain such additional indentation.
Another option would be to either add the indentation to all relevant
tests in the t1300, or to remove the indentation from all tests in the
t1300 that already contain it. I'd be happy to implement and submit
patches that do that, after we choose the direction we want to follow.
So, the ideal patch is one in which the reviewer reads the code and
simply nods in understanding without having to question any of the
author's choices. And the ideal test is one in which does the bare
minimum to verify that the condition being checked is in fact correct;
there should be no extraneous code or behavior which could mislead the
reader into wondering why it was done that way.
In this particular case, it wasn't clear whether you, as author,
realized that all leading TABs are stripped from the heredoc body, and
whether or not that mattered to you and whether or not you expected it
to be significant to the test's behavior.
I fully agree with the most of your points above. The only slight
disagreement would be about the bare minimum when it comes to automated
tests; in my, opinion, it's actually good to have a few twisties here
and there, simply to exercise the underlying logic better by such tests.
This probably opens a question whether the automated tests should be
orthogonal? Perhaps the majority od them should, but IMHO having a few
composite tests can only help with the validation of the underlying
logic.
> On a related note, it's not clear why you use 'X' to insert a TAB at
> the beginning of each line. As I understand it, the configuration file
> reader does not require such indentation, thus doing so is wasted.
> Moreover, it confuses readers of this code (and reviewers) into
> thinking that something unusual is going on, and leads to questions
> such as this one: Why do you use 'X' to insert a TAB at the beginning
> of the line?
Well, resorting to always not having such instances of 'X' to provide
leading indentation in test configuration files may actually make some
bugs go undetected in some tests. To me, having leading indentation
is
to be expected in the real configuration files in the field, thus
providing the same indentation in a test configuration feels natural
to
me, despite admittedly making the test configuration a bit less
readable.
Of course, consistency is good, but variety is also good when it comes
to automated tests. I'm not very familiar with the tests in git, so
please let me know if consistency outweights variety in this case, and
I'll happily remove the leading "X" indentations in the v3.
My assumption, perhaps incorrectly, was that existing tests already
verified correct behavior of leading whitespace and that the tests
added by this patch were about internal whitespace. If that's not the
case (and perhaps I didn't fully digest the commit message) then my
question about the leading "X" is off the mark.
Frankly, I can't be 100% sure without spending quite a lot of time, but
I don't think that the already existing tests in the t1300 were tailored
specifically to cover the verification of the leading whitespace, i.e.
of the indentation in git configuration files. To me, it seems more
like
a random choice of including the indentation or not.
Such a random approach may actually be good, despite bringing back the
above-mentioned question about the orthogonality of the tests.
If these new tests are also checking leading whitespace behavior, then
to improve coverage, would it make sense to have the leading "X" on
some lines but not others?
Good point, despite that not being the main purpose of the added tests.
I'll see to add a couple of tests that check the handling of
indentation,
possibly at some places in the t1300 that fit the best; improving the
tests coverage can only help in the long run.
