oxidecomputer / dropshot

expose REST APIs from a Rust program
Apache License 2.0
874 stars 76 forks source link

test_util: Add meaningful names to test LogContexts #900

Open hawkw opened 9 months ago

hawkw commented 9 months ago

Currently, when a test creates a new LogContext, a numeric counter is incremented every time a test creates a new LogContext, and the resultant path will be of the form {test_name}.{pid}.{id}.log. The counter is useful as it ensures that multiple LogContexts can coexist within a single test execution. However, it doesn't really help with identifying what entity in the test created that log file. Instead, this must be inferred by reading the test code to determine the order in which LogContexts are created, which can sometimes be challenging --- especially when there's a bunch of test support code spinning up multiple Dropshot servers.

It would be nice if there was a way to add a human-readable test component identifier to these log paths, like "server" or "client". The counter could still be used in addition to such an additional identifier to ensure all paths are unique.

davepacheco commented 9 months ago

Hmm. When this was created, the intent was to put the actual test name there. In practice, it's quite often not that anyway because:

I'm wondering if we should just treat this as an "arbitrary caller-provided label", which is really what it is anyway. Then change the callers to provide better names.

Related but tangential: we've long wanted a #[log_context] kind of test macro sort of like nexus_test but that gives your test function a LogContext and also cleans it up on success. If we had that, the logs could more reliably match the test name. But I still think it makes more sense to think of the name as a log label and not "the name of the test being run".

What do folks think?