Migrate image, volume and miscellaneous system tests. - #1868
Conversation
Code Coverage
|
00e8d17 to
02ae508
Compare
- Part of apple#1833. - Distributes fixture extensions for images and containers added as part of previous builder PR to more sensible locations.
02ae508 to
b09f662
Compare
| /// then removes it when `body` exits (cleanup handled by the fixture scope). | ||
| func withTempDir<T>(_ body: (URL) async throws -> T) async throws -> T { | ||
| let dir = URL(filePath: testDir.appending(UUID().uuidString).string) | ||
| try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) |
There was a problem hiding this comment.
Should delete the directory on exit?
There was a problem hiding this comment.
According to the docc, yes.
It exists in the fixture scratch directory which gets deleted on exit from the fixture scope though (unless you specify the environment variable for preserving the scratch directories).
Let me review the use cases for this helper and figure out what makes the most sense.
There was a problem hiding this comment.
This pattern resulted from literally translating the old tests to the new fixtures.
The new fixtures automatically run in a temp directory, so we can just get rid of the helper and have the tests operate directly in the scratch directory.
I noticed the same thing on the build tests but decided to leave those as is until we merged everything. Then, I've got a whole list of cleanups that remove unnecessary code like this.
| #expect(list.contains(v1) && list.contains(v2)) | ||
|
|
||
| let result = try f.run(["volume", "prune"]).check() | ||
| #expect(result.output.contains(v1) || !result.output.contains("No volumes to prune")) |
There was a problem hiding this comment.
Shouldn't it check result.output.contains(v1) only?
There was a problem hiding this comment.
Prune should delete both v1 and v2. Checking for those should suffice.
The "no volumes" thing is weird. I'm not sure why the absence of "No volumes" passes the test if the output doesn't contain `v1. Let's remove those.
| } | ||
| } | ||
|
|
||
| @Test func testAnonymousVolumeDetachedMode() async throws { |
There was a problem hiding this comment.
Not critical, but the original test was setting autoRemote: true so that container is deleted on stop. New version slightly changes the test semantics.
There was a problem hiding this comment.
Good catch...updated to match the old test.
| .relativePath | ||
|
|
||
| try await ContainerFixture.with { f in | ||
| f.addCleanup { _ = try? f.run(["system", "kernel", "set", "--recommended", "--force"]) } |
There was a problem hiding this comment.
Should catch and log if this fails? because it affects following tests.
There was a problem hiding this comment.
Agree. Changed to record test issues on non-zero status or throw.
| let c1 = "\(f.testID)-c1" | ||
| let c2 = "\(f.testID)-c2" | ||
| try f.doPull(alpine) | ||
| let image = try f.copyWarmupImage(alpine) |
There was a problem hiding this comment.
Why do we need to make copies of the alpine image for these tests?
There was a problem hiding this comment.
We don't. Claude got a little overexuberant I think. We only need to make a copy if we intend to delete the image as part of the test.
- Part of apple#1833. - Distributes fixture extensions for images and containers added as part of previous builder PR to more sensible locations.
Type of Change
Motivation and Context
Improve integration test reliability.
Testing