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

Use prefix instead of different ports in redis database - incomplete #309

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

A-atmos
Copy link
Contributor

@A-atmos A-atmos commented Apr 14, 2023

For Issue: #207

This PR isn't ready to merge, but is here to ask for recommendation

This PR passes the database tests, but alert.log isn't created, which result in failing of other integration tests!

@AlyaGomaa
Copy link
Collaborator

hey @A-atmos i just rebased your branch to the latest develop

the issue here is that no msgs are received in any channel because of the is_msg_intended_for() function in slips_utils
this function doesn't have the prefix when checking for msgs
if i were you i'd move it the db file, fix all modules accordingly, and change the function to use the prefix

thank you for the effort this is a really tough issue!

@A-atmos
Copy link
Contributor Author

A-atmos commented Apr 16, 2023

Thank you for the appreciation. I'm doing my best to resolve this issue, but I'm still facing some issues in the integration tests. I moved the utils.is_msg_intended_for() in database.py and refactored the code accordingly. I'm not getting any errors while testing database and other individual tests, but while testing the integration tests, I'm still facing some issues. But I'm trying to fix them. I'll get this issue resolved as soon as possible. Thanks again!

@AlyaGomaa
Copy link
Collaborator

hey @A-atmos integration tests run on files in our dataset/, try to see which file is failing in the test and then run slips normally on that file and check what errors it gives you
also you may need to modify the database fixture in conftest.py to match the new changes in the db

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.

None yet

2 participants