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

Adds tests for use_cases.py #102

Merged
merged 7 commits into from
Jun 20, 2024
Merged

Adds tests for use_cases.py #102

merged 7 commits into from
Jun 20, 2024

Conversation

amandasavluchinske
Copy link
Contributor

Closes #14

Copy link
Collaborator

@pamella pamella Jun 20, 2024

Choose a reason for hiding this comment

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

Nitpick suggestion: since we updated the structure to have both assistants.py and use_cases.py inside the helpers folder, I'd say we could move both test_assistants.py and test_use_cases.py to live within tests/helpers (similar to the tests/langchain).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense. thanks for the suggestion!

tests/test_use_cases.py Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

tests/test_use_cases.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pamella pamella left a comment

Choose a reason for hiding this comment

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

Looking good! I just left some suggestions

Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

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

I think we should separate test_helpers from test_use_cases. I think @pamella didn't meant to put it all in a single file, but a single directory instead.

@amandasavluchinske
Copy link
Contributor Author

I see!

@pamella
Copy link
Collaborator

pamella commented Jun 20, 2024

I think we should separate test_helpers from test_use_cases. I think @pamella didn't meant to put it all in a single file, but a single directory instead.

Yeap

  • tests/test_use_cases.py --> tests/helpers/test_use_cases.py
  • tests/test_assistants.py --> tests/helpers/test_assistants.py

Those changes will need an empty tests/helpers/__init__.py file too.

@amandasavluchinske amandasavluchinske requested a review from fjsj June 20, 2024 13:02
Copy link
Collaborator

@pamella pamella left a comment

Choose a reason for hiding this comment

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

LGTM

@fjsj fjsj merged commit 281da48 into main Jun 20, 2024
6 checks passed
@fjsj fjsj deleted the td/add-tests-helpers branch June 20, 2024 13:33
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.

Write tests for helpers and views
3 participants