Bulk load CDK: Even more tests

[edgao]

(based on https://github.com/airbytehq/airbyte/pull/47359/files, which has a few bugfixes in how we convert JsonNode to AirbyteValue)

as with other test porting PRs, probably best to just review by commit?

• add newline characters to field values in test (a la DAT.testLineBreakCharacters) - also renamed the test to reflect this
• delete some unnecessary micronaut annotations (the test framework no longer relies on micronaut at all)
• add a dedup test (DAT.testIncrementalDedupeSync / BaseTDTest.incrementalDedup - I only took the records from BaseTDTest, because the DAT version was very basic+naive)
  • fixed a bug in the RecordDiffer to properly compare temporal types
  • updates the mock destination to support deduping. This is, somehow, probably the cleanest depiction of upserting that we have anywhere >.>

[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		Oct 25, 2024 9:50pm

[edgao]

most (all?) destinations don't actually support traversing a jsonpath to fetch a cursor/PK, but it's easy enough to implement in java 🤷

[edgao]

@benmoriceau if you want to start dipping into some of this test stuff, this PR has a few interesting pieces:

1. there's an entire test case being ported into the new framework. (my personal philosophy: I prefer long/verbose tests over tests that are short, but require a lot of context/digging to understand what they're doing. So if it takes you a lot of effort to understand these tests, please tell me 🙏 )
2. The new tests run on docker in CI, but can be run without docker locally (you still have the option of running against docker, it's just not typically useful). Take a look at DestinationProcess.kt + the gradle stuff to see how it works
3. Identically to real connectors, the CDK itself runs the integration tests on a mock in-memory destination. This (a) makes it easier to write new tests (because testing the tests against real destinations is slower and less convenient), and (b) is an easy way to verify CDK behavior in isolation
4. This PR adds a test for deduping, so it also adds dedup support to that mock destination.
5. There's also a bugfix in RecordDiffer, which is the core of our integration tests. It generates a pretty-printed diff between a list of expected records, and the actual records in the destination. (take a look at DestinationDataDumper / IntegrationTest.dumpAndDiffRecords if you want to see how it's used)
6. (there's also a previous test case being updated to cover more stuff, in a way that's kind of gross - IMO this test case isn't super valuable, but it's not obviously useless enough to just delete it)

[benmoriceau]

@benmoriceau if you want to start dipping into some of this test stuff, this PR has a few interesting pieces:

1. there's an entire test case being ported into the new framework. (my personal philosophy: I prefer long/verbose tests over tests that are short, but require a lot of context/digging to understand what they're doing. So if it takes you a lot of effort to understand these tests, please tell me 🙏 )
2. The new tests run on docker in CI, but can be run without docker locally (you still have the option of running against docker, it's just not typically useful). Take a look at DestinationProcess.kt + the gradle stuff to see how it works
3. Identically to real connectors, the CDK itself runs the integration tests on a mock in-memory destination. This (a) makes it easier to write new tests (because testing the tests against real destinations is slower and less convenient), and (b) is an easy way to verify CDK behavior in isolation
4. This PR adds a test for deduping, so it also adds dedup support to that mock destination.
5. There's also a bugfix in RecordDiffer, which is the core of our integration tests. It generates a pretty-printed diff between a list of expected records, and the actual records in the destination. (take a look at DestinationDataDumper / IntegrationTest.dumpAndDiffRecords if you want to see how it's used)
6. (there's also a previous test case being updated to cover more stuff, in a way that's kind of gross - IMO this test case isn't super valuable, but it's not obviously useless enough to just delete it)

Thanks, I'll look at it when I'm done with the file transfer

[johnny-schmidt]

This is because docker-v-nondocker is now controlled by an explicit env variable check in the process factory?

[edgao]

yep, exactly. I just forgot to remove these annotations in #47006