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

Give ShotoverProcess powerful event assertions by parsing JSON events #950

Merged
merged 1 commit into from Dec 20, 2022

Conversation

rukai
Copy link
Member

@rukai rukai commented Dec 6, 2022

Goal

The goal of this PR is to allow making assertions on what tracing events occur during integration tests.
At first I wanted to do this with ShotoverManager by routing tracing events directly into a Vec.
However after thinking about it I realized there were a lot of advantages to instead using ShotoverProcess for this.

Advantages to replacing ShotoverManager with ShotoverProcess

  • events are isolated within each shotover instance so we can e.g. assert that each shotover instance logs the same error once.
  • more accurate shotover usage:
    • Shotover is gauranteed to shutdown when ShotoverProcess is dropped. Currently ShotoverManager still leaves tasks and resources alive after it is dropped.
    • No risk of cross contamination of state (e.g. global values) between running shotover instances and test logic
    • No API hacks, particularly at startup time, we only get access to APIs that are included in an actual binary.
      • The one exception is the alpha-transforms feature which will remain enabled in tests but not released binaries.
    • The binary that we test is closer to the real final binary that we ship. Its possible for optimizations/codegen to occur differently when called within a test binary.
  • possibly better iterative buildtimes
    • we can change just shotover or just the tests without having to recompile both into a single binary.
    • this wont take affect until we completely remove ShotoverManager though
  • Can remove a bunch of #[cfg(feature = "alpha-transforms")] because the binary is always compiled with --all-features
  • Due to the removal of a circular dependency, we no longer need #[path ...] hacks in order to import tests/helpers from test and bench binaries.
    • Instead tests/helpers can live properly in the test-helpers crate
  • We only need to maintain a single shotover helper, just ShotoverProcess instead of both ShotoverManager and ShotoverProcess
  • Logs are kept hidden unless the test is run with --show-output

The only disadvantage I can think of is losing the strong typing in the boundary between tests and shotover i.e. we go from calling rust code to executing a binary

Advantages to implementing event assertions on ShotoverProcess instead of ShotoverManager:

  • Our integration tests can process and format events however it likes, without having to worry about the formatting being usable for users.
    • Currently I am taking advantage of this by putting some basic coloring on our stack traces.
  • Our assertion that no panics occurred in shotover can be nicely treated as yet another case of our event assertions.
    • Panics are sent as error events in json logging mode, so we just get this for free.
  • Better support for concurrent tests, we wouldn't be able to run tests concurrently with the ShotoverManager approach due to tracing's global state - cant make use of it without huge refactors elsewhere though
  • Implementing it via ShotoverManager is just really complicated and there werent any good examples on how to do it, so it was hard to justify figuring it out when we should really replace ShotoverManager with ShotoverProcess anyway.

The only disadvantage I can think of is that we cant assert on logs from the test logic, but tbh thats acceptable because most of the time we want to assert on Shotovers events not the tests.

Approach

I renamed ShotoverProcess to BinProcess and moved it into a new crate tokio-bin-process.
The goal is to have it as a reusable crate that other projects can take advantage of.
To that end I have kept anything shotover specific out of that crate, and then made some small shotover specific wrappers in test-helpers/shotover_process.rs
Once we are happy with tokio-bin-process for our own usage we should move it into its own repo, for now keeping it in shotover-proxy helps us move quickly.

test_shotover_shutdown_when_topology_invalid_topology_subchains is a good example of what it looks like when we have some warnings/errors to allow.

A BinProcess enforces that it is properly shutdown in its drop implementation.
To properly shutdown a BinProcess the users calls a shutdown method such as shutdown_and_then_consume_events.
When this occurs BinProcess asserts that no error or warning events occurred.
If the user specifically desires that a warning or error occurs then they can provide EventMatcher's to shutdown_and_then_consume_events to describe which events they want to allow.
It is quite flexible but we will want developers to be as specific as possible when writing EventMatcher's

I would just review all the tokio-bin-process code from scratch as it has changed so drastically from the original ShotoverProcess code.

Future work

Future work can be split up into the following PRs:

  • replace ShotoverManager with ShotoverProcess in cassandra_int_tests (Remove unneeded #[cfg(feature = "alpha-transforms")] )
  • replace ShotoverManager with ShotoverProcess in redis_int_tests
  • Remove ShotoverProcess and remove all #[path ...] hacks in tests and benchmarks.

@rukai rukai marked this pull request as draft December 6, 2022 07:08
@rukai rukai force-pushed the assert_on_tracing branch 8 times, most recently from ded0f09 to 54b964c Compare December 8, 2022 07:50
@rukai rukai force-pushed the assert_on_tracing branch 10 times, most recently from 7c820f6 to 3055b76 Compare December 13, 2022 22:59
@rukai rukai marked this pull request as ready for review December 14, 2022 00:46
@rukai rukai merged commit 145465e into shotover:main Dec 20, 2022
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.

None yet

3 participants