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

add condition in sql/mongo tracker store in additional_events when retrieve_events_from_previous_conversation_sessions is true #8258

Merged
merged 11 commits into from
Mar 24, 2021

Conversation

indam23
Copy link
Contributor

@indam23 indam23 commented Mar 19, 2021

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@indam23 indam23 requested a review from wochinge March 19, 2021 16:22
rasa/core/tracker_store.py Outdated Show resolved Hide resolved
rasa/core/tracker_store.py Outdated Show resolved Hide resolved
@indam23 indam23 changed the base branch from main to 2.4.x March 22, 2021 08:49
@indam23 indam23 requested a review from a team March 22, 2021 08:49
@indam23 indam23 requested a review from a team as a code owner March 22, 2021 08:49
@indam23 indam23 removed request for a team March 22, 2021 09:01
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Change looks good overall - could you please add a test to avoid regression?

changelog/8258.bugfix.md Outdated Show resolved Hide resolved
@indam23
Copy link
Contributor Author

indam23 commented Mar 23, 2021

I parametrized the existing tests to confirm same behaviour - is there another side that should be tested?

@indam23
Copy link
Contributor Author

indam23 commented Mar 23, 2021

tests/core/test_evaluation.py::test_e2e_with_entity_evaluation is failing although it passes locally; can it be because the timeout for the test (240) is longer than the timeout for the pytest session (120s)?

@indam23 indam23 requested a review from wochinge March 24, 2021 07:01
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! The parametrize updates to the tests are amazing 💯 🥇

changelog/8258.bugfix.md Outdated Show resolved Hide resolved
tests/core/conftest.py Outdated Show resolved Hide resolved
indam23 and others added 2 commits March 24, 2021 10:21
Co-authored-by: Tobias Wochinger <[email protected]>
Co-authored-by: Tobias Wochinger <[email protected]>
@indam23 indam23 enabled auto-merge March 24, 2021 09:21
@indam23 indam23 merged commit 0024358 into 2.4.x Mar 24, 2021
@indam23 indam23 deleted the additional_events branch March 24, 2021 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants