Skip to content

fix: Bump standard tap tests max records limit from 25 to 150 - #3030

Merged
edgarrmondragon merged 1 commit into
mainfrom
fix/records-limit-150
May 12, 2025
Merged

edgarrmondragon merged 1 commit into
mainfrom
fix/records-limit-150

Conversation

@edgarrmondragon

@edgarrmondragon edgarrmondragon commented May 12, 2025 •

Copy link
Copy Markdown
Collaborator

Related

Summary by Sourcery

Bug Fixes:

  • Increase the default max_records_limit for test suites from 25 to 150 to prevent test failures due to low record caps

@sourcery-ai

sourcery-ai Bot commented May 12, 2025 •

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Adjusts the default limit for standard tap tests by increasing the maximum records threshold in the testing configuration to support larger datasets.

File-Level Changes

Change Details Files
Increase default max_records_limit for testing to support more records
  • Changed default max_records_limit value from 25 to 150
singer_sdk/testing/config.py

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @edgarrmondragon - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@codecov

codecov Bot commented May 12, 2025 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.68%. Comparing base (1578802) to head (434ec1b).
⚠️ Report is 199 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3030      +/-   ##
==========================================
+ Coverage   91.66%   91.68%   +0.01%     
==========================================
  Files          62       62              
  Lines        5314     5314              
  Branches      684      684              
==========================================
+ Hits         4871     4872       +1     
  Misses        311      311              
+ Partials      132      131       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed

codspeed Bot commented May 12, 2025

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #3030 will not alter performance

Comparing fix/records-limit-150 (434ec1b) with main (1578802)

Summary

✅ 8 untouched benchmarks

@edgarrmondragon
edgarrmondragon merged commit d8555f5 into main May 12, 2025
@edgarrmondragon
edgarrmondragon deleted the fix/records-limit-150 branch May 12, 2025 19:26
github-merge-queue Bot pushed a commit that referenced this pull request Mar 11, 2026
…ests (#3556)

https://github.com/meltano/sdk/blob/4a5dd894d69cc068d140acea6e15fa4040831611/singer_sdk/tap_base.py#L284-L286

## Related

- #3029
- #3030
- #3037
- #3545

## Summary by Sourcery

Adjust tap dry-run behavior to cap record counts on all streams while
ensuring parent streams continue emitting records when child streams hit
their record limits.

Bug Fixes:
- Prevent auto-generated dry-run syncs from aborting parent streams when
child streams reach their record limit by catching child abort
exceptions and stopping only remaining child syncs.
- Apply the dry-run record limit consistently to both parent and child
streams so all streams are subject to the same cap.

## Summary by Sourcery

Ensure dry-run syncs apply record limits consistently across parent and
child streams without preventing parent records from being emitted when
children hit their cap.

Bug Fixes:
- Apply the dry-run record limit to all streams, including parents and
children, instead of only non-child streams.
- Prevent child stream aborts due to dry-run record limits from stopping
sibling or parent stream processing by catching abort exceptions during
child sync.

Tests:
- Add a regression test verifying that parent and sibling records are
still emitted, and child records are correctly capped, when a child
stream hits the dry-run record limit.

---------

Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
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.

1 participant