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

[ISSUE #32871] adding integration tests for UpdatedCursorIncrementalS… #33597

Conversation

maxi297
Copy link
Contributor

@maxi297 maxi297 commented Dec 18, 2023

…tripeStream empty streams

What

Adding integration tests for all the UpdatedCursorIncrementalStripeStream that are empty streams.

How

There are two classes of UpdatedCursorIncrementalStripeStream:

  • Streams that needs to rely on filtering for the events parent streams: external_account_cards and external_account_bank_accounts
  • Streams that do not require filtering: payment_methods and early_fraud_warnings

Copy link

vercel bot commented Dec 18, 2023

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Jan 4, 2024 3:06pm

Copy link
Contributor

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
  • If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@@ -0,0 +1,34 @@
{
"object": "list",
"url": "/v1/accounts/acct_1032D82eZvKYlo2C/external_accounts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the fact that this is not the real URL that is being poked but no behavior rely on this so I don't think it's worth fixing now. The same applies for external_bank_accounts.json

Copy link
Contributor

Choose a reason for hiding this comment

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

why can't we have a real request with the actual API? Is it because there's no data in our account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The concern comes from the fact that the id in "/v1/accounts/<id>/external_accounts" depends on the parent stream. However, we don't use this field in our logic so there is no need really to make sure the values are align with the parent in the test

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment explaining that here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is JSON so I don't think I can add one specifically in this file. As it is only use in test_external_account_cards, I'll add that there

@octavia-squidington-iii octavia-squidington-iii added the area/connectors Connector related issues label Dec 18, 2023
@maxi297 maxi297 requested review from girarda and clnoll December 18, 2023 20:31
@maxi297 maxi297 marked this pull request as ready for review December 19, 2023 15:55
@octavia-squidington-iv octavia-squidington-iv requested review from a team December 19, 2023 15:56
@katmarkham katmarkham removed the request for review from a team December 20, 2023 14:32
@octavia-squidington-iv octavia-squidington-iv requested a review from a team December 20, 2023 23:24
Copy link
Contributor

@clnoll clnoll left a comment

Choose a reason for hiding this comment

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

Looks great! One small request.

@katmarkham katmarkham removed the request for review from a team January 3, 2024 18:19
@octavia-squidington-iv octavia-squidington-iv requested a review from a team January 4, 2024 15:01
@maxi297 maxi297 merged commit 7b4adce into issue-32871/test_other_incremental_stripe_stream Jan 4, 2024
11 of 13 checks passed
@maxi297 maxi297 deleted the issue-32871/test_updated_cursor_incremental_stripe_stream branch January 4, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants