Clever! Opt-in for those who desire precise tests.
Tests aren't only for testing a subjective "good enough" in the
estimation of the author of the code in question, but also for others
who later touch the same area and want to avoid regressions.
Which is why I think it's an anti-pattern to use "grep SOME-SUBSTR" in
lieu of test_cmp if we can easily do the latter.
Sounds good. It shouldn't be too hard to satisfy both camps,
i.e. the quoted demonstrates one way to allow test writers to
give expectation in-place in the single test file, and replacing
how it uses "grep" to check the output with test_cmp or whatever
wouldn't make the resulting tests too hard to write and maintain.
Clever! Opt-in for those who desire precise tests.
Tests aren't only for testing a subjective "good enough" in the
estimation of the author of the code in question, but also for others
who later touch the same area and want to avoid regressions.
Which is why I think it's an anti-pattern to use "grep SOME-SUBSTR" in
lieu of test_cmp if we can easily do the latter.
Sounds good. It shouldn't be too hard to satisfy both camps,
i.e. the quoted demonstrates one way to allow test writers to
give expectation in-place in the single test file, and replacing
how it uses "grep" to check the output with test_cmp or whatever
wouldn't make the resulting tests too hard to write and maintain.
It doesn't satisfy both camps, because I'd like to convert all these
tests to test_cmp because for a subsequent refactoring of userdiff.c by
me or others I don't know in advance what might break, so I'd like to
assert the exact current behavior.
Whereas your patch provides a way to opt-in individual tests to a
test_cmp-alike, but leaves the rest at grepping for the "RIGHT"
substring. Failures in the tests who aren't opted-in will be hidden.
It also means that subsequent changes to the behavior in the form of
submitted patches won't be as self-documenting, e.g. I've wondered if we
could introduce a case to balance parens in this code (sometimes C
function declarations stretch across lines), and there's e.g. the
arbitrary limit of 80 bytes on the line (which to be fair, we don't
curretly have tests for).
Anyway, as noted in [1] I don't see how this custom format of grepping
stuff out of plain-text files is simpler, particularly when its behavior
would start to rely on other things like "# HEADER |right()|" whose
behavior is a function of what we grep/sed when/where in the logic
driving the tests.
But if you & Johannes S. disagree with that I don't really say a way
forward with this series. I think e.g. squashing 09/27 into the rest
would make things simpler/less verbose, but the end-state would still be
matching the full hunk line, and if that's not something that's wanted
in any shape or form as a default...
1. https://lore.kernel.org/git/87h7mba3h3.fsf@evledraar.gmail.com/
From: Johannes Sixt <hidden> Date: 2021-02-23 21:04:58
Am 23.02.21 um 14:11 schrieb Ævar Arnfjörð Bjarmason:
On Wed, Feb 17 2021, Junio C Hamano wrote:
quoted
Sounds good. It shouldn't be too hard to satisfy both camps,
i.e. the quoted demonstrates one way to allow test writers to
give expectation in-place in the single test file, and replacing
how it uses "grep" to check the output with test_cmp or whatever
wouldn't make the resulting tests too hard to write and maintain.
It doesn't satisfy both camps, because I'd like to convert all these
tests to test_cmp because for a subsequent refactoring of userdiff.c by
me or others I don't know in advance what might break, so I'd like to
assert the exact current behavior.
Whereas your patch provides a way to opt-in individual tests to a
test_cmp-alike, but leaves the rest at grepping for the "RIGHT"
substring. Failures in the tests who aren't opted-in will be hidden.
It also means that subsequent changes to the behavior in the form of
submitted patches won't be as self-documenting, e.g. I've wondered if we
could introduce a case to balance parens in this code (sometimes C
function declarations stretch across lines), and there's e.g. the
arbitrary limit of 80 bytes on the line (which to be fair, we don't
curretly have tests for).
Anyway, as noted in [1] I don't see how this custom format of grepping
stuff out of plain-text files is simpler, particularly when its behavior
would start to rely on other things like "# HEADER |right()|" whose
behavior is a function of what we grep/sed when/where in the logic
driving the tests.
But if you & Johannes S. disagree with that I don't really say a way
forward with this series. I think e.g. squashing 09/27 into the rest
would make things simpler/less verbose, but the end-state would still be
matching the full hunk line, and if that's not something that's wanted
in any shape or form as a default...
1. https://lore.kernel.org/git/87h7mba3h3.fsf@evledraar.gmail.com/
I could live with a version of Junio's suggestion that is not opt-in,
i.e., the checks are mandatory and exact. The important point is that
there is only one file per test case; that would still count as
"sufficiently simple" in my book.
-- Hannes
Am 23.02.21 um 14:11 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Wed, Feb 17 2021, Junio C Hamano wrote:
quoted
Sounds good. It shouldn't be too hard to satisfy both camps,
i.e. the quoted demonstrates one way to allow test writers to
give expectation in-place in the single test file, and replacing
how it uses "grep" to check the output with test_cmp or whatever
wouldn't make the resulting tests too hard to write and maintain.
It doesn't satisfy both camps, because I'd like to convert all these
tests to test_cmp because for a subsequent refactoring of userdiff.c by
me or others I don't know in advance what might break, so I'd like to
assert the exact current behavior.
Whereas your patch provides a way to opt-in individual tests to a
test_cmp-alike, but leaves the rest at grepping for the "RIGHT"
substring. Failures in the tests who aren't opted-in will be hidden.
It also means that subsequent changes to the behavior in the form of
submitted patches won't be as self-documenting, e.g. I've wondered if we
could introduce a case to balance parens in this code (sometimes C
function declarations stretch across lines), and there's e.g. the
arbitrary limit of 80 bytes on the line (which to be fair, we don't
curretly have tests for).
Anyway, as noted in [1] I don't see how this custom format of grepping
stuff out of plain-text files is simpler, particularly when its behavior
would start to rely on other things like "# HEADER |right()|" whose
behavior is a function of what we grep/sed when/where in the logic
driving the tests.
But if you & Johannes S. disagree with that I don't really say a way
forward with this series. I think e.g. squashing 09/27 into the rest
would make things simpler/less verbose, but the end-state would still be
matching the full hunk line, and if that's not something that's wanted
in any shape or form as a default...
1. https://lore.kernel.org/git/87h7mba3h3.fsf@evledraar.gmail.com/
I could live with a version of Junio's suggestion that is not opt-in,
i.e., the checks are mandatory and exact. The important point is that
there is only one file per test case; that would still count as
"sufficiently simple" in my book.
So something where the tests in 14-15,17,19/27 would need to go back to
having the config that's part of the test driven by setup in
t/t4018-diff-funcname.sh, and the contents of a test file being e.g. (to
take the test from 25/27):
# HEADER |package main|
package main
import "fmt"
// ChangeMe
?
From: Johannes Sixt <hidden> Date: 2021-02-24 17:14:12
Am 24.02.21 um 12:12 schrieb Ævar Arnfjörð Bjarmason:
On Tue, Feb 23 2021, Johannes Sixt wrote:
quoted
I could live with a version of Junio's suggestion that is not opt-in,
i.e., the checks are mandatory and exact. The important point is that
there is only one file per test case; that would still count as
"sufficiently simple" in my book.
So something where the tests in 14-15,17,19/27 would need to go back to
having the config that's part of the test driven by setup in
t/t4018-diff-funcname.sh,
I'm afraid, I don't understand what you mean by this sentence. Do you
mean "not do the refactoring like in those patches"? Yes, that would
likely be unnecessary churn.
and the contents of a test file being e.g. (to
take the test from 25/27):
# HEADER |package main|
package main
import "fmt"
// ChangeMe
?
Yes. Except that I would move the "expected text" part to the end of the
file so that there is no danger that it is mistaken as a hunk header (in
case that a pattern is so loose that it matches that line).
-- Hannes