Vector: Better strategy for testing topology changes

[lukesteensen]

See #681

The code that manages our dynamic topology is some of the most complex we have. Unfortunately, the existing strategy of driving the tests of that code over the network has lead to them being relatively flaky in resource-constrained environments like CI.

We should make an attempt to refactor these tests (or potentially the code and then the tests) that allows us to run them without worrying about network load on the box causing spurious failures.

[bittrance]

I took a shot at it here https://github.com/bittrance/vector/commit/489231a3323e6e71990a9f56703511dfd6e1b465 . If you think it looks good, I'll be happy to rewrite the rest of the tests.

[LucioFranco]

@bittrance that looks really good! I am very happy to review a PR that rewrites those tests :)

There are a couple of things I think that should be done:

• Move topology tests to the tests/topology.rs directory
• Create a tests/support/mod.rs that contains the MockSource, MockSink and MockTransfrom.
• All three of those should support Log::Any
• Might be useful to provide a handle similar to how tower-test does it. https://github.com/tower-rs/tower/blob/master/tower-test/src/mock/mod.rs#L31
• Attempt to use tokio-current-thread runtime instead of the test_util::runtime since that should save us one thread per test.

Otherwise, the one thing that I'm not sure about your code is the use of RefCell I am curious to see if we could avoid this maybe?

Let me know if you have any questions 😄

[bittrance]

The basic issue is that the mpsc::Receiver does not implement Clone so in order to be able to pass in a mpsc::Receiver that can later be extracted in build we put it in an Option. However, since build signature just borrows self immutably, we cannot mutate that Option, so we use RefCell inner mutability. As an aside, it is probably just a question of time till this turns into Arc<Mutex<Option<Receiver<Event>>>>.

MockSource, MockSink and MockTransfrom

We are creating configs so they should be called *Config? That seems to be the convention elsewhere in the code.

provide a handle

If you mean a struct which combines the sender/receiver and souce/sink config, add_source and add_sink takes ownership of the config, so it can't be in a struct; hence a tuple that we immediately deconstruct in the test.

I'll churn out a few more tests and open a PR.

[LucioFranco]

Ah right, that is what I was thinking, I think Arc<Mutex<..>> is fine for now. Well prob want to not use that for benchmarks but for tests its fine.

We are creating configs so they should be called *Config? That seems to be the convention elsewhere in the code.

Yeah, MockSourceConfig is fine. What ever you think is good, im 👍

So I'm thinking a handle would let you for a source push items into the source. For the sink, pull items from the sink to see if they came through, and for transforms to make sure the in/out follow some pattern etc. This way we could for example test the topology in more ways.

[bittrance]

This is taking a bit longer than expected, but good progress is being made here https://github.com/timberio/vector/compare/master...bittrance:topology-test-refactoring?expand=1 . PR in the next couple of days, I hope.

[binarylogic]

No problem, very much appreciate your help with this 👍