| Message ID | 20230328202207.GA1241631@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | some chainlint fixes and performance improvements | expand |
On Tue, Mar 28 2023, Jeff King wrote: > Since 69b9924b875 (t/Makefile: teach `make test` and `make prove` to run > chainlint.pl, 2022-09-01), we run a single chainlint.pl process for all > scripts, and then instruct each individual script to run with the > equivalent of --no-chain-lint, which tells them not to redundantly run > the chainlint script themselves. > > However, this also disables the internal linter run within the shell by > eval-ing "(exit 117) && $1" and confirming we get code 117. In theory > the external linter produces a superset of complaints, and we don't need > the internal one anymore. However, we know there is at least one case > where they differ. A test like: > > test_expect_success 'should fail linter' ' > false && > sleep 2 & > pid=$! && > kill $pid > ' > > is buggy (it ignores the failure from "false", because it is > backgrounded along with the sleep). The internal linter catches this, > but the external one doesn't (and teaching it to do so is complicated[1]). > So not only does "make test" miss this problem, but it's doubly > confusing because running the script standalone does complain. > > Let's teach the suppression in the Makefile to only turn off the > external linter (which we know is redundant, as it was already run) and > leave the internal one intact. > > I've used a new environment variable to do this here, and intentionally > did not add a "--no-ext-chain-lint" option. This is an internal > optimization used by the Makefile, and not something that ordinary users > would need to tweak. > > [1] For discussion of chainlint.pl and this case, see: > https://lore.kernel.org/git/CAPig+cQtLFX4PgXyyK_AAkCvg4Aw2RAC5MmLbib-aHHgTBcDuw@mail.gmail.com/ > > Signed-off-by: Jeff King <peff@peff.net> > --- > t/Makefile | 2 +- > t/test-lib.sh | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/t/Makefile b/t/Makefile > index 88fa5049573..10881affdd0 100644 > --- a/t/Makefile > +++ b/t/Makefile > @@ -45,7 +45,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl > # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`) > # checks all tests in all scripts via a single invocation, so tell individual > # scripts not to "chainlint" themselves > -CHAINLINTSUPPRESS = GIT_TEST_CHAIN_LINT=0 && export GIT_TEST_CHAIN_LINT && > +CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && > > all: $(DEFAULT_TEST_TARGET) > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 62136caee5a..09789566374 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -1593,7 +1593,8 @@ then > BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true" > fi > > -if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 > +if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 && > + test "${GIT_TEST_EXT_CHAIN_LINT:-1}" != 0 > then > "$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0" || > BUG "lint error (see '?!...!? annotations above)" So, we used to use the "sed" script to run against the *.sh, and do the "eval 117" check, but since chainlint.pl we've checked all the tests in one go when invoked from the Makefile, and turned off the "eval 117". And, as this change points out, there's cases where chainlint.pl won't catch everything, so we'd like to keep the "eval" bits by default, but not the invoking of chainlint.pl by the script itself. All of which makes sense. But it seems to me that the findings in this change make the docs outdated, and we should update them, but maybe I'm missing something. Now the comment in the Makefile still says (as seen in the context): # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`) # checks all tests in all scripts via a single invocation, so tell individual # scripts not to "chainlint" themselves But as the commit message notes that's not accurate anymore. We're *not* telling them to turn off chainlint in its entirety, we're telling them to only suppress the chainlint.pl part. So if you invoke the Makefile with GIT_TEST_CHAIN_LINT=0 we'll still turn all of it off, but an invokation of chainlint.pl for the scripts as a batch is no longer thought to make the "eval 117" trick redundant. I haven't looked too deeply, but it seems that we should at least adjust that comment in the Makefile, or perhaps we should rename this "eval 117" thing to be "internal lint" or something, and not "chain lint", to avoid conflating it with chainlint.pl itself. So maybe something like this, i.e. we'd just "rebrand" this to begin with, and avoid editing the t/Makefile at all (untested)? t/README | 12 ++++++++++++ t/test-lib.sh | 6 +++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/t/README b/t/README index 29576c37488..83d58f2e680 100644 --- a/t/README +++ b/t/README @@ -196,6 +196,18 @@ appropriately before running "make". Short options can be bundled, i.e. this feature by setting the GIT_TEST_CHAIN_LINT environment variable to "1" or "0", respectively. +--internal-lint:: +--no-internal-lint:: + If --internal-lint is enabled, the test harness will try to + detect missing "&&"-chains in edge cases that "--chain-lint" + wouldn't notice, unlike "--chain-lint" this check is run from + within "test_expect_*" itself, and thus requires the tests to + run. + + You may also enable or disable this feature by setting the + GIT_TEST_INTERNAL_LINT environment variable to "1" or "0", + respectively. + --stress:: Run the test script repeatedly in multiple parallel jobs until one of them fails. Useful for reproducing rare failures in diff --git a/t/test-lib.sh b/t/test-lib.sh index 62136caee5a..792c68aa56a 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -205,6 +205,10 @@ parse_option () { GIT_TEST_CHAIN_LINT=1 ;; --no-chain-lint) GIT_TEST_CHAIN_LINT=0 ;; + --internal-lint) + GIT_TEST_INTERNAL_LINT=1 ;; + --no-internal-lint) + GIT_TEST_INTERNAL_LINT=0 ;; -x) trace=t ;; -V|--verbose-log) @@ -1090,7 +1094,7 @@ test_run_ () { test_cleanup=: expecting_failure=$2 - if test "${GIT_TEST_CHAIN_LINT:-1}" != 0; then + if test "${GIT_TEST_INTERNAL_LINT:-1}" != 0; then # turn off tracing for this test-eval, as it simply creates # confusing noise in the "-x" output trace_tmp=$trace
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Now the comment in the Makefile still says (as seen in the context): > > # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`) > # checks all tests in all scripts via a single invocation, so tell individual > # scripts not to "chainlint" themselves > > But as the commit message notes that's not accurate anymore. We're *not* > telling them to turn off chainlint in its entirety, we're telling them > to only suppress the chainlint.pl part. Correct. To avoid a confusion we saw in the thread that led to this series, we need an explanation that clearly distinguishes the "exit 117" one and the "script that parses the shell" one. If we consider that the name "chainlint" refers to the latter, the script, and re-read the three lines with that in mind, I think they are OK. It would become even clearer if we insert four words, like so: `test-chainlint' (which is ...) checks ... via a single invocation of the "chainlint" script, so tell ... > So if you invoke the Makefile with GIT_TEST_CHAIN_LINT=0 we'll still > turn all of it off, but an invokation of chainlint.pl for the scripts as > a batch is no longer thought to make the "eval 117" trick redundant. Yes, I think that is the idea reached by the discussion in the other thread that led to this series. > I haven't looked too deeply, but it seems that we should at least adjust > that comment in the Makefile, or perhaps we should rename this "eval > 117" thing to be "internal lint" or something, and not "chain lint", to > avoid conflating it with chainlint.pl itself. I agree that it would help to clarify that we mean by "chainlint" the script that parses (or the act of running the script, when it is used as a verb), and the above may be a way to do so. I however do not see the need to come up with a new name for the other one, until we have a way to toggle it enabled/disabled, which is not something the discussion in the other thread found necessary. Thanks.
On Wed, Mar 29, 2023 at 08:49:07AM -0700, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > Now the comment in the Makefile still says (as seen in the context): > > > > # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`) > > # checks all tests in all scripts via a single invocation, so tell individual > > # scripts not to "chainlint" themselves > > > > But as the commit message notes that's not accurate anymore. We're *not* > > telling them to turn off chainlint in its entirety, we're telling them > > to only suppress the chainlint.pl part. > > Correct. To avoid a confusion we saw in the thread that led to this > series, we need an explanation that clearly distinguishes the "exit > 117" one and the "script that parses the shell" one. If we consider > that the name "chainlint" refers to the latter, the script, and > re-read the three lines with that in mind, I think they are OK. It > would become even clearer if we insert four words, like so: > > `test-chainlint' (which is ...) checks ... via a single > invocation of the "chainlint" script, so tell ... I had hoped that "chainlint" in that comment would remain sufficient, as the context implies that we're disabling the script. But it's easy enough to expand. I squashed this in: diff --git a/t/Makefile b/t/Makefile index 10881affdd0..3e00cdd801d 100644 --- a/t/Makefile +++ b/t/Makefile @@ -44,7 +44,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`) # checks all tests in all scripts via a single invocation, so tell individual -# scripts not to "chainlint" themselves +# scripts not to run the external "chainlint.pl" script themselves CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && all: $(DEFAULT_TEST_TARGET) > > I haven't looked too deeply, but it seems that we should at least adjust > > that comment in the Makefile, or perhaps we should rename this "eval > > 117" thing to be "internal lint" or something, and not "chain lint", to > > avoid conflating it with chainlint.pl itself. > > I agree that it would help to clarify that we mean by "chainlint" > the script that parses (or the act of running the script, when it is > used as a verb), and the above may be a way to do so. I however do > not see the need to come up with a new name for the other one, until > we have a way to toggle it enabled/disabled, which is not something > the discussion in the other thread found necessary. Yeah. This distinction is not something anybody who is running or writing tests should have to worry about (and the fact that it did come up was a bug that this patch is fixing). So adding an option like "--no-chain-lint-internal" is just making things more confusing, and nobody would use it. You'd need it only if you are working on chainlint.pl itself (and comparing results with the internal linter or something), and there you are better off running "perl chainlint.pl t1234-foo.sh" itself. -Peff
Jeff King <peff@peff.net> writes: > I had hoped that "chainlint" in that comment would remain sufficient, as > the context implies that we're disabling the script. But it's easy > enough to expand. I squashed this in: > > diff --git a/t/Makefile b/t/Makefile > index 10881affdd0..3e00cdd801d 100644 > --- a/t/Makefile > +++ b/t/Makefile > @@ -44,7 +44,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl > > # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`) > # checks all tests in all scripts via a single invocation, so tell individual > -# scripts not to "chainlint" themselves > +# scripts not to run the external "chainlint.pl" script themselves OK. I've taken it and did "rebase -i" on this end. Thanks.
diff --git a/t/Makefile b/t/Makefile index 88fa5049573..10881affdd0 100644 --- a/t/Makefile +++ b/t/Makefile @@ -45,7 +45,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`) # checks all tests in all scripts via a single invocation, so tell individual # scripts not to "chainlint" themselves -CHAINLINTSUPPRESS = GIT_TEST_CHAIN_LINT=0 && export GIT_TEST_CHAIN_LINT && +CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && all: $(DEFAULT_TEST_TARGET) diff --git a/t/test-lib.sh b/t/test-lib.sh index 62136caee5a..09789566374 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1593,7 +1593,8 @@ then BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true" fi -if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 +if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 && + test "${GIT_TEST_EXT_CHAIN_LINT:-1}" != 0 then "$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0" || BUG "lint error (see '?!...!? annotations above)"
Since 69b9924b875 (t/Makefile: teach `make test` and `make prove` to run chainlint.pl, 2022-09-01), we run a single chainlint.pl process for all scripts, and then instruct each individual script to run with the equivalent of --no-chain-lint, which tells them not to redundantly run the chainlint script themselves. However, this also disables the internal linter run within the shell by eval-ing "(exit 117) && $1" and confirming we get code 117. In theory the external linter produces a superset of complaints, and we don't need the internal one anymore. However, we know there is at least one case where they differ. A test like: test_expect_success 'should fail linter' ' false && sleep 2 & pid=$! && kill $pid ' is buggy (it ignores the failure from "false", because it is backgrounded along with the sleep). The internal linter catches this, but the external one doesn't (and teaching it to do so is complicated[1]). So not only does "make test" miss this problem, but it's doubly confusing because running the script standalone does complain. Let's teach the suppression in the Makefile to only turn off the external linter (which we know is redundant, as it was already run) and leave the internal one intact. I've used a new environment variable to do this here, and intentionally did not add a "--no-ext-chain-lint" option. This is an internal optimization used by the Makefile, and not something that ordinary users would need to tweak. [1] For discussion of chainlint.pl and this case, see: https://lore.kernel.org/git/CAPig+cQtLFX4PgXyyK_AAkCvg4Aw2RAC5MmLbib-aHHgTBcDuw@mail.gmail.com/ Signed-off-by: Jeff King <peff@peff.net> --- t/Makefile | 2 +- t/test-lib.sh | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)