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

TDL-17880 Add missing test cases #131

Merged
merged 18 commits into from
Mar 11, 2022
Merged

Conversation

namrata270998
Copy link
Contributor

@namrata270998 namrata270998 commented Feb 28, 2022

Description of change

  • Updated the missing assertion in the discovery test
  • Added the assertion of checking duplicate primary keys in pagination.
  • Updated the assertion of duplicate primary keys in all_fields
  • Added missing assertion in start_date test case
  • Updated the create_object in utils.py

Manual QA steps

Risks

Rollback steps

  • revert this branch

@namrata270998 namrata270998 changed the base branch from TDL-15120-Add-Event_Type-Field-In-Schemas to crest-master February 28, 2022 12:54
@namrata270998 namrata270998 changed the base branch from crest-master to TDL-15120-Add-Event_Type-Field-In-Schemas February 28, 2022 17:01

actual_fields_by_stream = runner.examine_target_output_for_fields()
stream_primary_keys = self.expected_primary_keys()

for stream in self.expected_streams().difference(untested_streams):
with self.subTest(stream=stream):
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 have skipped the primary keys assertion as for each streams in the sync mode, the tap first fetches the actual stream records and any updates via the events endpoint which also retrieves the created as well as updated records. Hence there will be 2 records of the same primary key.

@namrata270998 namrata270998 marked this pull request as ready for review March 1, 2022 11:40
@namrata270998 namrata270998 changed the title Add missing test cases TDL-17880 Add missing test cases Mar 1, 2022
@namrata270998 namrata270998 changed the base branch from TDL-15120-Add-Event_Type-Field-In-Schemas to crest-master March 2, 2022 06:40
self.select_all_streams_and_fields(conn_id, our_catalogs, select_all_fields=True)

# Ensure tested streams have a record count which exceeds the API LIMIT
logging.info("Checking record counts for tested streams...")
print(" INFO Checking record counts for tested streams...")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to replace logging with print?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the logging, we were not able to log proper messages on the cci. Hence replaced with the print

Comment on lines +79 to +80
# verify there are no duplicate metadata entries
self.assertEqual(len(actual_fields), len(set(actual_fields)), msg = f"duplicates in the fields retrieved")
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment about this assertion at the function(test_run()) level comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment

Comment on lines +147 to +148
self.assertTrue(
primary_keys_sync_1.issubset(primary_keys_sync_2))
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment about this assertion at the function(test_run()) level comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment

# 'invoice_items',
# Data is generated automatically for 'invoice_line_items' when 'invoice_items' is created
'invoice_line_items',
# 'invoices',
'payout_transactions',
Copy link
Contributor

Choose a reason for hiding this comment

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

Put comment after this that this is the substream and it's not possible to solve for substreams.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, attach the card number in the comment.

@@ -472,12 +470,16 @@ def create_object(stream):
tax_rates=[], # TODO tax rates
)

if stream == 'charges':
if stream == 'charges' or stream == 'disputes':
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to check differently for these 2 streams? Please explain in the comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in the comment.

@namrata270998
Copy link
Contributor Author

namrata270998 commented Mar 4, 2022

This PR has the code from #123
Hence the PR #123 needs to be merged first

Copy link
Contributor

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

@namrata270998:

  • After execution of test_all_fields.py test, I observed that script is printing below error,

WARNING Stream[plans] Record[('plan_LHvXtOw4PlqR2c',)] Field[transform_usage] failed exact comparison. AssertionError({'divide_by': 2, 'round': 'up'} != "{'divide_by': 2, 'round': 'up'}")

There are such 49 warning messages observed during the execution. Since datatypes are different, equality assertion is always going to fail. Here approach should be comparing datatypes first then the values.

  • Instead of 2 different log lines, can we print comprehensive single line exception here?

e.g.

LINE1 => WARNING:[DATATYPE MISMATCH] Stream[plans] Record[('plan_LHvXtOw4PlqR2c',)] Field[transform_usage]: LINE2 => AssertionError({'divide_by': 2, 'round': 'up'} != "{'divide_by': 2, 'round': 'up'}")

LINE1 => WARNING:[VALUE MISMATCH] Stream[plans] Record[('plan_LHvXtOw4PlqR2c',)] Field[transform_usage]: AssertionError({'divide_by': 2, 'round': 'up'} != "{'divide_by': 2, 'round': 'up'}")

@kspeer825:
In Stripe all fields execution, I observed that execution is producing almost 3K+ lines of warning messages.

Need your help understanding below,

  • What is the advantage of printing all those warning?
  • Are we going to take any action on those warning messages?
  • Can we implement solution like just printing a diff between expectation and actual values?

tests/test_all_fields.py Outdated Show resolved Hide resolved
@kspeer825
Copy link
Contributor

kspeer825 commented Mar 10, 2022

@RushT007

What is the advantage of printing all those warning?

I think we originally just wanted to log as much information as possible

Are we going to take any action on those warning messages?

I think we hit a point of diminishing returns when testing this the first time around. So we left it in this state with the intention of refactoring it, when we had time to address it. I think the updates here are good, but we need to do an inventory of what test cases are actually covered here. I think the original pattern we used put us too far in the weeds. It took me about 20 minutes to figure out how this test was working and what it was covering. I think that's a problem.

Can we implement solution like just printing a diff between expectation and actual values?

This can be done although I don't know if it would give us much value. I think this PR can be merged as the changes are good but we should rethink the approach and card out a refactor of this test.

@namrata270998 namrata270998 merged commit a87d236 into crest-master Mar 11, 2022
RushiT0122 pushed a commit that referenced this pull request Jul 7, 2022
* TDL-14058: Update SDK and api version (#105)

* Updated SDK and API version

* Added expand parameter for more streams

* Updated replication key for invoices

* added backward compability for invoice bookmark

* Added unit test and code coverage report in circleci

* Resolved unit test failure

* Updated stripe SDK to latest version

* Running all test for testing

* updated stripe version in config.yml

* Running all test for testing

* Running all test for testing

* Running all test for testing

* Updated base class of tap_tester to handle replication key change of invoices

* Reverted back config.yml to run dependant tests

* Reverted back config.yml to run dependant tests

* Updated all_field test case

* Added back payout stream

* Added comments in test case

* Fixed key in bookmark for invoice_line_items

* Fixed key in bookmark for invoice_line_items in get_bookmark

* Fixed charge stream for all_field test case

* Fixed payouts stream for all_field test case

* Updated amount value

* Resolved circleci error

* Fiexed all_field test case�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[D�[�[�[3~�[�[C�[�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[C�[xed invoice_line_item failure in all_field test case

* Fix the build_daily workflow definition

* Resolved review comment

* Updated all_field test case

* Updated test case for customer and invoice_line_item stream

* Updated util.py

* Removed fields from missing fields

* fixed subscription_items stream

* Resolved review comment and added function for get and write bookmark

* Updated code comment

* Tdl 5894 update invoices stream (#115)

* initial commit

* added back for all streams

* resolved comments

* fixed cci errors

* fixed typo in comment

* updated comment

* fixed typo in function

* fixed failures

* fixed error

* resolved failure for discount

* updated discount schema

* added bugtracker

* Tdl 6026 Modify 'customer' stream on 'test_all_fields.py' to pass (#109)

* Initial commit

* Updated util.py �[D�[D�[D�[D�[D�[D�[D�[DAPI version to latest in util.py

* Added support of tax_ids field

* Updated discount schema

* Updated datatype of percent_off field.

* Tdl 6597 Modify 'product' stream on 'test_all_fields.py' to pass (#108)

* Initial commit

* Added back schema_keys

* Uncommented expected assertion

* Removed skus field from schema

* TDL-6587 Modify 'invoice_line_items' stream on 'test_all_fields.py' to pass (#112)

* Initial commit

* Updated schema file

* Removed price field from schema

* Removed tax_rates from schema

* Removed extra fields from schema

* Updated schema file

* Added metadata in schema

* Added invoice_line_items in KNOWN_FAILING_FIELDS

* Updated price value�[D�[D�[D�[D�[Drecord value

* Updated all_field test case

* Merged changes

* Uncommented code

* Updated code comment

* TDL-5992 modify subscription items stream (#114)

* initial commit with subscription_items stream

* reverted back to all streams

* resolved pylint

* testing for subscription_items only

* resolved error

* added back for all streams

* resolved cci errors

* added commnets

* added automatic fields for subscription items

* removed updated from the schema

* removed empty line and updated base

* resolved comments

* chahnged updated

* reverted changes

* reverted changes

* removed updated from subscription_items

* reverted a type change

Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com>

* TDL-17429 Revert back tiers field datatype conversion (#117)

* Initial commit for dict to stripe object conversion

* Resolved pylint error

* Updated test cases

* Added condtion to check dict type

Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com>
Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>
Co-authored-by: namrata270998 <75604662+namrata270998@users.noreply.github.com>
Co-authored-by: Prijen Khokhani <88327452+prijendev@users.noreply.github.com>

* Added tax_type and metadata in subcriptions_items

* added the proration_details due to cci failure

* TDL-17880 Add missing test cases (#131)

* Added event_type field in all schemas

* Added test cases for event updates

* Updated __init__ file

* Updated event_type field to updated_by_event_type field name

* Reverted back config.yml file changes

* Updated event_type field in all_field test case.

* Added missing test case assertion

* fixed circleci error and updated tests

* updated integration tests to resolve cci failures

* removed the unused variables

* removed duplicate code

* resolved comments

* added more comments

* added new map for intermittent missing fields

* updated schema missing fields map and changed the union

* added proper comment

Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com>
Co-authored-by: nevilparikh_crest <nevil.parikh@crestdatasys.com>

* Tdl 13149 sync payment intent (#127)

* Initial commit for payment_intent stream

* Updated bookmark test case.

* Updated expand parameter for charge stream in utils.py

* Updated utils.py

* Removed payment_intent object from charge stream

* Removed extra field from charges schema

* Added card field in schema

* Updated transfer_group and application_fee_amount field type

* Updated transfer_group field

* Updated datatype of destination field

* Updated transfer_data

* Added payment record creation

* Resolved test case error

* Resloved all fields test case error

* Resolved test case error

* Added comments in test case.

Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>

* Tdl 13711 upgrade json schema (#124)

* updated the transform_usage type and integration tests

* added plan back to the sub_items

* removed the workaround for the invoices 'created' field

* updated transform_usage type

* added code comment

* TDL 15120 add event type field in schemas (#123)

* Added event_type field in all schemas

* Added test cases for event updates

* Updated __init__ file

* Updated event_type field to updated_by_event_type field name

* Reverted back config.yml file changes

* Updated event_type field in all_field test case.

Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>

* updated charge.json

* TDL-14354 log request_id (#135)

* logged request_id for the sdk calls

* unittests for debug log

* changed the logic of new_list and updated unittests

* added code comments

* rsolved pylint error

* resolved cci failures

* resolved pylint errors

* rsolved pylint

* changed the function overwritten for logging request id

* resolved PR review comments

* skipped schema missing fields to pass the cci

* fixed cci issue

* added schema missing fields

* added receipt_url in fickle fields

* resolved PR comments

* TDL-17879 added authentication before discovery mode and unittests (#128)

* added authentication before discovery mode and unittests

* resolved comments

* fixed spelling error

* added schema missing fields

* added comment for pagination test

* added detailed comment

* added ticket it

* fixed circleci errors

* Tdl 17878 implement request timeout and retry (#126)

* added configurable request timeout with default as 5 minutes

* added comments

* added schema missing fields missing from schema

* fixed cci error

* added timeout in readme

* TDL-9856: ensure setup includes non-automatic balance transactions (#129)

* added test case to verify we sync only automatic payout's transactions

* added more comments

* resolved PR review comments

* resolve CCi error

* added doc-string in test_automatic_payout_transactions

* resolve CCi error

* resolve CCi error

* resolve CCi error

* resolve CCi error

* added try-catch in test_automatic_payout_transactions

Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>

* TDL-16966: Handle deleted invoice_line_items returned from the events updates endpoint (#137)

* Added retry logic for deleted invoice line items

* Resolved pylint error

* Added backoff over child also

* added code to skip deleted invoice line item call

* resolved unittest failure

* Updated all field test case.

* Reverted back changes in base.py

* Updated all field test case to pass cci.

Co-authored-by: harshpatel4_crest <harsh.patel4@crestdatasys.com>
Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>

* TDL-15168: Use `unique_line_item_id` for invoice updates' lines value instead of `id` (#134)

* updated id field for invoice line items

* resolve integration test error

* updated code to handle null invoice_item for invoice line items

* resolve review comments

* resolved CCi failure

Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>

* TDL-9801: `payouts` incorrectly mapped to `transfers` object results in transfer objects replicated by payouts stream (#133)

* added payout with transfer for payout events

* updated config.yml file

* removed unnecessary files

* resolved integration test error

* resolved integration test error

* resolved review comments

* fix integration test and run all fields test only

* fix cci failure

* fix cci failure

* fix cci failure

* fix cci failure, run all tests

Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>

* Tdl 17902 fix parent child relationship (#130)

* update child bookmark and make child stream  independent

* fixed pylint

* resolved PR review comments

* added function comments

* added child streams to bookmarks test and added a comment to all_fields

* resolved cci error

* resolved comments

* resolved comments

* resolved PR review comments

* fixed circleci errors

* skipped schema missing fields and removed extra checks for nested fields

* skipped receipt_url as it keeps changing every time in all_fields

* updated function name in unittest and resolved pylint error (#140)

Updated function names of unittest after all PR merge.

* TDL-18217 change lookback window logic (#138)

* added configurable lookback window

* added code comments

* resolved comments

* resolved comments

* changed lookback and added integration test

* added new map for intermittently missing fields

* resolved events stream lookback issue

* updated schema missing fields map

* added lookback test in config.yml

* initial commit

* configurable lookback window for bothstreams with updated tests

* resolved cci issue and added new assertion

* fixed cci issues

* addentsd comme

* added lookback test in all_tests_run

* changed to same default lookback for both streams

* resolved PR review comments

* fixed typo error

* added code comments

* changed the lookback loguc acc to Kyle's suggestions

* updated comments

* float value in lookback

* removed float as giving error

* handled 0 scenario for lookback

* handled string 0 and fixed cci

* resolved comments

* skipped new fields not present in schema fields to fix cci

* resolved comments

* added comments in unittests

* added lookback in readme

* updated debug with %s instead of f-string

* updated unittests for the logger statement

* removed the timedelta from start_date in lookback_window test

* added all_fields in lookback test deoendency

* updated the configurable lookback window test

* TDL-19384 optimize logic for parent child relationship to be independent (#141)

* initial commit with optimized logic for parent child streams

* fixed issues in the invoices and invoice_line_items sync

* fixed pylint

* updated unittests

* TDL-19384: Added Sub Stream check

* fixed unittests

* resolved PR comments and fix cci issues

* updated invoice_line_item to line_item

* Updated the code comments

* fixed typo

* updated code comments and added more unittests

* resolved pylint

* removed uwanted comments

* resolved PR comments

* resolved PR comments

* resolved PR review comments

* added integration test for only parent

* resolved PR comments

* skipped some newly generated fields from all_fields

* added failing field as missing nested field

Co-authored-by: dbshah1212 <dhruvin.shah@crestdatasys.com>

* removed skipped fields from SCHEMA_MISSING_FIELDS

* added missing fields in skipped fields

Co-authored-by: savan-chovatiya <80703490+savan-chovatiya@users.noreply.github.com>
Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>
Co-authored-by: namrata270998 <75604662+namrata270998@users.noreply.github.com>
Co-authored-by: nevilparikh_crest <nevil.parikh@crestdatasys.com>
Co-authored-by: Harsh <80324346+harshpatel4crest@users.noreply.github.com>
Co-authored-by: harshpatel4_crest <harsh.patel4@crestdatasys.com>
Co-authored-by: dbshah1212 <dhruvin.shah@crestdatasys.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants