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.

Sign up for GitHub

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

Jump to bottom

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

pamella
pamella reviewed Jun 20, 2024
View reviewed changes
tests/test_use_cases.py Outdated
Copy link
Collaborator

@pamella pamella Jun 20, 2024 •
edited
Loading

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!

pamella
pamella reviewed Jun 20, 2024
View reviewed changes
tests/test_use_cases.py Outdated Show resolved Hide resolved
pamella
pamella reviewed Jun 20, 2024
View reviewed changes
django_ai_assistant/helpers/use_cases.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

#102 (comment)

pamella
pamella reviewed Jun 20, 2024
View reviewed changes
tests/test_use_cases.py Outdated Show resolved Hide resolved
pamella
pamella reviewed Jun 20, 2024
View reviewed changes
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

fjsj
fjsj requested changes Jun 20, 2024
View reviewed changes
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 •
edited
Loading

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
pamella
pamella approved these changes Jun 20, 2024
View reviewed changes
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
Reviewers

@pamella pamella pamella approved these changes

@fjsj fjsj fjsj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Write tests for helpers and views
3 participants
@amandasavluchinske @pamella @fjsj