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

Get tests passing on all streams #97

Merged
merged 17 commits into from Apr 26, 2021
Merged

Get tests passing on all streams #97

merged 17 commits into from Apr 26, 2021

Conversation

luandy64
Copy link
Contributor

Description of change

This PR

  • removes the "untestable streams" list from all tests.
  • makes the test match the tap and expect the default page size to be 175, not 250
  • adds bookmarking to order_refunds
  • adds bookmarking to transactions
  • adds shopify error handling to transactions
    • The tests would fail with unhandled 429s
  • adds pagination to transactions

The only test expectation that changed is this line in the automatic_fields test

- self.expected_foreign_keys().get(stream, set())

QA steps

  • automated tests passing
  • manual qa steps passing (list below)
    • All tests pass locally

Risks

  • Low

Rollback steps

  • revert this branch

vagrant and others added 15 commits April 20, 2021 20:13
- Restrict a test run to just the streams passed in as a parameter
- Don't create connection in the test
- Run the test twice, once with each store
- Restrict streams tested to what was passed as a param
- Don't create a connection in the test
- Run with only the new store
- Run with all streams
There was not a query param for these streams to ask for records after a
certain date. This caused a failure in the start date test because we
would get too many records.

This change also includes a call to update and write a state message. The
bookmarks test was failing because there wouldn't be an entry in the final
state to assert on.
- Run test once per store
- Remove assertion on foreign key metadata
Copy link
Contributor

@kspeer825 kspeer825 left a comment

Choose a reason for hiding this comment

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

Just the one assertion in the pagination needs to be changed.

tests/base.py Outdated
Comment on lines 43 to 44
'date_window_size': 30,
'results_per_page': '50'
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels weird that results_per_page is a string but date_window_size is an integer. Not sure if that really matters outside of tap-tester though.



self.select_all_streams_and_fields(conn_id, our_catalogs, select_all_fields=True)
# Run a sync job using orchestrator
record_count_by_stream = self.run_sync(conn_id)
actual_fields_by_stream = runner.examine_target_output_for_fields()

for stream in self.expected_streams().difference(untested_streams):
for stream in testable_streams:
with self.subTest(stream=stream):

# verify that we can paginate with all fields selected
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the expected value should ever fall back to zero. It would only cover up the fact that we did not get past the page limit. Also shouldn't the limit be set to the results_per_page from get_properties since we are overwriting the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also shouldn't the limit be set to the results_per_page from get_properties since we are overwriting the default?

I agree with this and can make the change and test it

'start_date': '2021-04-01T00:00:00Z',
'shop': 'talenddatawearhouse',
'date_window_size': 30,
# BUG: https://jira.talendforge.org/browse/TDL-13180
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you throw this in base as well, please. Or what I usually do is mark it as BUG_<n> : jira-link in one place and then mark everywhere in the tests with BUG_<n> that it applies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do

We saw a failure on `pylint-2.8.1`

```
tap_shopify/streams/base.py:146:12: R1730: Consider using 'updated_at_max = min(updated_at_max, stop_time)' instead of unnecessary if block (consider-using-min-builtin)
```
@luandy64 luandy64 merged commit 8bc992b into master Apr 26, 2021
@luandy64 luandy64 deleted the fix-transaction branch April 26, 2021 15:13
This was referenced Apr 26, 2021
tmck-code pushed a commit to lexerdev/tap-shopify that referenced this pull request Feb 3, 2022
* Actually paginate the transactions stream

* WIP: Update pagination test to test all streams

* Break up streams between two shopify stores

* Fix test_pagination

- Restrict a test run to just the streams passed in as a parameter
- Don't create connection in the test

* Fix test_bookmarks

- Run the test twice, once with each store
- Restrict streams tested to what was passed as a param
- Don't create a connection in the test

* Fix test_start_date

- Run with only the new store
- Run with all streams

* Add task to remove config parameter

* Add error handling to Transactions requests

* Add post-request filtering to Transactions and Order Refunds

There was not a query param for these streams to ask for records after a
certain date. This caused a failure in the start date test because we
would get too many records.

This change also includes a call to update and write a state message. The
bookmarks test was failing because there wouldn't be an entry in the final
state to assert on.

* Wrap each store's test in a subtest

* Fix test_automatic

- Run test once per store
- Remove assertion on foreign key metadata

* Make pylint happy

* Fix whitespace

* Rename environment variables

* Trigger tests

* PR Feedback: Add bug comment in more places, API_LIMIT falls back to
config value

* Pin pylint to the previous working version

We saw a failure on `pylint-2.8.1`

```
tap_shopify/streams/base.py:146:12: R1730: Consider using 'updated_at_max = min(updated_at_max, stop_time)' instead of unnecessary if block (consider-using-min-builtin)
```
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.

None yet

3 participants