Re: [PATCH v4] branch: introduce --show-current display option

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 18, 2018, 14:19 UTC
Message-ID: <CAPig+cRbH0xQDg-acGnHs5cAQKueaAfPFF+AXUF2Gq75KqNupg@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1810181146250.4546@tvgsbejvaqbjf.bet>

On Thu, Oct 18, 2018 at 5:51 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 18 quoted lines

> On Wed, 17 Oct 2018, Eric Sunshine wrote:
> > On Wed, Oct 17, 2018 at 6:18 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > My suspicion: it is essentially the `(exit 117)` that adds about 100ms to
> > > every of those 67 test cases.
> >
> > The subshell chain-linter adds a 'sed' and 'grep' invocation to each test which doesn't help. (v1 of the subshell chain-linter only added a 'sed', but that changed with v2.)
> > You could disable the subshell chain-linter like this if you want test the (exit 117) goop in isolation:
>
> You're right! This is actually responsible for about five of those seven
> seconds. The subshell still hurts a little, as it means that every single
> of the almost 20,000 test cases we have gets slowed down by ~0.03s, which
> amounts to almost 10 minutes.
>
> This is "only" for the Windows phase of our Continuous Testing, of course.
> Yet I think we can do better than this.
>
> How difficult/involved, do you think, would it be to add a t/helper/
> command for chain linting?

Probably more effort than it's worth, and it would only save one process invocation.

Since the subshell portion of the chain-linting is done by pure textual inspection, an alternative I had considered was to just perform it as a preprocess over the entire test suite, much like the other t/Makefile "test-lint" targets. In other words, the entire test suite might be tested in one go with something like this:

    sed -f chainlint.sed t*.sh | grep -q '?![A-Z][A-Z]*?!' &&
        echo "BROKEN &&-chain"

That won't work today since chainlint.sed isn't written to understand everything which we might see outside of a test_expect_*, but doing it that way is within the realm of possibility. There were two reasons why I didn't pursue that approach.

First, although I was expecting Windows folks to complain (or at least speak up) about the extra 'sed' and 'grep', nobody did, so my impression was that those two extra commands were likely lost in the noise of the rest of the boilerplate commands invoked by test_expect_success(), test_run_(), test_eval_(), etc., and by whatever expensive commands are invoked by each test itself. Second, the top-level &&-chain "(exit 117)" linting kicks in even when you run a single test script manually, say after editing a test, which is exactly when you want to discover that you botched a &&-chain, so it seemed a good idea for the subshell &&-chain linter to follow suit. The t/Makefile "test-lint" targets, on the other hand, don't kick in when running test scripts in isolation.

However, a pragmatic way to gain back those 10 minutes might be simply to disable the chain-linter for continuous integration testing on Windows, but leave it enabled on other platforms. This way, we'd still catch broken &&-chains, with the exception of tests which are specific to Windows, of which I think there are very few.