Re: [PATCH v3 0/3] Add a JSON Schema for trace2 events

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 2, 2019, 16:59 UTC
Message-ID: <20190802165913.GA109863@google.com>
In-Reply-To: <nycvar.QRO.7.76.6.1908021347580.46@tvgsbejvaqbjf.bet>

Hi,

Johannes Schindelin wrote:

> On Thu, 1 Aug 2019, Jonathan Nieder wrote:

Show 5 quoted lines

>> What do you think of making the validation disabled by default and
>> using a parameter (see "Running tests with special setups" in
>> t/README) to turn it on?  That way, it should be okay for it to take
>> 10 minutes because this would only affect such specialized workers as
>> choose to set that parameter, instead of all of them.

[...]

Show 9 quoted lines

> So how about adding a separate test script that activates Trace2, then
> runs a _few_ selected, very specific commands, starting with a set that
> Josh thinks representative, validate the generated JSON, and then only
> add new invocations if we *ever* see violations of the schema, to verify
> that we don't regress on that front again?
>
> Such a script can't take more than a couple of seconds to run, I am
> sure. And it would totally strike a sensible balance between benefit and
> time spent.

I think this suggestion has been covered a little upthread, but apologies for not paying it the fuller attention that it deserves.

Such a test would check that

* the validation code still works, the schema is well formed, etc
* traces produced by common commands fit the schema

That makes it very likely the right thing to do. This is a test that's cheap to run, so we can make it happen automatically as part of a normal test suite run for any developer that has ajv installed (including various CI jobs). And it unblocks getting patches 1 and 2 from this series in. So, basically, you're right. :)

That said, it still leaves an unmet need that is important to us.

We cannot trust the JSON output and rely on it without a more exhaustive test that the schema is accurate. It is very easy to add a new field or event type that only shows up in a specialized code path, without remembering to update the schema or docs, because of the nature of how the trace2 API works.

In this thread, you can already see some of the fruit of this more exhaustive approach:

- it identifies the tests that write invalid UTF-8 to the traces,
  illustrating an issue with the format (i.e., a real bug) that we
  can now be more aware of

- it identified a field that had been renamed in code where we had
  forgotten to update the documentation

The exhaustive approach really helps. Arguing against it kind of feels like saying "leak checkers are great, but why run one on the full test suite instead of a dedicated tool that exercises the code paths where you expect to find leaks?"

In the short term, we can run tests internally to check that Git keeps following the schema. Let's not block patches 1 and 2 by this --- instead, the simple basic functionality test you're describing seems like a good way to make progress for the moment.

We can upstream it later.

Thanks, Jonathan