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.

Sign up for GitHub

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

Jump to bottom

Write DAT(s) that test different type combinations #10325

Merged
merged 12 commits into from
Feb 23, 2022
Merged

Write DAT(s) that test different type combinations #10325

merged 12 commits into from
Feb 23, 2022

Conversation

DoNotPanicUA
Copy link
Contributor

@DoNotPanicUA DoNotPanicUA commented Feb 14, 2022 •
edited
Loading

What

Write Data type tests for destinations.
#9443

How

Implement a new test in the acceptance test. The test should cover different combinations of destinations. For example, not all destinations support arrays or objects.
In addition, by default, the test should not be executed without an integration change from the destination side.

Recommended reading order

  1. DestinationAcceptanceTest.java
  2. DataTypeTestArgumentProvider.java

@DoNotPanicUA DoNotPanicUA linked an issue Feb 14, 2022 that may be closed by this pull request
Write DAT(s) that test different type combinations #9443
Closed
@DoNotPanicUA DoNotPanicUA self-assigned this Feb 14, 2022
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 14, 2022 20:45 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 14, 2022 20:45 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 16, 2022 18:21 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 16, 2022 18:21 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 16, 2022 21:23 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 16, 2022 21:23 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 17, 2022 15:50 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 17, 2022 15:50 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 17, 2022 17:17 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 17, 2022 17:17 Inactive
@DoNotPanicUA DoNotPanicUA changed the title [WIP] Write DAT(s) that test different type combinations Write DAT(s) that test different type combinations Feb 17, 2022
alexandr-shegeda
alexandr-shegeda approved these changes Feb 21, 2022
View reviewed changes
Copy link
Contributor

@alexandr-shegeda alexandr-shegeda left a comment

Choose a reason for hiding this comment

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

LGTM

@DoNotPanicUA DoNotPanicUA marked this pull request as ready for review February 21, 2022 13:19
@DoNotPanicUA
Copy link
Contributor Author

While testing the PR using S3, Mysql, and BigQuery Denormalized, I found several places to improve our acceptance tests. The improvements require minor investigation for destinations. So, it makes sense to do them as part of the integration-specific destinations with the new tests.
Note! The test will be disabled by default for all destinations.

  1. Check nested objects after normalization
  2. Check arrays after normalization
  3. Take into account timezone for DateTime values with timezone
  4. Improve comparing numbers. For example, 0 = 0.0
  5. Add stand-alone test with support flag for the additionalProperties

edgao
edgao approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

nice! I really like this structure :) didn't read through the test data in detail, but it seemed reasonable 👍

Added one question and a couple very small comments, otherwise LGTM! 🚛

...main/java/io/airbyte/integrations/standardtest/destination/DataTypeTestArgumentProvider.java
return Arguments.of(testConfig.messageFile, testConfig.catalogFile, testConfig.testCompatibility);
}

public record TestCompatibility(boolean requireBasicCompatibility,
Copy link
Contributor

Choose a reason for hiding this comment

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

checking my understanding: once the items in your comment are resolved, this will be true for all destinations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, we need to upgrade our abstract test part for the proper result validation. As it's a common part, we apply it for all future destinations after we do for one destination.

...rc/main/java/io/airbyte/integrations/standardtest/destination/DestinationAcceptanceTest.java Outdated
return;

final AirbyteCatalog catalog = readCatalogFromFile(catalogFilename);
final ConfiguredAirbyteCatalog configuredCatalog = getDefaultAirbyteCatalog(catalog);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: can this just call CatalogHelpers.toDefaultConfiguredCatalog directly?

@DoNotPanicUA @edgao
Update airbyte-integrations/bases/standard-destination-test/src/main/…
2690fed 
…java/io/airbyte/integrations/standardtest/destination/DestinationAcceptanceTest.java

Co-authored-by: Edward Gao <edward.gao@airbyte.io>
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 23, 2022 13:13 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 23, 2022 13:13 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 23, 2022 14:09 Inactive
@DoNotPanicUA DoNotPanicUA temporarily deployed to more-secrets February 23, 2022 14:09 Inactive
@DoNotPanicUA
Copy link
Contributor Author

DoNotPanicUA commented Feb 23, 2022 •
edited by github-actions bot
Loading

/test connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1887840903
✅ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1887840903
No Python unittests run

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets February 23, 2022 14:36 Inactive
@DoNotPanicUA DoNotPanicUA merged commit 9a4eec0 into master Feb 23, 2022
@DoNotPanicUA DoNotPanicUA deleted the aleonets/9443-data-type-acceptence-test branch February 23, 2022 15:09
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Feb 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edgao edgao edgao approved these changes

@alexandr-shegeda alexandr-shegeda alexandr-shegeda approved these changes

@tuliren tuliren Awaiting requested review from tuliren

Assignees

@DoNotPanicUA DoNotPanicUA

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Write DAT(s) that test different type combinations
4 participants
@DoNotPanicUA @edgao @alexandr-shegeda @octavia-squidington-iii