sdk icon indicating copy to clipboard operation
sdk copied to clipboard

bug: `No records returned in stream` warning for ignored streams

Open ReubenFrankel opened this issue 8 months ago • 2 comments

Singer SDK Version

0.45.1

Is this a regression?

  • [ ] Yes

Python Version

3.9

Bug scope

Taps (catalog, state, etc.)

Operating System

No response

Description

Ignoring no records for a stream surfaces another UserWarning I didn't previously spot after working on #2903, #2906 and #2907:

No records returned in stream

https://github.com/ReubenFrankel/tap-f1/actions/runs/13897554712

Should there a be a UserWarning emitted here if a developer is explicitly choosing to ignore no records for streams?

https://github.com/meltano/sdk/blob/c6672eb70002c7430f5db30fba05bb21cf9f0c11/singer_sdk/testing/tap_tests.py#L106:L111

Link to Slack/Linen

No response

ReubenFrankel avatar Mar 27 '25 21:03 ReubenFrankel

Should there a be a UserWarning emitted here if a developer is explicitly choosing to ignore no records for streams?

I would say probably not.

Is this warning independent from the error in the linked workflow?

FAILED tests/test_core.py::TestTapF1::test_tap_stream_primary_keys[driver_standings] - KeyError: 'position'

edgarrmondragon avatar Apr 01 '25 02:04 edgarrmondragon

Is this warning independent from the error in the linked workflow?

Yes, I just linked the workflow where this first cropped up after I started ignoring no records for the sprint_results stream:

No records returned in stream 'sprints_results'.

I have since fixed the KeyError issue: https://github.com/ReubenFrankel/tap-f1/pull/111

You can see the same warning appears in recent successful runs as well: https://github.com/ReubenFrankel/tap-f1/actions/runs/14191574119

ReubenFrankel avatar Apr 01 '25 09:04 ReubenFrankel

thanks @ReubenFrankel,

Does the approach in https://github.com/meltano/sdk/pull/2946 make sense then?

I like the idea of using pytest.skip instead of simply passing the test. Wdyt?

edgarrmondragon avatar Apr 01 '25 20:04 edgarrmondragon

Does the approach in #2946 make sense then?

Looks good to me - I prefer that approach as well. 🙂

I started working on a fix too, which included a refactor of ignore_no_records as an instance property, if you want to include that in your PR as well: https://github.com/ReubenFrankel/sdk/commit/e76b99878cb5d3bd0c77296617cac0e200f504f8

ReubenFrankel avatar Apr 01 '25 20:04 ReubenFrankel

included a refactor of ignore_no_records as an instance property, if you want to include that in your PR as well: ReubenFrankel@e76b998

I'm seeing an error when I cherry pick that:

  File "/Users/edgarramirez/Arch/sdk/singer_sdk/testing/templates.py", line 204, in <module>
    class AttributeTestTemplate(StreamTestTemplate[TapTestRunner]):

TypeError: <class 'singer_sdk.testing.templates.StreamTestTemplate'> is not a generic class

but I'm happy to review a PR to make that change.

edgarrmondragon avatar Apr 01 '25 20:04 edgarrmondragon

I'm seeing an error when I cherry pick that:

  File "/Users/edgarramirez/Arch/sdk/singer_sdk/testing/templates.py", line 204, in <module>
    class AttributeTestTemplate(StreamTestTemplate[TapTestRunner]):

TypeError: <class 'singer_sdk.testing.templates.StreamTestTemplate'> is not a generic class

I'll follow up with a working one once #2946 is merged then. 😅

ReubenFrankel avatar Apr 01 '25 20:04 ReubenFrankel

https://github.com/ReubenFrankel/tap-f1/actions/runs/14206461897 No warnings! 😍 Thank you @edgarrmondragon

ReubenFrankel avatar Apr 01 '25 21:04 ReubenFrankel