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

feat: Add DocumentWriter v2 #5435

Merged
merged 9 commits into from
Aug 16, 2023
Merged

feat: Add DocumentWriter v2 #5435

merged 9 commits into from
Aug 16, 2023

Conversation

julian-risch
Copy link
Member

@julian-risch julian-risch commented Jul 25, 2023

Related Issues

Proposed Changes:

This PR implements the new DocumentWriter component that can interact with all Stores and it adds two unit tests and one integration test.

How did you test it?

new unit tests, new integration test

Notes for the reviewer

Checklist

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Jul 25, 2023
@coveralls
Copy link
Collaborator

coveralls commented Jul 27, 2023

Pull Request Test Coverage Report for Build 5876944024

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.001%) to 48.131%

Files with Coverage Reduction New Missed Lines %
utils/context_matching.py 1 95.7%
Totals Coverage Status
Change from base Build 5867910704: 0.001%
Covered Lines: 11407
Relevant Lines: 23700

💛 - Coveralls

@julian-risch julian-risch marked this pull request as ready for review July 28, 2023 10:45
@julian-risch julian-risch requested review from a team as code owners July 28, 2023 10:45
@julian-risch julian-risch requested review from dfokina, ZanSara and silvanocerza and removed request for a team and ZanSara July 28, 2023 10:45


@pytest.mark.integration
def test_writer_adds_documents_to_store():
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this test is not necessary as we're not even testing the DocumentWriter really.
If we want to test that the component does what we're supposed to do we just need to verify that self.store.write_documents is called when executing run.
The actual writing is not this component's responsibility.
Something like this should work.

@pytest.mark.unit
def test_document_writer_run():
    mock_store = store(MagicMock)()
    writer = DocumentWriter()
    writer.store = mock_store

    data = writer.input(documents=[Document(content="This is the text of a document.")])
    writer.run(data=data)

    assert mock_store.write_documents.called```

@silvanocerza silvanocerza merged commit 22c7601 into main Aug 16, 2023
@silvanocerza silvanocerza deleted the document-writer-v2 branch August 16, 2023 11:48
DosticJelena pushed a commit to smartcat-labs/haystack that referenced this pull request Aug 23, 2023
* add draft of WriteToStore and basic test

* add DocumentWriter implementation

* draft unit and integration tests

* add release note

* mock Store in unit tests

* pylint

* Update haystack/preview/components/writers/document_writer.py

Co-authored-by: Daria Fokina <[email protected]>

* Remove unnecessary test

* Rework DocumentWriter to support new Component I/O definition

---------

Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Silvano Cerza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DocumentWriter (v2)
4 participants