On 8/3/2020 7:17 PM, Jonathan Nieder wrote: > Taylor Blau wrote: > >> I still only partially buy into this, though. I had to deal with some >> rather fragile grepping through trace2 JSON in t4216 (? or something, >> the log-bloom tests) recently, and found it a little fragile, but not >> overly so. >> >> I'd rather just move forward, > > This means we're promising to never change the JSON serialization, or > that we're willing to pay with time in the future to update all tests > that assume the current JSON serialization. > > I do not want to make that promise. I've lived through the same work > of updating tests that assumed particular sha1s and having to change > them to be more generic and do not want to have to do that again. > > If you're saying that you are willing to do that work when the time > comes, then that counts for a lot. If you're saying that we should > not change the JSON serialization, then it makes me worry that we made > a mistake in choosing JSON as a format, since using a well defined > format with an ecosystem of serialization + deserialization libraries > was the whole point. The benefit of JSON is that it is simple to be _additive_. We can always jam more data into the format. What I'm doing is making the following assumptions: 1. There will always be an event line corresponding to launching a subcommand. 2. This line will contain the list of arguments used for that subcommand in a JSON array. 3. We don't add extra whitespace in that JSON array. The first two items are important promises made by the trace2 event output. By depending on them, I'm only adding more places where we expect the JSON *schema* to remain consistent. Rather, I only require that the JSON schema does not _remove_ that data which is expected by tools that are reading and reporting that data. Only the third item is something that adds an additional restriction on the JSON format. But also: why would we ever add extraneous whitespace characters to the output intended for computer parsing? This concern raises a giant "YAGNI" warning in my head. What is seems like you are asking instead is for me to create a tool in the test suite that parses each JSON line, extracts a specific member from that JSON object, reconstructs a command-line invocation from the JSON array, and reports whether that process worked for any line in the event output. That seems overly complicated and puts even more requirements on the JSON schema than I have placed in my current implementation. If this is to truly be a hard requirement for these tests to move forward, then I would rather do an alternate approach that would be a better test mechanism than scraping trace data: mock the subcommands. I would create a new directory, "t/mock", and create a new 'git' executable there. By setting --exec-path in our test commands, we would launch this mocked version of Git instead of our built version. The mock could report each set of command-line arguments to a known file. With some care, we could even provide an instructions file to tell it which error code to return or whether it should write data to stdout. This would allow testing things like the "rewrite after verify fails" behavior already in the maintenance builtin. If I'm to spend time engineering something more complicated just to check "did this subcommand run with these arguments?" then I'd rather do it this way. Thanks, -Stolee
