-
Notifications
You must be signed in to change notification settings - Fork 88
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
Re-submitting PR #140 (Add additional base test suite tests) #145
Conversation
* add check for unique primary keys in replicated records * replace explicit set of expected streams with `expected_check_streams` from base * update `test_run` doc string
* adjust imports * use specific error class * set encoding
* use latest image * trim pylint disable options * add unit tests step * make sure integration tests run always
@kspeer825 I've closed #140 and re-submitted here from the branch so circle ci can run. The newly added tap-tester tests will have some failures as the tap is currently emitting not selected fields. If you could validate that the tests are valid and correct per standard expectations? |
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.
This branch has conflicts with master, please fix before I can review. If tests are not passing, please implement a workaround in the failing test so that failing streams are omitted and it does pass. And document this workaround with a link to a Bug ticket in Jira explaining the issue and the streams effected.
* add parsing of "org/*" wildcard to retreive all repos for an org * add unit test cases for extract_repos_from_config() * add requests-mock for dev requirements * add unit test for get_all_repos()
* add basic backoff retry * add backoff to setup.py * replace deprecated assertEquals method with assertEqual * add MAX_SLEEP_SECONDS parsing from config and DEFAULT_MAX_SECONDS for rate limiting * add comments for changes and use DEFAULT_SLEEP_SECONDS * add pylint ignore for global-statement
- add full list of replicated streams - update GitHub docs links
@kspeer825 all the tap-tester tests are now passing except for the bookmark test which fails on every single stream in a number of different ways. This is because the tap is quite old in its implementation and how it bookmarks streams (using |
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.
The test suite is looking good, but I left some comments for your review.
In response to your bookmarks test:
If the tap is not properly implementing incremental replication or full table replication, that needs to be documented. If the test needs to be altered to cover differences in implementation from the base suite expectation that is acceptable so long as the behavior meets the requirements. An acceptable difference would be anything that is a "best practice" rather than a functional requirement i.e. saving state based on a key that differs in name from the replication key for that stream. I would need examples of the failures you are seeing to advise any further than that.
* adjust test expectatons for streams * create simulated_states based on test data and tap behavior * adjust tests based on commits and pr_commits schema
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.
I see a lot of the changes we discussed 👍 there is one issue in the automatic fields test though.
Also could you update the .circleci/config.yml to incorporate the slack orb and the tap-tester-user
context as documented in number 3. here https://github.com/stitchdata/tap-tester/blob/master/reference/circleci_configs.md#requirements-for-tap-repos. This is a new requirement for our workflows running in Circle.
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.
One minor change in config. Tests are good.
…singer-io#145) * add mac to gitignore * change testing repo * adjust `start_date_2` for new repo * add tap-tester automatic fields * add tap-tester all fields * add all expected streams to all fields test * set specific bookmark for test-repo * fix `collaborators` stream bookmark spelling for tap-tester * add more streams to automatic fields test * add tap-tester bookmarks * updates to automatic fields test: * add check for unique primary keys in replicated records * replace explicit set of expected streams with `expected_check_streams` from base * update `test_run` doc string * omit `team_memberships` stream from `expected_check_streams` * build expected_check_streams() using expected_streams() * add bug id and description * pylint fixes: * adjust imports * use specific error class * set encoding * adjust circle config: * use latest image * trim pylint disable options * add unit tests step * make sure integration tests run always * Re-submitting PR singer-io#141 (All repos for an organization) (singer-io#146) * add parsing of "org/*" wildcard to retreive all repos for an org * add unit test cases for extract_repos_from_config() * add requests-mock for dev requirements * add unit test for get_all_repos() * Re-submitting PR singer-io#142 (Improve Rate Limiting and Retry Logic) (singer-io#143) * add basic backoff retry * add backoff to setup.py * replace deprecated assertEquals method with assertEqual * add MAX_SLEEP_SECONDS parsing from config and DEFAULT_MAX_SECONDS for rate limiting * add comments for changes and use DEFAULT_SLEEP_SECONDS * add pylint ignore for global-statement * README updates: (singer-io#148) - add full list of replicated streams - update GitHub docs links * add streams to excluded_streams that aren't respecting automatic fields * add NotFoundException handling to collaborators stream * add bug info * adjust test for test-repo * pylint fixes * run unit and integations steps always * don't raise a NotFoundException to deal with access issues to resources * pylint fix and return empty response body for 404 * add collaborators stream to excluded set * fixes to tap-tester tests * adjust 2nd sync dates for data * deal with None from get method * adjust start date tap-tester dates * actually deal with NoneType in get method * FIX: sub_streams sync functions passed parent metadata * remove expected_check_streams after bug identified and addressed * use expected_streams after bug identified and addressed * don't write a bookmark for FULL_TABLE streams * update unit test expectations to recent changes * updates to bookmarks tap-tester: * adjust test expectatons for streams * create simulated_states based on test data and tap behavior * adjust tests based on commits and pr_commits schema * update base based on tap behavior and test data * adjust test expectations for team_members stream * Exclude collaborators stream due to access issues in circle * update circle config to include slack orb and tap-tester-user context * add bug info * add tap-tester-user to daily build context
Description of change
Re-submitting PR #140 from a branch on singer-io master for TDL-14049
Manual QA steps
Risks
Rollback steps