Built-in Testing Approaches and Aims

[kgpayne]

Having spent some time recently talking with @visch @BuzzCutNorman and others on target testing, and in reviewing this tap testing contribution from @stkbailey it makes sense to collate all these thoughts into a discussion. Broadly the questions surrounding testing are:

• How best to package built-in tests applicable to all taps/targets?
  • These should predominantly test that the specific implementation responds as expected for advertised capabilities (e.g. discovery) and conforms to the singer spec (e.g. correct handling of example RECORD messages in targets). We have separate and extensive tests that ensure the SDK itself is behaving as intended, which need not be duplicated when testing specific tap/targets.
  • This would include expectations on and configuration of tests which can be run with and without credentials.
• How best to provide tools and frameworks that allow tap/target developers to write more and better tests?
  • In particular it would be helpful to provide examples/documentation of mocking of external systems when testing specific tap/target implementations. E.g. test that a received RECORD results in the correctly formatted PUT or INSERT statement for the remote service, without actually submitting those requests (which would otherwise require credentials and sandboxes).

How best to package built-in tests applicable to all taps/targets?

Ideally tests can be imported from the existing singer_sdk.testing module and would be discoverable by pytest as individual tests. The current approach uses a factory function (get_standard_tap_tests) which returns a list of test functions to be run sequentially. Whilst this works well as a first iteration, this approach does not leverage the benefits of pytest discovery (i.e. itemised tests) and in particular IDE integrations that simplify the running and debugging via discovered itemised tests.

E.g.

The above can readily be implemented using pytest classes in singer_sdk.testing that are subclassed in specific tap/target implementations:

# singer_sdk.testing

class StandardSqlTargetTests:
     """Standard Target Tests."""

    def test_record_before_schema(self, Target, config):
        ...

    def test_invalid_schema(self, Target, config):
        ...

    def test_record_missing_key_property(self, Target, config, sqlalchemy_connection):
        ...

# target_snowflake/tests/test_standard.py

class TestTargetSnowflake(StandardSqlTargetTests):
    """Test Snowlflake using the standard SDK Target Tests."""

Passing of Target, config and sqlalchemy_connection instances can be handled using required fixtures, and similar test classes can be provided for suites of tests by type (tap and target).

How best to provide tools and frameworks that allow tap/target developers to write more and better tests?

I believe this is the intent of this MR by @stkbailey, providing both a TapTestRunner class to wrap and simplify the execution of Tap instances and also a TestTemplate base class for individual tests. Stephens' approach leverages @pytest.parametrize to advertise tests to pytest, but could easily be adapted to the class-based approach to offer fully-itemised test definitions (rather than a large single parameterised test).

In order to more easily facilitate mocking, we might benefit from breaking out the http interactions onto a separate HTTPConnector class, as per @edgarrmondragon suggestion here. This would allow developers to easily create mock connector classes for the purposes of testing, allowing core functionality to be tested without access to remote systems.

[kgpayne]

Related discussions in the pytest issues here and here. TLDR; is that class inheritance (as described above) is the best current solution for our use-case. The trouble with using hooks to override the built-in collection mechanisms seems to be that fixture resolution (and potential clashes) won't work out-of-the-box, and there is currently no easy way to override the fixture discovery mechanisms to also inject external fixtures.

[laurentS]

Jumping in here after a bit of headbanging while researching #1164 yesterday. Focusing on taps for now, I think it would be very helpful to (re)validate actual taps output on all tests.

At the start of this conversation, you mention

We have separate and extensive tests that ensure the SDK itself is behaving as intended

while this is true, it is very easy for tap developers to break this correct behaviour by overriding some method in a child class, or by unknowingly using an untested corner case scenario.
I think it would it would help if those base tests were also run on child classes, or at least a subset of them that enforce adherence to the singer spec for instance.

One thing in particular that I would have liked for a while is for the sdk to prevent me from writing "obviously wrong" code. For instance, it should check that my tap:

• always issues a SCHEMA message before a corresponding RECORD message
• always closes the stream with a valid STATE message
• each message has a valid format and follows the relevant JSONschema (it's easy to forget a print() in the code and break all that carefully crafted logic)
• more such basic validity checks at the singer interface...

All the above may seem obvious, but I can think of a handful of bugs we've had with tap-github or tap-gitlab that would have been caught by such trivial checks and saved us a few days of debugging.

I had been thinking of creating a TestTarget class that I could use to pipe the tap's output into for this purpose, but I think it might be possible to mock sys.stdout (or sys.stdin for targets) so that all tests are automatically validated against the singer spec, as the tap sends all messages to it. Maybe something like a context manager:

from singer_sdk.testing import sdk_magic_stdout_mock
# sdk_magic_stdout_mock.__enter__() mocks sys.stdout so that all singer messages are captured during the test

def test_in_tap_xyz():
    with sdk_magic_stdout_mock() as m:
        run_my_tap_with_some_use_case()
        assert this == that
        assert len(m.messages) == 42
    # sdk_magic_stdout_mock.__exit__() runs a bunch of asserts for me to ensure
    # that I followed the singer spec

Ideally the context manager would expose a hook/the list of messages so that I can do extra checks specific to my test, things like:

• assert number of records seen
• assert state is like xyz at the end of the run
• assert some_value appeared in the output
• ...

This way, I get the certainty that my tap follows the spec with 1 line of code and a bunch of extra whitespace :)

I'm not sure what this would look like for targets, but maybe something similar.