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

SAT verifies that an incremental stream with no data still produces a state message #17338

Closed
evantahler opened this issue Sep 28, 2022 · 2 comments · Fixed by #17791
Closed
Assignees

Comments

@evantahler
Copy link
Contributor

evantahler commented Sep 28, 2022

After an OC issue, we added this to the CDK - #17296. However, for connectors which don't use the CDK we should check this assertion

In an incremntal sync
if the cursor is already at the highwatermark
(records might be emitted)
but the same state message as before should be re-emitted in the sync

We already have a test for "abnormal state" - start looking here

This story is not to make all 200 connectors pass this test, but probably test with one or 2 connectors

@alafanechere
Copy link
Contributor

This test is already existing here.

    def test_state_with_abnormally_large_values(self, connector_config, configured_catalog, future_state, docker_runner: ConnectorRunner):
        configured_catalog = incremental_only_catalog(configured_catalog)
        output = docker_runner.call_read_with_state(config=connector_config, catalog=configured_catalog, state=future_state)
        records = filter_output(output, type_=Type.RECORD)
        states = filter_output(output, type_=Type.STATE)

        assert (
            not records
        ), f"The sync should produce no records when run with the state with abnormally large values {records[0].record.stream}"
        assert states, "The sync should produce at least one STATE message"

But this test is optional if future_state_path is not declared in acceptance_test_config.yaml.
I'm suggest to:

  • add unit-tests for this test
  • list all GA/Beta connectors that do not have this test
  • Add future_state_path to GA/Beta connectors missing it

@evantahler
Copy link
Contributor Author

Planning Notes:

  • Why did notion pass the test? It should have failed - @alafanechere to investigate. Maybe we are missing some test data or the stream isn't tested? - It is not. Only 3 streams are tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants