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

Source Stripe: fix stream schemas #21858

Merged
merged 6 commits into from
Jan 27, 2023

Conversation

roman-yermilov-gl
Copy link
Contributor

@roman-yermilov-gl roman-yermilov-gl commented Jan 25, 2023

What

https://github.com/airbytehq/oncall/issues/1364

How

Fix stream schemas:

  • start -> start_date
  • discount.start_date -> discount.start
    for subscription stream,
  • add status_transitions for invoices stream

@octavia-squidington-iv octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Jan 25, 2023
@roman-yermilov-gl
Copy link
Contributor Author

roman-yermilov-gl commented Jan 25, 2023

/test connector=connectors/source-stripe

🕑 connectors/source-stripe https://github.com/airbytehq/airbyte/actions/runs/4007987867
✅ connectors/source-stripe https://github.com/airbytehq/airbyte/actions/runs/4007987867
Python tests coverage:

Name                        Stmts   Miss  Cover
-----------------------------------------------
source_stripe/source.py        22      0   100%
source_stripe/__init__.py       2      0   100%
source_stripe/streams.py      304     27    91%
-----------------------------------------------
TOTAL                         328     27    92%
	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       141      5    96%   87, 93, 239, 243-244
	 source_acceptance_test/conftest.py                     211     95    55%   36, 42-44, 49, 54, 77, 83, 89-91, 110, 115-117, 123-125, 131-132, 137-138, 143, 149, 158-167, 173-178, 193, 217, 248, 254, 262-267, 275-285, 293-306, 311-317, 324-335, 342-358
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              402    115    71%   53, 58, 93-104, 109-116, 120-121, 125-126, 308, 346-363, 376-387, 391-396, 402, 435-440, 478-485, 528-530, 533, 598-606, 618-621, 626, 682-683, 689, 692, 728-738, 751-776
	 source_acceptance_test/tests/test_incremental.py       160     14    91%   58-65, 70-83, 246
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1609    339    79%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:94: The previous and actual specifications are identical.
============ 49 passed, 1 skipped, 31 warnings in 927.91s (0:15:27) ============

"start": {
"start_date": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm interested to know why a failing test didn't catch this before? Is it because we have yet to test this stream with expected_records?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can potentially test it with expected records if we strictly compare records. If not, then we allowed by default to have additional fields that not specified in schema. We currently have task to migrate to stripe tests to strict level (assigned to me) so I can check it during migration

Copy link
Contributor

Choose a reason for hiding this comment

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

We currently have task to migrate to stripe tests to strict level (assigned to me) so I can check it during migration

Does this imply that we intend to have all the fields in the schema, but we just missed this one? (And the future test work will help us catch it faster?)

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 just have remembered that migration did not catch it properly with expected records :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I just have remembered that migration did not catch it properly with expected records :)

Can you please file a bug about this then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@evantahler @mfsiega-airbyte I'm not sure this is actually a bug. A missing field may be not spotted by the test because we guarantee that additionalProperties field always equals True. Do you think we should override this requirement when testing if data matches its schema? if yes, I will create an issue for that.
Another thing is that we have lots of streams not filled with data so the test is just skipped. This should be fixed for GA connectors when they all have a test strictness level set to high

@mfsiega-airbyte
Copy link
Contributor

One question: the report here mentioned at least one other field missing (status_transitions in the Invoices stream). Shall we add that here? And more generally, do we have a mechanism to ensure that all fields are included? (I see a thread about testing?)

@davydov-d
Copy link
Collaborator

davydov-d commented Jan 26, 2023

/test connector=connectors/source-stripe

🕑 connectors/source-stripe https://github.com/airbytehq/airbyte/actions/runs/4014953645

@davydov-d davydov-d changed the title Ryermilov/source stripe fix subscription schema Source Stripe: fix stream schemas Jan 26, 2023
@davydov-d
Copy link
Collaborator

Bumped a major version because of schema changes

@davydov-d
Copy link
Collaborator

davydov-d commented Jan 26, 2023

/test connector=connectors/source-stripe

🕑 connectors/source-stripe https://github.com/airbytehq/airbyte/actions/runs/4015028609
✅ connectors/source-stripe https://github.com/airbytehq/airbyte/actions/runs/4015028609
Python tests coverage:

Name                        Stmts   Miss  Cover
-----------------------------------------------
source_stripe/source.py        22      0   100%
source_stripe/__init__.py       2      0   100%
source_stripe/streams.py      304     27    91%
-----------------------------------------------
TOTAL                         328     27    92%
	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       141      5    96%   87, 93, 239, 243-244
	 source_acceptance_test/conftest.py                     211     95    55%   36, 42-44, 49, 54, 77, 83, 89-91, 110, 115-117, 123-125, 131-132, 137-138, 143, 149, 158-167, 173-178, 193, 217, 248, 254, 262-267, 275-285, 293-306, 311-317, 324-335, 342-358
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              402    115    71%   53, 58, 93-104, 109-116, 120-121, 125-126, 308, 346-363, 376-387, 391-396, 402, 435-440, 478-485, 528-530, 533, 598-606, 618-621, 626, 682-683, 689, 692, 728-738, 751-776
	 source_acceptance_test/tests/test_incremental.py       160     14    91%   58-65, 70-83, 246
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1609    339    79%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:94: The previous and actual specifications are identical.
============ 49 passed, 1 skipped, 31 warnings in 906.37s (0:15:06) ============

@davydov-d
Copy link
Collaborator

One question: the report here mentioned at least one other field missing (status_transitions in the Invoices stream). Shall we add that here? And more generally, do we have a mechanism to ensure that all fields are included? (I see a thread about testing?)

@mfsiega-airbyte I added status_transitions field within this PR. Regarding your question about a mechanism to ensure that all fields are included, please see my comment above. I think it is possible for GA connectors when they all have high test strictness level. Additionally, we should probably report a bug for source-acceptance-tests package (but I'm not sure about this)

@davydov-d
Copy link
Collaborator

davydov-d commented Jan 26, 2023

/publish connector=connectors/source-stripe

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@davydov-d
Copy link
Collaborator

davydov-d commented Jan 26, 2023

/publish connector=connectors/source-stripe

🕑 Publishing the following connectors:
connectors/source-stripe
https://github.com/airbytehq/airbyte/actions/runs/4015357269


Connector Did it publish? Were definitions generated?
connectors/source-stripe

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 26, 2023 13:34 — with GitHub Actions Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 26, 2023 13:34 — with GitHub Actions Inactive
@davydov-d
Copy link
Collaborator

will merge this one when @klsoper approves

@github-actions
Copy link
Contributor

Airbyte Code Coverage

There is no coverage information present for the Files changed

Total Project Coverage 23.97%

@davydov-d davydov-d merged commit 01b0565 into master Jan 27, 2023
@davydov-d davydov-d deleted the ryermilov/source-stripe-fix-subscription-schema branch January 27, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/stripe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants