I went through all the commits and did some cleanups. I also had a new claude session review the commits, including a careful cataloguing of old and new tests and making sure I didn't drop anything. I also introduced and verified TEST_DEBUG_SHELL as a replacement for --debug-shell.

That's a big chunk of work! Thanks!

I've had a quick look over it and there are things I do like about this, but I'd be happier if this weren't so unittest/javaesque, i.e. there's got to be some test classes for setup (what fixtures did in pytest), but some of the config tests, json or kmod could do with just test functions, making the diff smaller (and the whole thing more flat). Also, generally using the functions from barrage.assertions seems preferable to those self.assertEqual unittest things.

@behrmann cheers! Agreed to restructuring the classes wrt. fixtures, and flattening some test files.

Also, generally using the functions from barrage.assertions seems preferable to those self.assertEqual unittest things.

I don't understand this. The docs explicitly say "Assertion helpers — same names as unittest", and barrage's own tests all use self.assertEqual() and the like. It seems to me that the Assert.eq() etc. methods are just an internal implementation detail of AsyncTestCase.assert*() ?

Ok, so this might be a case of the blind leading the blind, since I've not yet really had the time to look at barrage all to closely (mea culpa), but I don't think the Assert.* are internal, since they are explicitly mentioned in the README and I think Daan might have added them, because I've mentioned how much I dislike the unittest style of tests. :) I would greatly prefer the pytest style of using plain assert, but the implementation of that is somewhat magical and too involved, which is why that was reasonable middle ground. He also referred to that style yesterday as "old style", which also indicates that the explicit Assert.* are intended for use.

@behrmann Ah! got it -- these are for test functions, as obviously they don't have a self.

Most unit tests are straightforward, I fully agree they should be functions. I already re-converted them locally.

Only test_config.py raises a little bit of doubt: The thing is, barrage really doesn't have a concept for "fixtures" in the pytest sense. We can use the AsyncExitStack functionality and move the fixtures into helpers. That amounts to one extra line per test. I'd personally use a class for this to share the setUp(), and also provide grouping for related tests. But it's not a strong opinion, and as these are existing tests, I get that it's attractive to minimize the structural diff.

I pushed a new version which moves the unit and install tests to functions. I didn't yet touch the integration tests.

@behrmann I talked to @daandemeyer (he was sitting next to me), and the executive decision was to do the Assert.* things everywhere. FWIW, I agree, for consistency -- we should just update barrage's tests and doc for that.

I moved the integration tests to functional as well. I spot-checked some of them, but I still run into io.systemd.NamespaceResource.NoDynamicRange all the time, even with --max-concurrency=1. There is clearly a bug with that still, but it doesn't affect CI (which runs as root). So let's get the bots' opinion.

There are still lots of concurrency bugs once more than one VM actually runs in parallel. Working through them.

I worked this out. This first requires a bunch of fixes for the tests to allow running them in parallel, I split that out into #4360.

Then I fixed the actual mkosi invocations to be async as well. On the latest run on my fork things now succeed, and they only take ~ 7 instead of ~ 15 minutes, i.e. actual 2x parallelism as specified in the workflow. I'm afraid we can't get higher until barrage learns about test VM resource usage and throttling itself -- test_initrd_luks takes 2 GiB, everything else 1.5 GiB, so on a 4 GiB GitHub runner concurrency=2 is the maximum that we can do reliably. But on my laptop, or if we would run this stuff on custom infra, it can go much higher.

For those following along at home: At least for me, the NoDynamicRange failures on unprivileged runs are rather unbearable (and I refuse to run builds/tests as root on my laptop). I have a commit which helps a lot, but it's a bit gross and it seems to me there is a systemd-nsresourced bug to fix instead. But just in case you play around with this locally.

@daandemeyer also sent a PR to systemd about that: systemd/systemd#42534

Rebased and adjusted for the "pass on snapshot to extension build" change that happened in the meantime. Mostly green, just this ppc64le download failure smells like qemu emulation segfault. Feel free to retry.

Otherwise ready to review.

#4373 landed, so that output directory fix needs to be re-done.

#4376 landed, so once again this is unblocked. I rebased.