Kinesis sink tests - Githubissues

istreeter commented 1 year ago

localstack is started twice : once for the sink test and once for the source test. It would be great to start it only once.

I kinda feel if you only want localstack to start once, then it shouldn't be managed as a cats-effect resource scoped to the Spec.

@benjben I think you raise a very important point. It would be great to figure out a pattern that we can re-use across all our repos. Because this requirement might come up again and again.

This PR has followed the same pattern set out by the other snowplow repos and I think that's good enough for now.

benjben commented 1 year ago

I kinda feel if you only want localstack to start once, then it shouldn't be managed as a cats-effect resource scoped to the Spec.

Agreed!

It would be great to figure out a pattern that we can re-use across all our repos. Because this requirement might come up again and again.

Indeed. We might need a Semaphore and a var with a counter.

colmsnowplow commented 1 year ago

OK bar what Ian mentioned, I think I've addressed it all now! Thanks for the feedback @benjben, shout if there's any more

colmsnowplow commented 1 year ago

And my 2c on the other convo - I think whatever we do with localstack, it should work smoothly for both when we want to run all the tests, and when we only want to run one.

I don't understand the patterns that well but I think my preferred design would:

Not make localstack a resource, nor tie it to a spec/scope within the test framework, nor wrap it in effects, if possible
Set up localstack once, at the beginning of a run of any one or more test(s) in kinesis-it
Tear it down once, at the end
Leave it up to the individual specs/tests to create their resources either via AWS clients, or via API
Allow us to change localstack settings for just one test, without having to redeploy localstack (which might not be possible for those that are set by env var - use case in mind is to change KINESIS_ERROR_PROBABILITY for tests on retries)

colmsnowplow commented 1 year ago

After rebasing, it was simpler to just cherry-pick the commit that adds tests - closing this one.

snowplow-incubator / common-streams

Kinesis sink tests #21