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

feat: improve included tap and target tests in singer_sdk.testing #1171

Merged
merged 119 commits into from
Jan 30, 2023

Conversation

kgpayne
Copy link
Contributor

@kgpayne kgpayne commented Nov 11, 2022

Closes #1169

This work incorporates both #580 and #1123 into a unified testing framework for developers tap and target implementations.

Example usage with sample TapCountries:

# tap_countries/tests/test_core.py
"""Tests standard tap features using the built-in SDK tests library."""

from singer_sdk.testing import TapTestRunner, get_test_class
from singer_sdk.testing.suites import (
    tap_stream_attribute_tests,
    tap_stream_tests,
    tap_tests,
)

from tap_countries.tap import TapCountries

SAMPLE_CONFIG = {}


TestTapCountries = get_test_class(
    test_runner=TapTestRunner(tap_class=TapCountries, config=SAMPLE_CONFIG),
    test_suites=[tap_tests, tap_stream_tests, tap_stream_attribute_tests],
)

Produces the following pytest -v output:

==================================================================== test session starts =====================================================================
platform darwin -- Python 3.8.11, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 -- /Users/kp/Library/Caches/pypoetry/virtualenvs/tap-countries-XN8-Xyes-py3.8/bin/python
cachedir: .pytest_cache
rootdir: /Users/kp/Projects/singer/tap-countries
collected 12 items

tap_countries/tests/test_core.py::TestTapCountries::test_tap_cli_prints PASSED                                                                         [  8%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_discovery PASSED                                                                          [ 16%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_stream_connections PASSED                                                                 [ 25%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_stream_continents_catalog_schema_matches_record PASSED                                    [ 33%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_stream_continents_record_schema_matches_catalog PASSED                                    [ 41%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_stream_continents_returns_record PASSED                                                   [ 50%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_stream_continents_primary_keys PASSED                                                     [ 58%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_stream_countries_catalog_schema_matches_record PASSED                                     [ 66%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_stream_countries_record_schema_matches_catalog PASSED                                     [ 75%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_stream_countries_returns_record PASSED                                                    [ 83%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_stream_countries_primary_keys PASSED                                                      [ 91%]
tap_countries/tests/test_core.py::TestTapCountries::test_tap_stream_attribute_countries_continent_is_object PASSED                                     [100%]

===================================================================== 12 passed in 1.60s =====================================================================

and the following UI in VSCode:

Screenshot 2022-11-11 at 16 24 53


📚 Documentation preview 📚: https://meltano-sdk--1171.org.readthedocs.build/en/1171/

@kgpayne
Copy link
Contributor Author

kgpayne commented Nov 16, 2022

Code work is done. It took a bit of gymnastics to get parameterisation of stream and stream-attribute tests working, but I think it was worthwhile 😅

Screenshot 2022-11-16 at 17 05 49

Screenshot 2022-11-16 at 17 06 10

@kgpayne
Copy link
Contributor Author

kgpayne commented Nov 16, 2022

Todo:

  • Update SDK Sample tests to use new standard tests
  • Update Tap/Target Cookiecutters to use new standard tests
  • Polish Typing and Docstrings
  • Add Usage Docs + Examples
  • Fix sample test failure

Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

@kgpayne Just some final nits and this will be g2g 😄

pyproject.toml Outdated Show resolved Hide resolved
singer_sdk/testing/__init__.py Show resolved Hide resolved
docs/testing.md Show resolved Hide resolved
@kgpayne kgpayne dismissed afolson’s stale review January 25, 2023 22:07

Suggested changes applied 🙏

@aaronsteers
Copy link
Contributor

@kgpayne - Fantastic work here. I'll try to do a final review tonight.

Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

This is great stuff, Ken!

@aaronsteers
Copy link
Contributor

aaronsteers commented Jan 26, 2023

@kgpayne - I want to just confirm this doesn't break tests on existing taps and targets. Do you know?

I updated this one to confirm on the target side:

✅ Tests pass above after reverting the original test file as test_sdk_legacy.py. 🎉

And we probably should do similar on a tap implementation is possible.

@kgpayne
Copy link
Contributor Author

kgpayne commented Jan 26, 2023

@aaronsteers yes, this is backwards compatible 🙂 The new factory functions have new names, and work differently, so are opt-in. @edgarrmondragon also asked, and I did some spot checks to be extra sure 👍

Copy link
Contributor

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

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

@kgpayne - Thanks for all your time today processing my questions in our zoom call. Based on our conversation, I think this meets the criteria of a stable increment and is ready to ship.

@edgarrmondragon - I know you put tons of effort and time into this as well, so thank you. 🙏

@aaronsteers
Copy link
Contributor

aaronsteers commented Jan 27, 2023

Follow ups identified in our call (will edit later to add links):

  1. Tests should store large results to local temp files rather than attempting to hold the entire dataset in memory.
  2. Classes RESTStream and SQLStream should try to leverage max record constraints if specified in the internal private member.

These are non-blocking for this PR, as discussed. Developers may run into scaling limits if their config defines a very large test dataset, but existing tests will still work regardless, and the failures / scaling issues will be raised only when the developer is in a mode to expect and deal with those.

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.

feat: improve included tap and target tests in singer_sdk.testing
5 participants