Issue 32871/test other incremental stripe stream

[maxi297]

What

Following the tests for application_fees, there are a couple of streams which share the same behavior.

How

This PR mostly copy/paste the test for application_fees to apply them to the following streams:

• authorizations (IncrementalStripeStream)
• cards (IncrementalStripeStream)
• reviews (IncrementalStripeStream)
• transactions (IncrementalStripeStream)

Note

There was a discussion about if we should use inheritance to have less code to maintain for tests. We decided not to go that way for now because:

• In theory, each tests are independent as the test is agnostic of the implementation
• Tests that are implemented in parent classes are not well integrated with IDE (at least PyCharm). Hence, the experience is not so great if you want to run a specific test
• If an IncrementalStripeStream changes but not the others, we need to rely on the devs to copy the parent's class content to make sure he doesn't start from scratch again and identify changes properly

The drawback of the current solution is that when we change the behavior in IncrementalStripeStream for example, the behavior probably needs to be updated for all the tests for streams that implements IncrementalStripeStream. This would lead to more maintenance if the change is non-breaking in regards to the test class API. For example, if the test requires new information, the amount of work would be comparable for both solution has the dev will have to go in all the tests to update them with the new information any. If a test is added and is leveraging information that is available in all the child test classes, now the current solution would require more work to maintain.

I'm willing to reconsider the final decision if there are strong opinions on this

[vercel]

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

[github-actions]

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.

[girarda]

just a few comments lgtm!

[girarda]

nit: from looking at the code, it looks like the stream is adding the "legacy cursor field" to the record

[girarda]

Suggested change

	def test_when_read_then_add_cursor_field(self, http_mocker: HttpMocker) -> None:
	def test_when_read_then_add_created_cursor_field(self, http_mocker: HttpMocker) -> None:

or

Suggested change

	def test_when_read_then_add_cursor_field(self, http_mocker: HttpMocker) -> None:
	def test_when_read_then_add_legacy_cursor_field(self, http_mocker: HttpMocker) -> None:

?

[maxi297]

Given this comment and the current one, my answer would be:

Can you expand on that? My understanding is that when this stream was moved to the events stream, the cursor field was changed to updated. However, the records on the authorizations endpoint does not have this field so we backfill it. Hence, when we read without a state (which leads us to query the authorizations endpoint), we have to set the updated as this is the cursor field. Note that this logic is creating invalid cursor fields and there was a ticket created for that

EDIT: I'll continue my merge sequence but will come back to comment if we perform any changes regarding this on another PR