-
Notifications
You must be signed in to change notification settings - Fork 803
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
Unit tests #607
Unit tests #607
Conversation
indam23
commented
Feb 4, 2021
•
edited
Loading
edited
- Create unit tests for custom actions, focusing on external APIs
- Closes add actions unit tests #535
- Rename workflows more intuitively and add action unit testing to CI pipeline
- remove mergepal - use automerge instead
- add running unit tests
- rename workflows & files for what they actually do
@erohmensing not all actions are covered by these, but since it's getting to be a lot of changes already, I think we should get this mergeable and in before adding more. Wdyt? |
I agree, it makes sense to do it in phases, i'll take a look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job with these! The fixtures are super thorough, i appreciate you giving time and thought to how to make these rerunnable without filling up our instance/newsletter with dummy conversations and such 😄
A few overall comments:
- I think it makes sense to keep the functions for getting community events outside of the community event class (still under the same API though, where they currently are), as they aren't particular to specific event instances
- Generally, consistency in the docstrings (uppercase vs lowercase, how the quotes are positioned
Coming back to review the tests themselves tomorrow -- mind blacking them in the meantime? Im so used to it now its faster to read 🙈
Co-authored-by: Ella Rohm-Ensing <[email protected]>
A few comments got lost in the accodion btw -- about the domain getting reformatted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm stoked about these! but please also check the comments about the domain, I think it will break the forms as it is currently
@erohmensing I know you already approved, but restructuring the tests to match the actions folder changed enough things that I think it needs another look |
|
||
from rasa_sdk.executor import CollectingDispatcher | ||
from rasa_sdk import Tracker | ||
from rasa.shared.core.domain import Domain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to use the rasa_sdk Domain
here, which is already a dict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but we want to load the actual domain of the bot - I started using an empty dict, but realized that could mask problems in the case that an action starts looking in the domain.