Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Destinations cdk: implement snapshotting spec test #45468

Merged

Conversation

edgao
Copy link
Contributor

@edgao edgao commented Sep 13, 2024

depends on #45632 - CI will fail until that gets released

tl;dr add a test that:

  • runs spec on the connector
  • diffs that spec against the spec in expected_spec.json
  • writes that spec back out to expected_spec.json

the original spec.json got deleted at some point in this stack? so I just created a new expected_spec.json - no strong opinion on where to put it, but I wanted it to be more obvious that it's not the "canonical" spec.

other things to note:

  • junit isn't able to discover the test from inside cdk/testFixtures, hence the class E2eSpecTest: SpecTest(). I'm pretty sure there's a way to make this work but haven't bothered looking into it yet, and I think this is good enough for now.
  • SpecTest actually writes out the new expected_spec.json for convenience. The expectation is that we'll just git commit that anytime we modify the spec.
  • jackson's pretty printer doesn't behave the same as prettier, so I excluded the expected_spec.json files from airbyte-ci's format command.

I already committed the updated spec, but there's still a few diffs against the original spec. Pasting them here, I think they're all intentional (e.g. changing type, tweaking wording, swapping example -> examples)

The property "$.connectionSpecification.properties.test_destination.oneOf.0.properties.test_destination_type.const" in the expected json is not found
The property "$.connectionSpecification.properties.test_destination.oneOf.0.properties.logging_config.oneOf.1.properties.max_entry_count.description" didn't match. Expected "Max number of entries to log. This destination is for testing only. So it won't make sense to log infinitely. The maximum is 1,000 entries.", Received: "Number of entries to log. This destination is for testing only. So it won't make sense to log infinitely. The maximum is 1,000 entries."
The property "$.connectionSpecification.properties.test_destination.oneOf.0.properties.logging_config.oneOf.1.properties.nth_entry_to_log.example" in the expected json is not found
The property "$.connectionSpecification.properties.test_destination.oneOf.0.properties.logging_config.oneOf.1.properties.nth_entry_to_log.type" didn't match. Expected "number", Received: "integer"
The property "$.connectionSpecification.properties.test_destination.oneOf.0.properties.logging_config.oneOf.2.properties.max_entry_count.description" didn't match. Expected "Max number of entries to log. This destination is for testing only. So it won't make sense to log infinitely. The maximum is 1,000 entries.", Received: "Number of entries to log. This destination is for testing only. So it won't make sense to log infinitely. The maximum is 1,000 entries."
The property "$.connectionSpecification.properties.test_destination.oneOf.1.properties.test_destination_type.const" in the expected json is not found
The property "$.connectionSpecification.properties.test_destination.oneOf.2.properties.test_destination_type.const" in the expected json is not found
The property "$.connectionSpecification.properties.test_destination.oneOf.2.properties.millis_per_record.description" didn't match. Expected "Number of milli-second to pause in between records.", Received: "The number of milliseconds to wait between each record."
The property "$.connectionSpecification.properties.test_destination.oneOf.3.properties.test_destination_type.const" in the expected json is not found

example diff in expected_spec.json:
image

Copy link

vercel bot commented Sep 13, 2024

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 6:20pm

DestinationSyncMode.APPEND_DEDUP,
)
}
//package io.airbyte.integrations.destination.e2e_test
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build is broken, commented this out just for testing purposes

@edgao edgao force-pushed the edgao/spec_test branch 3 times, most recently from 79e7420 to c8fa119 Compare September 13, 2024 23:48
@edgao edgao force-pushed the edgao/spec_test branch 2 times, most recently from 8ce08af to b1a901a Compare September 16, 2024 18:44
@edgao edgao force-pushed the edgao/cdk_integration_tests branch 2 times, most recently from 5d48b46 to dfd289a Compare September 16, 2024 23:38
@edgao edgao force-pushed the edgao/spec_test branch 2 times, most recently from f166b9f to d9b9550 Compare September 17, 2024 16:22
@edgao edgao changed the base branch from edgao/cdk_integration_tests to edgao/formatter_ignore_autogen_file September 17, 2024 21:46
@edgao edgao force-pushed the edgao/spec_test branch 2 times, most recently from 7fbe0ae to 58f564a Compare September 17, 2024 21:59
@edgao edgao force-pushed the edgao/formatter_ignore_autogen_file branch from cd8fbf0 to 00dee67 Compare September 17, 2024 22:23
@edgao edgao changed the base branch from edgao/formatter_ignore_autogen_file to edgao/cdk_integration_tests September 20, 2024 18:20
Base automatically changed from edgao/cdk_integration_tests to issue-9361/load-cdk-with-e2e-dest-post-refactor September 20, 2024 20:02
@edgao edgao merged commit 9511fda into issue-9361/load-cdk-with-e2e-dest-post-refactor Sep 20, 2024
24 of 29 checks passed
@edgao edgao deleted the edgao/spec_test branch September 20, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues CDK Connector Development Kit connectors/destination/e2e-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants