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

Let rating service publish rating changes to RabbitMQ #804

Conversation

cleborys
Copy link
Member

Closes #803.

@cleborys cleborys requested a review from BlackYps June 29, 2021 20:50
@cleborys cleborys force-pushed the feature/#803-publish-rating-changes-to-rabbitmq branch from 284319e to a7b589e Compare June 29, 2021 20:52
@cleborys
Copy link
Member Author

cleborys commented Jun 29, 2021

Running the tests locally, I see tests/integration_tests/test_matchmaker.py fail from time to time, both on this branch and on develop. It's not always the same test that fails and running in isolation they all seem to pass.

Likewise tests/integration_tests/test_server_instance.py::test_multiple_contexts seems to also fail on develop, at least when I run it locally.

Copy link
Collaborator

@Askaholic Askaholic left a comment

Choose a reason for hiding this comment

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

Maybe you want to add an integration test? I think I added some helpers for getting rabbitmq messages in my broadcast PR.

@cleborys cleborys force-pushed the feature/#803-publish-rating-changes-to-rabbitmq branch from f7db8d2 to 29cb8d6 Compare July 4, 2021 18:12
@codecov
Copy link

codecov bot commented Jul 4, 2021

Codecov Report

Merging #804 (6d87d04) into develop (5d3c1cf) will decrease coverage by 0.05%.
The diff coverage is 76.47%.

Impacted Files Coverage Δ
server/broadcast_service.py 100.00% <ø> (ø)
server/game_service.py 99.01% <ø> (-0.01%) ⬇️
server/lobbyconnection.py 94.20% <ø> (ø)
server/rating_service/rating_service.py 95.72% <73.33%> (-1.97%) ⬇️
server/gameconnection.py 95.05% <100.00%> (ø)
server/message_queue_service.py 97.84% <100.00%> (+0.02%) ⬆️
server/timing/timer.py 79.36% <0.00%> (+1.58%) ⬆️

@cleborys
Copy link
Member Author

cleborys commented Jul 4, 2021

I added an integration test - turns out there are some floating point issues somewhere in the interaction between the database and trueskill that lead to results like this:

E       AssertionError: assert {'new_rating_...00000002, ...} == {'new_rating_...: 2000.0, ...}
E         Omitting 6 identical items, use -vv to show
E         Differing items:
E         {'old_rating_mean': 2000.0000000000002} != {'old_rating_mean': 2000.0}

tests/integration_tests/test_game.py Outdated Show resolved Hide resolved
tests/integration_tests/test_game.py Outdated Show resolved Hide resolved
tests/unit_tests/test_game_rating.py Outdated Show resolved Hide resolved
tests/unit_tests/test_game_rating.py Outdated Show resolved Hide resolved
tests/unit_tests/test_game_rating.py Outdated Show resolved Hide resolved
@cleborys cleborys force-pushed the feature/#803-publish-rating-changes-to-rabbitmq branch from 1d19488 to 6d471c1 Compare July 29, 2021 09:42
@cleborys cleborys force-pushed the feature/#803-publish-rating-changes-to-rabbitmq branch from 6d471c1 to 6d87d04 Compare August 3, 2021 18:41
@Askaholic Askaholic merged commit 147426e into FAForever:develop Aug 4, 2021
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.

Publish rating changes to RabbitMQ for LeagueService to use
3 participants