-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 Shopify: Add BalanceTransactions
stream
#9850
🎉 Source Shopify: Add BalanceTransactions
stream
#9850
Conversation
Hi @harshithmullapudi ! Sorry to bother you, but I just want to know if you need something else from my side to continue with the review of this PR. |
Hey we are just less in number of folks working so kinda crunch on time but
will review it today or tomorrow. Thanks for the patience. Will surely get
back to you if any need comes
|
Hey can you run sourceAcceptanceTests and share the screenshots? |
@harshithmullapudi , AcceptanceTests's output is present in the PR description, under |
Hey yeah I ran the tests and they were failing
|
hey looks like the way the balance stream is implemented its params are order _id. You can find more |
Hy! Acceptance tests for Maybe you are getting Acceptance tests
❯ docker pull airbyte/source-acceptance-test
Using default tag: latest
latest: Pulling from airbyte/source-acceptance-test
Digest: sha256:3431e847124f13a0a94a62c91de313f0e2a69c1419aa6946d0c58a8b5383dfe4
Status: Image is up to date for airbyte/source-acceptance-test:latest
docker.io/airbyte/source-acceptance-test:latest
❯ python -m pytest integration_tests -p integration_tests.acceptance
Test session starts (platform: linux, Python 3.7.9, pytest 6.2.5, pytest-sugar 0.9.4)
cachedir: .pytest_cache
rootdir: /home/da/f14/git/airbyte, configfile: pytest.ini
plugins: sugar-0.9.4, requests-mock-1.9.3, timeout-1.4.2
collecting ...
airbyte-integrations/connectors/source-shopify/integration_tests/integration_test.py::test_dummy_test ✓ 6% ▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_match_expected[inputs0] ✓ 11% █▎
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_required[inputs0] ✓ 17% █▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_optional[inputs0] ✓ 22% ██▎
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_has_secret[inputs0] ✓ 28% ██▊
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_secret_never_in_the_output[inputs0] ✓ 33% ███▍
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_defined_refs_exist_in_json_spec_file[inputs0] ✓ 39% ███▉
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_oauth_flow_parameters[inputs0] ✓ 44% ████▌
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestConnection.test_check[inputs0] ✓ 50% █████
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestConnection.test_check[inputs1] ✓ 56% █████▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestConnection.test_check[inputs2] ✓ 61% ██████▎
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_discover[inputs0] ✓ 67% ██████▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_defined_cursors_exist_in_schema[inputs0] ✓ 72% ███████▎
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_defined_refs_exist_in_schema[inputs0] ✓ 78% ███████▊
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― TestBasicRead.test_read[inputs0] ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
self = <source_acceptance_test.tests.test_core.TestBasicRead object at 0x7f1ca70bcc10>, connector_config = SecretDict(******)
configured_catalog = ConfiguredAirbyteCatalog(streams=[ConfiguredAirbyteStream(stream=AirbyteStream(name='customers', json_schema={'type': ... 'incremental'>, cursor_field=['id'], destination_sync_mode=<DestinationSyncMode.append: 'append'>, primary_key=None)])
inputs = BasicReadTestConfig(config_path='secrets/config.json', configured_catalog_path='integration_tests/configured_catalog.j...s={'abandoned_checkouts'}, expect_records=None, validate_schema=True, validate_data_points=False, timeout_seconds=3600)
expected_records = [], docker_runner = <source_acceptance_test.utils.connector_runner.ConnectorRunner object at 0x7f1ca6f75e50>
detailed_logger = <Logger detailed_logger acceptance_tests_logs/test_core.py__TestBasicRead__test_read[inputs0].txt (DEBUG)>
def test_read(
self,
connector_config,
configured_catalog,
inputs: BasicReadTestConfig,
expected_records: List[AirbyteMessage],
docker_runner: ConnectorRunner,
detailed_logger,
):
output = docker_runner.call_read(connector_config, configured_catalog)
records = [message.record for message in filter_output(output, Type.RECORD)]
assert records, "At least one record should be read using provided catalog"
if inputs.validate_schema:
self._validate_schema(records=records, configured_catalog=configured_catalog)
> self._validate_empty_streams(records=records, configured_catalog=configured_catalog, allowed_empty_streams=inputs.empty_streams)
../../bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:327:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <source_acceptance_test.tests.test_core.TestBasicRead object at 0x7f1ca70bcc10>
records = [AirbyteRecordMessage(stream='collects', data={'id': 19149737197733, 'collection_id': 218563281061, 'product_id': 5630...', 'shop_url': '****', 'id': '54676127909|38210170323109'}, emitted_at=1644870000607, namespace=None), ...]
configured_catalog = ConfiguredAirbyteCatalog(streams=[ConfiguredAirbyteStream(stream=AirbyteStream(name='customers', json_schema={'type': ... 'incremental'>, cursor_field=['id'], destination_sync_mode=<DestinationSyncMode.append: 'append'>, primary_key=None)])
allowed_empty_streams = {'abandoned_checkouts'}
def _validate_empty_streams(self, records, configured_catalog, allowed_empty_streams):
"""
Only certain streams allowed to be empty
"""
counter = Counter(record.stream for record in records)
all_streams = set(stream.stream.name for stream in configured_catalog.streams)
streams_with_records = set(counter.keys())
streams_without_records = all_streams - streams_with_records
streams_without_records = streams_without_records - allowed_empty_streams
> assert not streams_without_records, f"All streams should return some records, streams without records: {streams_without_records}"
E AssertionError: All streams should return some records, streams without records: {'order_risks', 'price_rules', 'order_refunds', 'products', 'customers', 'balance_transactions', 'metafields', 'fulfillments', 'pages', 'transactions', 'discount_codes', 'orders', 'draft_orders', 'fulfillment_orders', 'inventory_items', 'custom_collections'}
E assert not {'balance_transactions', 'custom_collections', 'customers', 'discount_codes', 'draft_orders', 'fulfillment_orders', ...}
../../bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:244: AssertionError
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestBasicRead.test_read[inputs0] ⨯ 83% ████████▍
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_full_refresh.py::TestFullRefresh.test_sequential_reads[inputs0] ✓ 89% ████████▉
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py::TestIncremental.test_two_sequential_reads[inputs0] ✓ 94% █████████▌
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py::TestIncremental.test_state_with_abnormally_large_values[inputs0] ✓ 100% ██████████
========================================================================================================== short test summary info ===========================================================================================================
FAILED ../../bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestBasicRead::test_read[inputs0] - AssertionError: All streams should return some records, streams without records: {'order_risks', 'price_rules', 'o...
Results (142.10s):
17 passed
1 failed
- airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:310 TestBasicRead.test_read[inputs0]
|
@ron-damon we're waiting our team add data to validate the final part of integration tests. |
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.
Looks fine, but this implementation is not complete.
It misses the Payouts
stream, which is needed in order to fetch the BalanceTransactions
stream, see the docs: https://shopify.dev/api/admin-rest/2022-01/resources/transactions#resource-description
We need to implement the Payouts
stream to have the payout_id
and use it inside the BalaceTransactions
stream API calls.
What can be done here at this point:
- The contributor just adds the
Payouts
stream and makes it work withBalanceTransactions
stream usingstream_slices
method. - We will leverage this contribution in order to complete it by ourselves, refactor this implementation and finish it by covering all the implementation and all the tests on our end.
@ron-damon @harshithmullapudi WDYT?
Hey @bazarnov, thanks for taking a look. According to the documentation, the IMHO, it's better to have this stream independent from another stream ( Make sense to you? |
Got your point. It's reasonable having the After that, please edit the
This means If you faced with the issue of missing
This will make it pass. If you have any questions, feel free to ask here. |
I think we will continue this in #10204 |
I think we can close this PR, due to this one |
What
Adds
BalanceTransactions
stream to Shopify source connector.How
id
logic in a new classIncrementalByIDShopifyStream
that inherits fromIncrementalShopifyStream
Collects
to inherit fromIncrementalByIDShopifyStream
BalanceTransactions
Recommended reading order
source_shopify/source.py
🚨 User Impact 🚨
None.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesTests
Unit
Integration
Acceptance
Acceptance tests for
secrets/config_oauth.json
were disabled because we only have API Password.There is 1 test failing because the account used for them doesn't have data for many streams. We've other shops that have a lot of data, but the tests take too much time to fetch all the data.