feat: Select streams for testing #3280
Open
+338
−5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR adds a new streams parameter to TapTestRunner by propagating it through runner initialization and execution paths, enhances the core run_sync_dry_run implementation to resolve stream names, updates testing factory helpers to accept streams, and augments tests to fully cover the new behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
edgarrmondragon
changed the title
Documentation build overview
Show files changed (3 files in total): 📝 3 modified | ➕ 0 added | ➖ 0 deleted
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/core/testing/test_runners.py:181-190` </location>
<code_context>
+ streams=None,
)
+ def test_init_with_streams_parameter(self, mock_tap_class):
+ """Test TapTestRunner initialization with streams parameter."""
+ config = {"test": "config"}
+ streams = ["users", "orders", "products"]
+
+ runner = TapTestRunner(
+ tap_class=mock_tap_class,
+ config=config,
+ streams=streams,
+ )
+
+ assert runner.streams == streams
+ assert runner.singer_class is mock_tap_class
+ assert runner.config == config
+
+ def test_init_with_streams_none(self, mock_tap_class):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for invalid or unexpected types for the streams parameter.
Adding tests for invalid types, such as integers, dicts, or lists with non-string/non-Stream elements, will improve error handling and type safety for the streams parameter.
Suggested implementation:
```python
def test_init_with_streams_empty_sequence(self, mock_tap_class):
"""Test TapTestRunner initialization with empty streams sequence."""
def test_init_with_streams_invalid_type_int(self, mock_tap_class):
"""Test TapTestRunner initialization with streams as an integer."""
config = {"test": "config"}
streams = 123 # Invalid type
with pytest.raises(TypeError):
TapTestRunner(
tap_class=mock_tap_class,
config=config,
streams=streams,
)
def test_init_with_streams_invalid_type_dict(self, mock_tap_class):
"""Test TapTestRunner initialization with streams as a dict."""
config = {"test": "config"}
streams = {"users": True} # Invalid type
with pytest.raises(TypeError):
TapTestRunner(
tap_class=mock_tap_class,
config=config,
streams=streams,
)
def test_init_with_streams_invalid_list_elements(self, mock_tap_class):
"""Test TapTestRunner initialization with streams as a list of non-string elements."""
config = {"test": "config"}
streams = [123, None, object()] # Invalid elements
with pytest.raises(TypeError):
TapTestRunner(
tap_class=mock_tap_class,
config=config,
streams=streams,
)
```
These tests assume that `TapTestRunner` raises a `TypeError` when invalid types are passed to the `streams` parameter. If it currently does not, you will need to add type checking and exception raising logic to the `TapTestRunner` implementation to ensure these tests pass.
</issue_to_address>
### Comment 2
<location> `tests/core/testing/test_runners.py:213-231` </location>
<code_context>
+
+ assert runner.streams == []
+
+ def test_run_sync_dry_run_with_streams(self, mock_tap_class):
+ """Test run_sync_dry_run method with streams parameter."""
+ streams = ["users", "orders"]
+ runner = TapTestRunner(
+ tap_class=mock_tap_class,
+ streams=streams,
+ )
+
+ mock_tap = Mock(spec=Tap)
+ mock_tap.run_sync_dry_run.return_value = True
+
+ with patch.object(runner, "new_tap", return_value=mock_tap):
+ result = runner.run_sync_dry_run()
+
+ assert result is True
+ mock_tap.run_sync_dry_run.assert_called_once_with(
+ dry_run_record_limit=runner.suite_config.max_records_limit,
+ streams=streams,
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for run_sync_dry_run when streams is an empty sequence.
Please add a test to confirm run_sync_dry_run behaves correctly when streams is empty, ensuring no sync is attempted.
```suggestion
def test_run_sync_dry_run_with_streams(self, mock_tap_class):
"""Test run_sync_dry_run method with streams parameter."""
streams = ["users", "orders"]
runner = TapTestRunner(
tap_class=mock_tap_class,
streams=streams,
)
mock_tap = Mock(spec=Tap)
mock_tap.run_sync_dry_run.return_value = True
with patch.object(runner, "new_tap", return_value=mock_tap):
result = runner.run_sync_dry_run()
assert result is True
mock_tap.run_sync_dry_run.assert_called_once_with(
dry_run_record_limit=runner.suite_config.max_records_limit,
streams=streams,
)
def test_run_sync_dry_run_with_empty_streams(self, mock_tap_class):
"""Test run_sync_dry_run method with empty streams sequence."""
runner = TapTestRunner(
tap_class=mock_tap_class,
streams=[],
)
mock_tap = Mock(spec=Tap)
mock_tap.run_sync_dry_run.return_value = True
with patch.object(runner, "new_tap", return_value=mock_tap):
result = runner.run_sync_dry_run()
# Expect no sync to be attempted, so run_sync_dry_run should not be called
assert result is None or result is False
mock_tap.run_sync_dry_run.assert_not_called()
```
</issue_to_address>
### Comment 3
<location> `tests/core/testing/test_factory.py:139-140` </location>
<code_context>
tap_class=factory.tap_class,
config=factory.config,
suite_config=None,
+ streams=None,
parse_env_config=True,
)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider parametrizing tests for the streams argument in factory methods.
Parametrizing the tests will allow you to easily cover additional valid stream values and ensure correct handling in the factory.
Suggested implementation:
```python
import pytest
@pytest.mark.parametrize(
"streams",
[
None,
[],
["stream1"],
["stream1", "stream2"],
]
)
def test_factory_streams_parametrized(factory, streams):
result = factory.create(
tap_class=factory.tap_class,
config=factory.config,
suite_config=None,
streams=streams,
parse_env_config=True,
)
# Add assertions here to verify correct handling of streams
assert result is not None
# Optionally, check that result.streams matches the input
if hasattr(result, "streams"):
assert result.streams == streams
```
```python
# If there are other tests using streams, consider parametrizing them similarly.
```
- You may need to adjust the test function name and the factory invocation to match your actual test and factory method signatures.
- If your test suite uses fixtures for `factory`, ensure they are compatible with parametrization.
- Add or refine assertions to check for correct behavior with each `streams` value.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3280 +/- ##
=======================================
Coverage 93.47% 93.48%
=======================================
Files 69 69
Lines 5659 5666 +7
Branches 699 700 +1
=======================================
+ Hits 5290 5297 +7
Misses 264 264
Partials 105 105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
edgarrmondragon
force-pushed
the
add-tests-for-taprunner-streams-parameter
branch
from
22c2bba to
41da3c6
Compare
CodSpeed Performance ReportMerging #3280 will not alter performanceComparing Summary
|
edgarrmondragon
force-pushed
the
add-tests-for-taprunner-streams-parameter
branch
from
41da3c6 to
e61f8a9
Compare
edgarrmondragon
force-pushed
the
add-tests-for-taprunner-streams-parameter
branch
from
e61f8a9 to
f57b9e6
Compare
- Add tests for TapTestRunner initialization with streams parameter - Add tests for run_sync_dry_run with streams parameter - Add tests for different stream sequence types (list, tuple, empty) - Add integration test for complete workflow with streams - Fix factory tests to include streams=None parameter expectations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Edgar Ramírez Mondragón <[email protected]>
edgarrmondragon
force-pushed
the
add-tests-for-taprunner-streams-parameter
branch
from
f57b9e6 to
fca9043
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
streamsparameter inTapTestRunnerTest Coverage Added
Changes Made
streams=NoneparameterTest Plan
🤖 Generated with Claude Code
Summary by Sourcery
Introduce a 'streams' parameter to TapTestRunner and propagate it through the testing factory and runner, extend run_sync_dry_run to filter streams by name or object, and add comprehensive tests for streams behavior.
New Features:
Enhancements:
Tests: