Blueprint planner: clean up pending MGS update test helpers

[jgallagher]

This is mostly because I found the existing support stuff pretty unwieldy when I went to try to extend it to do host OS updates too; e.g., this return type

    fn test_collection_config() -> BTreeMap<
        (SpType, u16),
        (&'static str, &'static str, &'static str, &'static str),
    > { .. }

and the arguments to this function

    fn make_collection(
        active_version: ArtifactVersion,
        active_version_exceptions: &BTreeMap<(SpType, u16), ArtifactVersion>,
        inactive_version: ExpectedVersion,
        active_rot_version: ArtifactVersion,
        active_rot_version_exceptions: &BTreeMap<
            (SpType, u16),
            ArtifactVersion,
        >,
        inactive_rot_version: ExpectedVersion,
    ) -> Collection { .. }

are already pretty rough, and I was only going to make that worse by adding host OS stuff to them.

I took a stab at pulling this out and putting some names on things; I'm sure there's more that could be done (and we may want to do that as we add host OS and bootloader tests), but figured this was a decent stopping point.

The changes here are mostly refactoring. I did add one extra assertion: we had a couple tests that would build a set of expected_updates, then confirm all the updates produced by planning were present in expected_updates. But they weren't confirming that we'd seen all the expected updates. When I added this assertion, the test_whole_system_simultaneous() test failed. I think this is because we can't actually update the whole system simultaneously: we do at most one kind of update to a given board at once. I reworked this test some to "update all the RoTs" then "update all the SPs" instead.

In terms of reviewing, I'd probably look mostly at the reworked tests themselves and see if these changes are a clarity improvement. The changes there are a lot smaller than the diff, since all of the supporting code moved around. This will conflict with #8664; I'm happy to do the work to merge those together. (It looks like we had similar ideas; e.g., about make_collection() not really being sustainable as it was written!)

I think with these changes we could also move the respective SP / RoT tests into their submodules instead of leaving them all in the higher-level mod.rs.

[karencfv]

This looks sooooo nice! Thanks a bunch for cleaning up the tests! It's way easier to understand what's going on now.

I merged the RoT bootloader planner PR, which breaks this. If it's too annoying to deal with, perhaps we can remove those tests temporarily and add them after we merge this? WDYT?

[karencfv]

Ah yes, this is waaaaayyy nicer ✨

[karencfv]

Maybe we should change the name of the test at this point? Something like test_whole_system_simultaneous_per_component or something similar?

[jgallagher]

Hmm, I agree the current name is misleading, but simultaneous_per_component is kind of a mouthful. Just thinking out loud: The test is showing what we'd do if we tried to update as much as we can in one planning iteration. How about test_allow_simultaneous_updates()? (Or "concurrent" instead of "simultaneous"?)

I added a TODO that this test shows we would do the wrong thing w.r.t. bootloaders if allowed to stage multiple updates simultaneously. I don't think fixing it is super urgent (since in production we don't allow staging multiple updates simultaneously); I could file a new issue or add a comment to and reopen #7819?

[karencfv]

How about test_allow_simultaneous_updates()? (Or "concurrent" instead of "simultaneous"?)

Sounds good! Any variant is fine with me.

I don't think fixing it is super urgent (since in production we don't allow staging multiple updates simultaneously)

@davepacheco: Maybe this is me not getting it, but thinking about it, if we don't allow simultaneous updates in production, then why are we testing that we can do them?

[karencfv]

Hm, I haven't used this crate before. I should take a good look at it! These tests are cleaning up nicely

[jgallagher]

Yeah, this crate is great. It's @sunshowers's extension of the idmap crate I threw together for maps where the key is derived from the value (but critically, iddqd allows keys that borrow from the values; idmap didn't).

[jgallagher]

I merged the RoT bootloader planner PR, which breaks this. If it's too annoying to deal with, perhaps we can remove those tests temporarily and add them after we merge this? WDYT?

I addressed all the conflicts, hopefully correctly. It wasn't too bad, but is probably worth a second look?

Having to call three methods to set the default versions for SP, RoT, and bootloader was getting kinda unwieldy itself, so I also collapsed those in 97aa18f for the common case where all three are using the same default active and inactive versions.

[davepacheco]

As you suggested, I've only skimmed the changes to the tests. Looks like a huge improvement! Thanks for doing it.

[karencfv]

Looks great! Thanks for all the clean up

[karencfv]

How about test_allow_simultaneous_updates()? (Or "concurrent" instead of "simultaneous"?)

Sounds good! Any variant is fine with me.

I don't think fixing it is super urgent (since in production we don't allow staging multiple updates simultaneously)

@davepacheco: Maybe this is me not getting it, but thinking about it, if we don't allow simultaneous updates in production, then why are we testing that we can do them?