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
🐛 Source Stripe: adding integration tests #33306
Conversation
… issue-32871/test_events
…to-test-events-stream
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Before Merging a Connector Pull RequestWow! 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:
If the checklist is complete, but the CI check is failing,
|
…s-stream' into issue-32871/test_events
Currently failing because some of the tooling is not release until this is released |
airbyte-integrations/connectors/source-stripe/unit_tests/integration/test_events.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-stripe/source_stripe/streams.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-stripe/unit_tests/integration/config.py
Show resolved
Hide resolved
airbyte-integrations/connectors/source-stripe/unit_tests/integration/test_events.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-stripe/unit_tests/integration/test_events.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-stripe/unit_tests/integration/test_events.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-stripe/unit_tests/integration/test_events.py
Show resolved
Hide resolved
_a_response().build() | ||
) | ||
self._read(_config().with_start_date(_A_START_DATE)) | ||
http_mocker.assert_number_of_calls(request, 3) # one call for full_refresh availability, one call for incremental availability and one call for the actual read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a CDK bug? why does it check the full refresh availability if we're reading in incremental mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a CDK bug but a stripe one if it's one. See
airbyte/airbyte-integrations/connectors/source-stripe/source_stripe/availability_strategy.py
Lines 75 to 78 in 7a9ee9a
is_available, reason = self._check_availability_for_sync_mode(stream, SyncMode.full_refresh, logger, source, None) | |
if not is_available or not stream.supports_incremental: | |
return is_available, reason | |
return self._check_availability_for_sync_mode(stream, SyncMode.incremental, logger, source, {stream.cursor_field: 0}) |
Basically, as long as the stream is available in full_refresh and supports incremental, we check availability for both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the team tried to add the state in the availability strategy in order to check properly (see #32223). It feels like this is a case where we instantiate things without all the information and hence we need to update the library which we agreed is not a good pattern.
My feeling is that when we will have full_refresh with state, we will need to rethink that so I would punt on that
assert output.most_recent_state == {"events": {"created": cursor_value}} | ||
|
||
@HttpMocker() | ||
def test_given_state_when_read_then_use_state_for_query_params(self, http_mocker: HttpMocker) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the query params are derived from the stream slices, not the state, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stream slices is a implementation detail. At this level, the input that affect the HTTP requests is the state
airbyte-integrations/connectors/source-stripe/unit_tests/resource/http/response/events.json
Show resolved
Hide resolved
airbyte-integrations/connectors/source-stripe/unit_tests/integration/test_events.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: maxi297 <maxi297@users.noreply.github.com> Co-authored-by: Catherine Noll <clnoll@users.noreply.github.com> Co-authored-by: Brian Lai <51336873+brianjlai@users.noreply.github.com>
Co-authored-by: maxi297 <maxi297@users.noreply.github.com> Co-authored-by: Catherine Noll <clnoll@users.noreply.github.com> Co-authored-by: Brian Lai <51336873+brianjlai@users.noreply.github.com>
Co-authored-by: maxi297 <maxi297@users.noreply.github.com> Co-authored-by: Catherine Noll <clnoll@users.noreply.github.com> Co-authored-by: Brian Lai <51336873+brianjlai@users.noreply.github.com>
What
Adding integration tests for source-stripe events stream
How
Recommended reading order