Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test updates: favor cleanups over defers, and only log from cleanup methods #2043

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

zackattack01
Copy link
Contributor

A few improvements after reading more about best practices and parsing confusing test output:

  • we should try to favor t.Cleanup() over defer in tests, especially where we run in parallel
  • we should try to keep actual tests out of cleanup methods

This has a few small changes to accomplish this within the runtime tests:

  • adds an ensureShutdownOnCleanup helper function to ensure even failing tests call shutdown on the runner. this function does not act as a test, but will log additional info if there is an unexpected failure during shutdown
    • it is safe to use this alongside waitShutdown, and the differences should be well documented in the code
    • the hope here is that we will stop getting in situations where an early failure leads to no shutdown being triggered which leads to failure to cleanup the test root directory, and noisy output
  • removes the teardown func that we were previously returning from setupOsqueryInstanceForTests. tests should test the shutdown methodology with waitShutdown (happy path), and rely on the shutdown from ensureShutdownOnCleanup if anything goes wrong before we get a chance to waitShutdown
  • updates the cleanup func from testRootDirectory to log any issues, but not fail the test itself to avoid any confusion (failures here should only be a symptom of some previous issue)

@zackattack01 zackattack01 marked this pull request as ready for review January 10, 2025 21:01
@zackattack01 zackattack01 added this pull request to the merge queue Jan 10, 2025
Merged via the queue into main with commit 96b7cff Jan 10, 2025
33 checks passed
@zackattack01 zackattack01 deleted the zack/runtime_tests2 branch January 10, 2025 22:04
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.

3 participants