Skip to content

Conversation

@davepacheco
Copy link
Collaborator

There are two common problems:

  • Tests create a LogContext and never invoke cleanup_successful(). This leaves log files hanging around in $TMPDIR.
  • Tests create a ControlPlaneTestContext and never invoke teardown(). This also leaves log files hanging around in $TMPDIR. The Drop impls on CockroachInstance and ClickHouseInstance should cause those programs to shut down and their storage directories to be cleaned up, but it's not a guarantee. If not, we might leak them, using up memory in the process.

This change fixes them. I did this by looking at all the log files in $TMPDIR, which are named by what test created them, finding the test, and adding the appropriate cleanup call.


Obviously, this pattern is easy to mis-use. I'd welcome improvements here. The design goal is:

  • On success, these resources (temp directories and child processes) are automatically cleaned up.
  • On failure, the log files are kept around for post hoc debugging. (A key goal was that if a test fails in some rare case, we should still be able to debug it. This feels important for eliminating flaky tests.)

Because we want to keep these around on failure, we can't have the Drop impls clean up these files. We want to do that only if the test fails. The only way we know if it succeeds or fails is if the programmer tells us they succeeded by calling teardown()/cleanup_successful(). We could flip the default assumption so that you have to tell us when it failed, but that seems much harder to use and more error-prone.

@teisenbe
Copy link
Contributor

We might be able to simplify this for developers with using an attribute macro on the test functions that add invocations of the context creation and teardown and uses a std::panic::catch_unwind block to detect test failure. If you want me to expand on that, I'd be happy to

@david-crespo
Copy link
Contributor

We might be able to simplify this for developers with using an attribute macro on the test functions that add invocations of the context creation and teardown and uses a std::panic::catch_unwind block to detect test failure. If you want me to expand on that, I'd be happy to

Please do. This was discussed in chat (failure detection mechanism unspecified) and we agreed we would like that very much.

@davepacheco
Copy link
Collaborator Author

We might be able to simplify this for developers with using an attribute macro on the test functions that add invocations of the context creation and teardown and uses a std::panic::catch_unwind block to detect test failure. If you want me to expand on that, I'd be happy to

Agreed. We discussed something similar in chat. I think that's a good idea. I'd like to not block this PR on that though as I don't know that I'll be able to get to that any time soon. (If anyone else wants to, that'd be great!)

@davepacheco davepacheco merged commit 651ef0d into main Dec 10, 2021
@davepacheco davepacheco deleted the test-detritus branch December 10, 2021 19:46
@teisenbe
Copy link
Contributor

I could maybe throw it together, I could use an excuse to learn to write procedural macros

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants