Clean up TestContext when pack build unexpectedly succeeds

[colincasey]

Moves the creation of the TestContext to before the invocation of run_pack_command so that the context will be cleaned up regardless of the outcome.

@fixes #586

[schneems]

Your PR looks good in terms of I think it does what it says it does. I'm wanting to make it so that it doesn't regress in the future i.e. if someone were to re-order the commands in a future PR tests wouldn't fail.

That's the "changes requested": To encode the fix in the build/test logic (through tests, types, or both).

We could (brainstorm):

• Add a test for this explicit behavior (probably hard and/or expensive)
• Couple run_pack_command with needing a TestContext (even if it's not used) to force it's creation before it's run or possibly introduce another function
• Move the creation of the TestContext to the same location that allocates the resource (RAII)
• Couple the creation of the image name with the creation of something that tears it down.

Ideally I want to move the logic closest to where the resource is allocated. In this case we decouple creating an image name from allocating/generating that image (via pack). I'm proposing to couple the teardown logic of the image name along with image name creation so then there's never a scenario where one would be created and not torn down.

Sketch

• Introduce a ImageName struct that holds, well, an image name.
• Have util::random_docker_identifier() return this new struct instead of a string
• Move the drop logic to this struct ImageName struct
• Add a new field to TestContext to hold an ImageName

Now when TestContext is dropped it will drop it's contents and call drop on ImageName triggering the cleanup (existing behavior is preserved). And since ImageName is allocated before run_pack_command is ever called, if it fails then ImageName drop behavior will be called (new behavior).

This assumes that the drop logic is a no-op if an image name has not been created in docker.

[Malax]

@edmorley does #619 also resolve this issue? In any case, this PR here won't apply cleanly anymore after run_pack_command changed so much.:/

[edmorley]

@Malax No #619 does not fix this directly, though it does give more options here as to how to fix this issue.

It seems the options here now are to:

1. (This is now only possible after libcnb-test: Refactor run_pack_command and improve panic messages #619) Make an explicit call to remove the Docker image on the "successful but panicing" branch here:

   libcnb.rs/libcnb-test/src/test_runner.rs

   Lines 170 to 171 in 346a186

   	(PackResult::Failure, Ok(LogOutput { stdout, stderr })) => {
   	panic!("The pack build was expected to fail, but did not:\n\n## stderr:\n\n{stderr}\n## stdout:\n\n{stdout}\n");

2. (As Richard suggested above) Add a new Image (or similar) struct, which has the Drop on it itself, which can be created sooner than the TestContext - and will handle the image removal itself, rather than on TestContext drop.
3. Move the creation of TestContext earlier (the current PR's implementation), but add a comment explaining why.

I think my preference is for option (1) or (2).

Thoughts?

[Malax]

Thoughts?

I would vote for option 1 here. I find Drop complicates the code and makes it hard to reason about. If we can avoid using it I think we should.

[edmorley]

I would vote for option 1 here. I find Drop complicates the code and makes it hard to reason about. If we can avoid using it I think we should.

Works for me! I've opened #625.