unit_tests/sources/declarative/file/test_file_stream.py (1)

180-216: 💡 Verification agent

🧩 Analysis chain

Real file writes can leave artefacts on the test runner

test_get_article_attachments_and_file_is_uploaded exercises the real LocalFileSystemFileWriter and then checks Path(file_reference.staging_file_url).exists().
While this gives strong end-to-end confidence, it also:

• creates files under /tmp/airbyte-file-transfer which may persist between test runs, pollute CI caches, or collide when tests run in parallel;
• depends on the executing user having write permission to that location.

Could we isolate the side-effect? Two options:

-with HttpMocker() as http_mocker:
+with tempfile.TemporaryDirectory() as tmpdir, HttpMocker() as http_mocker, \
+     patch("airbyte_cdk.sources.declarative.retrievers.file_uploader.local_file_system_file_writer.STAGING_DIR", tmpdir):

or mock the writer altogether and assert that the intended path would be created, avoiding disk I/O:

-with HttpMocker() as http_mocker:
+with HttpMocker() as http_mocker, \
+     patch("airbyte_cdk.sources.declarative.retrievers.file_uploader.local_file_system_file_writer.LocalFileSystemFileWriter.write") as mock_write:
+    mock_write.return_value = 12345

This keeps the test hermetic while preserving its behavioural check—what do you think?

---

#!/usr/bin/env bash
set -e

echo "Searching for LocalFileSystemFileWriter definition..."
rg "class LocalFileSystemFileWriter" -n .

echo
echo "Searching for STAGING_DIR constant..."
rg "STAGING_DIR" -n .

---

#!/usr/bin/env bash
set -e
# Locate the LocalFileSystemFileWriter implementation
rg "class LocalFileSystemFileWriter" -n .
# Locate the STAGING_DIR constant
rg "STAGING_DIR" -n .

---

Could we verify the locations of LocalFileSystemFileWriter and the STAGING_DIR constant to ensure our patch targets are correct? wdyt?

---

🏁 Script executed:

#!/usr/bin/env bash
set -e

echo "🔍 Searching for LocalFileSystemFileWriter definition..."
rg "class LocalFileSystemFileWriter" -n .

echo
echo "🔍 Searching for STAGING_DIR constant..."
rg "STAGING_DIR" -n .

Length of output: 757

---

Let’s peek inside the writer to see how it references the staging directory—so we can patch the right constant. wdyt?

#!/usr/bin/env bash
set -e
echo "📄 Showing the first 200 lines of LocalFileSystemFileWriter:"
sed -n '1,200p' airbyte_cdk/sources/declarative/retrievers/file_uploader/local_file_system_file_writer.py

---

Could we peek at how the writer picks its staging directory so we can patch the right constant or env var? wdyt?

#!/usr/bin/env bash
set -e
sed -n '1,200p' airbyte_cdk/sources/declarative/retrievers/file_uploader/local_file_system_file_writer.py

---

Could we inspect how the writer pulls in the staging directory constant so we know exactly what to patch? wdyt?

#!/usr/bin/env bash
set -e

echo "🔍 Looking for AIRBYTE_STAGING_DIRECTORY usage in LocalFileSystemFileWriter..."
rg "AIRBYTE_STAGING_DIRECTORY" -n airbyte_cdk/sources/declarative/retrievers/file_uploader/local_file_system_file_writer.py || true

echo
echo "🔍 Searching for any staging-dir imports or references..."
rg "files_directory" -n airbyte_cdk/sources/declarative/retrievers/file_uploader/local_file_system_file_writer.py || true

---

Isolate filesystem side-effects in this test

Real LocalFileSystemFileWriter writes to the default staging dir (/staging/files or /tmp/airbyte-file-transfer), which can leave artifacts, collide under parallel CI, or require extra permissions. How about we make the test hermetic by either:

• Overriding the staging directory constant to a temp folder:

-with HttpMocker() as http_mocker:
+with tempfile.TemporaryDirectory() as tmpdir, HttpMocker() as http_mocker, \
+     patch("airbyte_cdk.sources.utils.files_directory.AIRBYTE_STAGING_DIRECTORY", tmpdir):

• Or mocking the writer’s disk I/O entirely:

-with HttpMocker() as http_mocker:
+with HttpMocker() as http_mocker, \
+     patch("airbyte_cdk.sources.declarative.retrievers.file_uploader.local_file_system_file_writer.LocalFileSystemFileWriter.write") as mock_write:
+    mock_write.return_value = 12345

This keeps the behavioral check (we still assert on file_reference) without touching the real filesystem. Wdyt?

airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)

3859-3861: Integration with stream creation process

These lines integrate the include_files logic into the file uploader creation during stream setup. The changes respect the PR's goal of controlling file inclusion based on stream configuration.

Could this be extended in the future to handle other per-stream configuration settings using a similar pattern? Wdyt?

unit_tests/sources/declarative/file/test_file_stream.py (1)

217-276: Heavy duplication—parameterise to keep tests tidy?

test_get_article_attachments_with_filename_extractor (and several siblings) repeat ~40 lines of HTTP stubbing, patch set-ups, and catalog construction that differ only by include_files and expectation flags. That repetition:

• inflates the file to ~500 lines, making maintenance harder;
• risks divergence when one test is updated but its twins are not.

Would a parameterised test using pytest.mark.parametrize (or a helper fixture if you must stay in unittest) reduce noise? Sketch:

@parameterized.expand([
    (True, True),   # include_files, expect_fs_writer
    (False, False), # ...
])
def test_article_attachments_writer_selection(self, include_files, expect_fs_writer):
    # shared arrange ...
    with patch(...) as fs_write, patch(...) as noop_write:
        ...
        catalog = CatalogBuilder().with_stream(
            ConfiguredAirbyteStreamBuilder()
            .with_name("article_attachments")
            .with_include_files(include_files)
        ).build()
        output = read(self._config(), catalog, yaml_file=...)
        self.assertEqual(bool(fs_write.called), expect_fs_writer)
        self.assertEqual(bool(noop_write.called), not expect_fs_writer)

Fewer lines, one place to update—wdyt?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

unit_tests/sources/declarative/file/test_file_stream.py (1)

123-151: Good mocking approach for file writers.

The test now properly mocks both NoopFileWriter.write and LocalFileSystemFileWriter.write methods to verify that the correct writer is used based on the include_files flag. You're setting a mock file size to simulate the return value from the file system writer.

Would adding a docstring to this test method help clarify its purpose, similar to how you've documented the other test methods? Perhaps something like "Test that article attachments can be read and files are included when include_files is true"? wdyt?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro