Destinations CDK: better integration tests

[edgao]

Implement the test framework, write a minimal set of base tests, and implement those tests for destination-e2e-test. I stuck with our existing abstract class + concrete per-destination implementation strategy, because:

1. tests are opt-out
   1. like I mentioned in last (?) week's meeting - micronaut makes running the base class kind of wonky, so I think it's still best practice to have the override testFoo() { super.testFoo() } declarations. But even without that, we still get the test case.
2. it gives us an ok way to run individual test cases from inside intellij

general review guide:

1. DestinationMessage.kt has some simple convenience constructors
2. Start with IntegrationTest.kt, which is where the meat of the PR lives
3. Take a look at DestinationDataDumper, DestinationCleaner, ExpectedRecordMapper, and NameMapper - these are interfaces that destinations can/should implement. DataDumper doesn't have a noop implementation because every destination must implement it; everything else is optional.
4. Check out DestinationProcess, which is how we launch the connector via micronaut (and eventually Docker). It's basically just a wrapper around CliRunner.
5. CheckIntegrationTest + BasicFunctionalityIntegrationTest show how to write some basic tests.
6. Then skim through all the destination-e2e-test code - it's hopefully pretty self-explanatory after everything else in this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Sep 20, 2024 4:42pm

[edgao]

• Destinations cdk: implement snapshotting spec test #45468
• Destinations CDK: better integration tests #45113 👈
• Bulk CDK: Add conveniencen method to BufferingOutputConsumer to track message consumption #45611
• Bulk CDK (Destinations): Use Descriptor instead of full Stream as map key #45610
• Destinations cdk: recorddiffer that handles strongly-typed records #44874
• Load CDK E2ETest Destination w/o tests #45147 : 1 other dependent PR (#45634 )
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @edgao and the rest of your teammates on Graphite

[edgao]

I'll revert this before merging, this was just driving me nuts locally

[edgao]

I didn't check what these two classes are doing, but will revive these tests before merging this PR

[edgao]

this test case is mostly here to (a) be a quick smoke test on our state handling, and (b) show the typical interaction with DestinationProcess. Most tests we'll actually write don't need any of the state stuff, and would look more like:

runSync(config, stream, listOf(records...))
dumpAndDiffRecords(...)
// (potentially more syncs, if we're e.g. testing refreshes)

[johnny-schmidt]

Does this mean that we never test the null namespace case?

[edgao]

we can still do that. Something along these lines, specifics of config.setDefaultNamespace tbd:

val modifiedConfig = config.setDefaultNamespace(randomizedNamespace)
runSync(modifiedConfig, records = listOf(Record(namespace=null, ...)))
verify(namespace=randomizedNamespace, ...)

[edgao]

though it's a bit trickier for s3, where namespace=null is an actual thing :/ I think we'd just disregard randomizedNamespace for that:

runSync(config, records=[Record(namespace = null, name = "test_stream_with_random_chars1234")])

(i.e. this field is purely advisory, it's up to each individual test to use it or not)

[johnny-schmidt]

Dq, why is the cleaner its own thing and not an entry point into the process?

[edgao]

like why not have a cleanup method on DestinationProcess? I think they're pretty separate concerns - the dest process is just the connector (i.e. it supports spec/check/write)

but the cleaner is a weird out-of-connector thing. @stephane-airbyte 's recent Snowflake PR actually has a good example of what I want this class to do - https://github.com/airbytehq/airbyte/pull/45370/files#diff-c86b086ef3cc7bf564aedd699868dcc38661d51b3cc1bf34616720971866c6f6

(i.e. show schemas -> drop schema/etc)

[johnny-schmidt]

This looks solid to me. If you want to merge it down into e2e I'll start working on the final productionalization.

I think overall we'll want to iterate on these interfaces (mine and yours) and work out how we want to handle configs, injection in general, mapping to and from schemas and records, all that. But we can learn as we go.

[edgao]

@johnny-schmidt / @stephane-airbyte I do want to call this piece out specifically - I copied this pattern from existing tests, but it could (/should) just be some utility class that gets constructed by test classes, instead of an extra layer of inheritance.

I'm basically reserving my next couple weeks to work on testing stuff (and probably won't have time to make big changes b/c OC)... so I'd prefer to just leave that debate for later, so we can at least get things merged + unblock further development? It should be a pretty simple switch, just a bit tedious.

[johnny-schmidt]

I'm fine with this as a foundation. We can talk through some of the implications of this stuff at the next CDK meeting?

[edgao]

sgtm!

[edgao]

I think overall we'll want to iterate on these interfaces (mine and yours) and work out how we want to handle configs, injection in general, mapping to and from schemas and records, all that. But we can learn as we go.

+1. This is just the bare minimum to show what I want to do + get some basic demonstrations working, I fully expect we'll be tweaking/adding stuff as we go.