[BenjaminPelletier]

This PR enhances the information that can be provided about resource inputs to a test suite. Instead of a dict with just resource type as the value, a list of resource inputs is added. In general, lists of items are better than dicts with arbitrary keys because the latter is difficult to deal with in a number of schema-checking activities, as I learned with ResourceCollection.resource_declarations in configuration. The former is also easier to expand with additional pieces of information later. The old resources field remains supported to allow leisurely upgrading to resource_inputs, and indeed this PR only uses the new field in one of the test suites. A description field for the test suite overall is also added and rendered into documentation.

The size of this PR is smaller than it looks initially; only the first commit is substantive, and then the second commit is just make format to rerender the auto-generated test suite documentation.

[the-glu]

The size of this PR is smaller than it looks initially; only the first commit is substantive, and then the second commit is just make format to rerender the auto-generated test suite documentation.

Shouldn't we mark those as generated in gitattributes, so the diff is not shown by default in PR, but still expandable if needed?

(If yes, probably worth and issue since it's shouldn't be fixed there)

[BenjaminPelletier]

The size of this PR is smaller than it looks initially; only the first commit is substantive, and then the second commit is just make format to rerender the auto-generated test suite documentation.

Shouldn't we mark those as generated in gitattributes, so the diff is not shown by default in PR, but still expandable if needed?

I don't think so -- seeing the differences in the autogenerated artifacts can often be useful review tools to easily determine the external effect of particular changes (these autogenerated files constitute an interface to test designers and test report consumers). It just happens to be that, in this PR, those changes are all due to a single conceptual change and so don't require separate conceptual review for each generated file changed.

[the-glu]

I don't think so -- seeing the differences in the autogenerated artifacts can often be useful review tools to easily determine the external effect of particular changes (these autogenerated files constitute an interface to test designers and test report consumers).

It wouldn't prevent anyone from seeing them, it just collapses them by default: they’re still one click away for anyone who wants to check the external effect of a change.

I don’t think there’s much value in reviewing them carefully in a PR: they’re derived entirely from the changed source values, so the meaningful review happens on those values. If the source is correct, we can reasonably trust the tools to generate the right output, and in the scope of a PR (except ones changing output), we probably don't want to check that.

Meanwhile, showing them expanded by default adds noise: large generated diffs make it harder to spot the actual changes and can lead reviewers to skim everything, including the parts that really need attention. Collapsing them keeps the focus on what humans should be reviewing, without losing the ability to inspect the generated output when it’s useful.

Anyway, that's out of scope for this PR, just a general thought as we have a lot of those, especially JSON ones.

[BenjaminPelletier]

they’re derived entirely from the changed source values, so the meaningful review happens on those values.

I'm not sure I agree with that. For instance, if I'm buying groceries and the items are $2.78, $4.29, $9.87, and $3.12, then the total of $20.06 is derived entirely from the changed source values, but it is more meaningful to review $20.06 to determine whether I will be able to pay for my groceries with a $20 bill. Certain review is sometimes valuable on inputs, certain review is sometimes valuable on outputs. In this case, I think output review is still useful because that output is what establishes the interface to users. It's useful to know that characteristics of our external interface are being changed, or when things are being added to or removed from our external interfaces. A reviewer might think they have an idea of what effect a change to an input would have on that output, but it's much more definitive to simply look at the interface.

Another example of the value of sometimes reviewing outputs is in configuration design. It can be very difficult to determine what ultimate effect a change to a .libsonnet file or a .jsonnet file will have on the ultimately-rendered object, so reviewing what effects it does, in fact, have on the rendered object is a very valuable review tool in that case as well.

I certainly don't think all generated files are valuable for a human to read during review though. For instance, the .gen.go files in the dss api package are strictly there to make Go symbols matching particular interfaces available to the rest of the code. It's very rare that we change the rendering routine, so usually I would expect those files to not need reading when they change to match an interface change.

large generated diffs make it harder to spot the actual changes and can lead reviewers to skim everything

I think we already have a good mechanism for this in practice: if a lot of lines are going to change due to a single small concept (like running make format after making some other type of change), we've often performed the substantive change in one commit, added the large amount of noise in the second commit, and noted to reviewers what each commit does and that they may want to focus on the first commit -- exactly how this PR is structured. Hopefully reviewers are not skimming everything when the description of the PR highlights an area to focus on (like the first commit).