Test refactor proposal #909

Jake-Shadle · 2024-03-15T16:06:56Z

This is a first pass at showing a different way to write integration tests, ie, tests that use multiple components together.

An incremental step at changing APIs and tests either binding sockets ahead of time that are then passed to the APIs, or for the proxy case, not binding and using the port returned from run_server in most cases. While the available_addr function is still used, this is first step at completely removing it. While it works in most cases, it's actually not guaranteed to be 100% reliable as there is nothing stopping the OS of assigning the port to another call causing a bind failure for the second test to use it, so eventually removing this will entirely remove bind-related flakiness.
Moves the logic of spinning up the various components into separate pieces from the CLI interface to them. The goal of this change is to start to tease apart the user interface (CLI) portions from the actual code that is run, to make writing tests easier. This also completely removes the notion of unwrapping the RuntimeConfig from the Admin inside various places in favor of passing in the actual config for the component in a type safe manner. For instance previously running the manage command with a relay server set would panic as .unwrap_agent() was used inside MdsClient
Move tests out to a separate crate. This is done so that common test code can be placed in the library and new dependencies can be used etc so that (eventually) test helper code in test.rs can be removed from quilkin itself
Make "unit" tests integration tests. While iterating on quilkin I've noticed that the there is a lot of test code in the library that shouldn't be there, as it means changing unit tests needs to compile quilkin (including all the test code) instead of just being able to recompile and relink the single test binary. I'm not sure why it was done this way but I suspect it might be due to...
Add helper for test tracing. Currently the tracing-test dev dependency can be used to add a proc macro to a test function to get trace output to be outputted on test failure. Unfortunately this macro has no way to filter messages other than allowing only message from teh current crate, which makes integration tests useless as then no trace messages from quilkin itself will be captured, or else no filtering either by crate nor level (the level used in the tracing-test is trace!) which results in massive spam from eg hyper/h2 that makes test tracing from integration tests utterly useless. So I just made a simple trace_test! macro that makes it trivial to capture only the output we care about in tests to make debugging slightly more tolerable
The qt (quilkin-test) crate also introduces a proposed API to make writing integration tests easier, removing a lot of boilerplate by creating various components with minimal necessary configuration that can then be hooked together with dependencies. An example of this is shown in the refactored relay_routing test in mesh.rs

This is a temporary change, but just makes a change to config where it internally has a datacenter or icao enum as the agent config is completely different from all other components :p

A partial step at changing APIs and tests either binding sockets ahead of time that are then passed to the APIs, or for the proxy case, not binding and using the port returned from run_server in most cases. While the available_addr function is still used, this is partial step at completely removing it. While it works in most cases, it's actually not guaranteed to be 100% reliable as there is nothing stopping the OS of assigning the port to another call causing a bind failure for the second test to use it

This moves the rest of the CLI commands into components the same as proxy in the previous commit. The goal of this change is to start to tease apart the user interface (CLI) portions from the actual code that is run, to make writing tests easier This also completely removes the notion of unwrapping the `RuntimeConfig` from the `Admin` inside various places in favor of passing in the actual config for the component in a type safe manner. For instance previously running the manage command with a relay server set would panic as .unwrap_agent() was used inside MdsClient

Trace output is incredibly spammy at debug level, this just moves some offenders to trace level

This is a first pass at showing a different way to write integration tests, ie, tests that use multiple components together. 1. Move tests out to a separate crate. This is done so that common test code can be placed in the library and new dependencies can be used etc so that (eventually) test helper code in test.rs can be removed from quilkin itself 2. Make "unit" tests integration tests. While iterating on quilkin I've noticed that the there is a lot of test code in the library that shouldn't be there, as it means changing unit tests needs to compile quilkin (including all the test code) instead of just being able to recompile and relink the single test binary. I'm not sure why it was done this way but I suspect it might be due to... 3. Add helper for test tracing. Currently the tracing-test dev dependency can be used to add a proc macro to a test function to get trace output to be outputted on test failure. Unfortunately this macro has no way to filter messages other than allowing only message from teh current crate, which makes integration tests useless as then no trace messages from quilkin itself will be captured, or else _no filtering_ either by crate nor level (the level used in the tracing-test is trace!) which results in _massive_ spam from eg hyper/h2 that makes test tracing from integration tests utterly useless. So I just made a simple trace_test! macro that makes it trivial to capture only the output we care about in tests to make debugging slightly more tolerable 4. The qt (quilkin-test) crate also introduces a proposed API to make writing integration tests easier, removing a _lot_ of boilerplate by creating various components with minimal necessary configuration that can then be hooked together with dependencies. An example of this is shown in the refactored relay_routing test in mesh.rs

github-actions · 2024-03-15T16:07:11Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-03-15T16:09:24Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

markmandel · 2024-03-15T20:55:03Z

Having a quick look before getting on a plane for GDC - but this is an interesting approach! All of this seems like an improvement.

Make "unit" tests integration tests. While iterating on quilkin I've noticed that the there is a lot of test code in the library that shouldn't be there, as it means changing unit tests needs to compile quilkin (including all the test code) instead of just being able to recompile and relink the single test binary. I'm not sure why it was done this way but I suspect it might be due to...

Blame me, and my relative lack of experience with Rust vs y'all's at Embarks 😃

While it works in most cases, it's actually not guaranteed to be 100% reliable as there is nothing stopping the OS of assigning the port to another call causing a bind failure for the second test to use it, so eventually removing this will entirely remove bind-related flakiness.

Binding to 0 will result in a conflict? In theory, that shouldn't be possible - or am I missing something here?

Move tests out to a separate crate. This is done so that common test code can be placed in the library and new dependencies can be used etc so that (eventually) test helper code in test.rs can be removed from quilkin itself

Sounds good to me - I always struggled with the best way to do this.

So I just made a simple trace_test! macro that makes it trivial to capture only the output we care about in tests to make debugging slightly more tolerable

Sounds excellent. Right now having to step into a test and rerun it locally after CI failure to get at deeper logs is not pleasant.

Only other note in case this gets reviewed and potentially merged while I'm at GDC - Apache headers on everything please 👍🏻 (just in case).

github-actions · 2024-03-19T08:20:41Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-03-19T08:57:51Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-03-19T08:59:09Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-03-19T13:21:28Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-03-19T14:15:41Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-03-19T14:19:40Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

src/net/xds/client.rs

test/src/lib.rs

github-actions · 2024-03-22T08:33:42Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-03-22T08:44:21Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

XAMPPRocky · 2024-03-22T08:44:56Z

For @markmandel talked with Jake and we're going to merge this now, and do a followup PR for the one agones test, just to reduce merge conflicts and divergences.

github-actions · 2024-03-22T09:04:36Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

quilkin-bot · 2024-03-22T09:15:43Z

Build Succeeded 🥳

Build Id: e8ce28a7-bc2e-44bd-baf4-26fe921c8465

The following development images have been built, and will exist for the next 30 days:

us-docker.pkg.dev/quilkin/ci/quilkin:0.9.0-dev-6287896

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/909/head:pr_909 && git checkout pr_909
cargo build

Jake-Shadle added 8 commits March 7, 2024 08:06

Fix content encoding

c6ff64a

Replace single constant module with top level public export

439f641

Make AddressType Copy

9098470

Split config

fc9016d

This is a temporary change, but just makes a change to config where it internally has a datacenter or icao enum as the agent config is completely different from all other components :p

Relax logging

f78ce42

Trace output is incredibly spammy at debug level, this just moves some offenders to trace level

github-actions bot added the size/xl label Mar 15, 2024

Oops, unstaged

eef41ca

Fix example

b3ad573

Put constant in more logical place

a74d8f7

Oops

10d788e

Jake-Shadle added 3 commits March 19, 2024 13:42

Use simple fixed array for IcaoCode

9855b81

Transition datacenter_discovery

b9c141c

Oops

8337b3c

Fix add_nodes_from_config

54da99b

Fix lints

dd3c454

Port proxy::run_server

fad2d91

XAMPPRocky approved these changes Mar 21, 2024

View reviewed changes

src/net/xds/client.rs Show resolved Hide resolved

test/src/lib.rs Show resolved Hide resolved

Impl a few proxy tests

977086c

Disable test

22ca420

Jake-Shadle marked this pull request as ready for review March 22, 2024 08:40

Jake-Shadle requested review from markmandel and koslib as code owners March 22, 2024 08:40

XAMPPRocky enabled auto-merge (squash) March 22, 2024 08:43

Merge branch 'main' into test-refactor

87fb39e

XAMPPRocky mentioned this pull request Mar 22, 2024

Fix Last Agones Test From Refactor #911

Closed

Sigh

6287896

XAMPPRocky merged commit 2a46b36 into main Mar 22, 2024
10 checks passed

Jake-Shadle deleted the test-refactor branch March 22, 2024 09:34

markmandel added area/tests Unit tests, integration tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc labels Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test refactor proposal #909

Test refactor proposal #909

Jake-Shadle commented Mar 15, 2024

github-actions bot commented Mar 15, 2024

github-actions bot commented Mar 15, 2024

markmandel commented Mar 15, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

XAMPPRocky commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

quilkin-bot commented Mar 22, 2024

Test refactor proposal #909

Test refactor proposal #909

Conversation

Jake-Shadle commented Mar 15, 2024

github-actions bot commented Mar 15, 2024

github-actions bot commented Mar 15, 2024

markmandel commented Mar 15, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

XAMPPRocky commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

quilkin-bot commented Mar 22, 2024