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

Re-submitting PR #140 (Add additional base test suite tests) #145

Merged
merged 47 commits into from
Feb 14, 2022

Conversation

loeakaodas
Copy link
Contributor

Description of change

Re-submitting PR #140 from a branch on singer-io master for TDL-14049

Manual QA steps

Risks

Rollback steps

  • revert this branch

@loeakaodas
Copy link
Contributor Author

@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?

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.

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.

loeakaodas and others added 8 commits November 16, 2021 15:07
* 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
@loeakaodas
Copy link
Contributor Author

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.

@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 since instead of update_at determined at execution time). If you can review that the other tests are now good to go and then make a recommendation on the bookmark test; should it be removed completely for now or adjusted to the taps implementation?

@loeakaodas loeakaodas requested a review from kspeer825 December 14, 2021 02:18
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.

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.

tests/base.py Outdated Show resolved Hide resolved
tests/test_github_all_fields.py Show resolved Hide resolved
tests/test_github_automatic_fields.py Outdated Show resolved Hide resolved
tests/test_github_automatic_fields.py Show resolved Hide resolved
tests/test_github_start_date.py Show resolved Hide resolved
tests/unittests/test_get_all_repos.py Outdated Show resolved Hide resolved
@loeakaodas loeakaodas requested a review from kspeer825 January 26, 2022 06:06
@loeakaodas loeakaodas requested review from luandy64 and removed request for manand31 January 26, 2022 17:34
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.

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.

tests/test_github_automatic_fields.py Outdated Show resolved Hide resolved
@loeakaodas loeakaodas requested a review from kspeer825 January 29, 2022 00:02
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.

One minor change in config. Tests are good.

.circleci/config.yml Show resolved Hide resolved
@loeakaodas loeakaodas requested a review from kspeer825 January 31, 2022 18:24
@KrisPersonal KrisPersonal self-requested a review February 11, 2022 05:59
@KrisPersonal KrisPersonal merged commit 7921b5f into master Feb 14, 2022
AJWurts pushed a commit to villagelabsco/tap-github that referenced this pull request Oct 24, 2024
…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
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.

3 participants