Have CI check for diffs after integration tests

[sellout]

Overview

There are two ways for transcripts to fail – with an error, or by having output different from what is committed. The integration tests don’t currently check for the latter.

Implementation notes

I also had to make it so modifying ci.yaml caused a cache miss for ci.yaml – which makes sense. If we change the workflow, we want to make sure that the changes to it are correct, which using the cache would bypass.

Test coverage

This PR improves the coverage of the integration tests by checking state that was overlooked.

[aryairani]

Awesome, thanks @sellout

[aryairani]

@sellout

Not sure where the best place to have this conversation is, but I'll put it here.

So I'm reluctant to include **/ci.yaml here, even though it's certainly more correct, because it causes any (possibly unrelated) change to the ci process — and there's often dozens of these when trying to add or modify any feature — take 45-60+ mins to evaluate.

Any ideas for how to best deal with that?

[sellout]

From the commit log, this file changes like three times a month, but that overlooks how many commits may have been pushed (and possibly edited) but never made it into trunk. So yeah, I can see it being churny within any of those changes, as you mostly have to iterate on CI directly.

I think the first thing to consider is whether it can be broken into independent workflows. It looks like there’s a legit dependency graph there, so I guess it’s unlikely (but, as an example, here it is before I moved the ormolu job a couple months ago).

Extracting more complicated CI steps into scripts that can easily be run locally can help reduce the churn of CI files, since they can more easily be tested & iterated on locally. But then those scripts should also be part of the hash, or else it just moves the same problem. And I am a fan of small jobs, but combining sequences of jobs into one can make this easier, too. E.g., on this run, it looks like “generate jit source”, “build jit binary”, and “test jit” are a sequence. They’re not quite, because “generate jit source” is one job, and then “build jit binary” is a matrix that all depend on that. But “built jit binary” and “test jit” seem like they could reasonably be a single job.

It might just be best to just leave **/ci.yaml out, with a comment in case someone else (or me next month) comes back to add it again. That’s probably enough. If there’s some way to clear a particular cache entry¹, that could also be mentioned in the comment. But I think mostly people won’t notice. I only did because I made a change that I expected to cause a CI failure (before pushing the fix), and it didn’t fail.

Of course, there’s also just performance improvement, which is nice because it generally helps developers and/or users, too, but that’s usually motivated the other way around – because users or developers are frustrated.

Footnotes

1. Maybe a manually-triggerable workflow? But then that key value needs to be kept in sync between multiple workflows, which seems just likely to re-introduce the issue that Revert "Have CI check for diffs after integration tests" #5801, Revert "Revert "Have CI check for diffs after integration tests"" #5802, & don't use actions/cache to share artifacts between jobs #5804 are solving. Unless CI moves to some templating approach, which is a bigger lift. ↩