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

Fix create balance when incomplete data #315

Merged
merged 4 commits into from
Jun 9, 2024
Merged

Fix create balance when incomplete data #315

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e261714
Fix create balance when incomplete data
jauggy Jun 8, 2024
d7a6c3a
Update after geekingfrog review
jauggy Jun 8, 2024
97f567c
Merge branch 'master' into jauggy/fix-create-balance
StanczakDominik Jun 9, 2024
c953968
Drop mock dependencies
StanczakDominik Jun 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions lib/teiserver/battle/libs/balance_lib.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ defmodule Teiserver.Battle.BalanceLib do

def create_balance(groups, team_count, opts) do
start_time = System.system_time(:microsecond)
groups = standardise_groups(groups)

# We perform all our group calculations here and assign each group
# an ID that's used purely for this run of balance
Expand Down Expand Up @@ -154,6 +155,23 @@ defmodule Teiserver.Battle.BalanceLib do
|> Map.put(:time_taken, System.system_time(:microsecond) - start_time)
end

@doc """
Sometimes groups have missing data so we need to refetch it.
If we go through balancer_server then all the required data should be there
"""
def standardise_groups(groups) do
groups
|> Enum.map(fn group ->
# Iterate over our map
Map.new(group, fn {user_id, value} ->
cond do
is_number(value) -> {user_id, get_user_rating_rank_old(user_id, value)}
true -> {user_id, value}
end
end)
end)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand this bit correctly, when the value in the map is not a user struct, you want to fetch the corresponding user and replace the id with that user right?

In that case there's a much simpler way: Enum.map

    Enum.map(groups, fn group ->
      group
      |> Enum.map(fn {user_id, value} ->
        cond do
          is_number(value) -> {user_id, get_user_rating_rank_old(user_id, value)}
          true -> {user_id, value}
        end
      end)
      |> Enum.into(%{})
    end)

or, if you prefer using guards, you can also do the following

    Enum.map(groups, fn group ->
      group
      |> Enum.map(fn
        {user_id, value} when is_number(value) ->
          {user_id, get_user_rating_rank_old(user_id, value)}

        {user_id, value} ->
          {user_id, value}
      end)
      |> Enum.into(%{})
    end)

The advantage of Enum.map is that the intent is clearer, you are applying some sort of transformation on every element. In the case of a map it's a tuple of {key, value}.
When using map_reduce, it's a lot more complicated to figure out what's going on. The fact that you had to discard one of the return value is a code smell as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a feeling there was a better way to do it. Before you reviewed this, I had already posted a question on Elixir forums: https://elixirforum.com/t/iterate-through-map-of-dynamic-keys-and-update-their-values/64061/3
I'm liking the idea of Map.new


# Removes various keys we don't care about
defp cleanup_result(result) do
Map.take(
Expand Down Expand Up @@ -582,6 +600,14 @@ defmodule Teiserver.Battle.BalanceLib do
%{rating: rating, rank: rank, name: name}
end

@doc """
This is used by some screens to calculate a theoretical balance based on old ratings
"""
def get_user_rating_rank_old(userid, rating_value) do
%{rank: rank, name: name} = Account.get_user_by_id(userid)
%{rating: rating_value, rank: rank, name: name}
end

defp fuzz_rating(rating, multiplier) do
# Generate something between -1 and 1
modifier = 1 - :rand.uniform() * 2
Expand Down
2 changes: 2 additions & 0 deletions mix.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,12 @@
"libring": {:hex, :libring, "1.6.0", "d5dca4bcb1765f862ab59f175b403e356dec493f565670e0bacc4b35e109ce0d", [:mix], [], "hexpm", "5e91ece396af4bce99953d49ee0b02f698cd38326d93cd068361038167484319"},
"logger_file_backend": {:hex, :logger_file_backend, "0.0.13", "df07b14970e9ac1f57362985d76e6f24e3e1ab05c248055b7d223976881977c2", [:mix], [], "hexpm", "71a453a7e6e899ae4549fb147b1c6621f4233f8f48f58ca10a64ec67b6c50018"},
"math": {:hex, :math, "0.6.0", "69325af99e600123f6d994833502f6d063a2fa8f2786a3c0461fe6c6123a5166", [:mix], [], "hexpm", "2c73a64d64f719ee1f2821d382f3ed63e8e9564e5176d1c8aa777aac49b41bf7"},
"meck": {:hex, :meck, "0.9.2", "85ccbab053f1db86c7ca240e9fc718170ee5bda03810a6292b5306bf31bae5f5", [:rebar3], [], "hexpm", "81344f561357dc40a8344afa53767c32669153355b626ea9fcbc8da6b3045826"},
"merkle_map": {:hex, :merkle_map, "0.2.1", "01a88c87a6b9fb594c67c17ebaf047ee55ffa34e74297aa583ed87148006c4c8", [:mix], [], "hexpm", "fed4d143a5c8166eee4fa2b49564f3c4eace9cb252f0a82c1613bba905b2d04d"},
"metrics": {:hex, :metrics, "1.0.1", "25f094dea2cda98213cecc3aeff09e940299d950904393b2a29d191c346a8486", [:rebar3], [], "hexpm", "69b09adddc4f74a40716ae54d140f93beb0fb8978d8636eaded0c31b6f099f16"},
"mime": {:hex, :mime, "1.6.0", "dabde576a497cef4bbdd60aceee8160e02a6c89250d6c0b29e56c0dfb00db3d2", [:mix], [], "hexpm", "31a1a8613f8321143dde1dafc36006a17d28d02bdfecb9e95a880fa7aabd19a7"},
"mimerl": {:hex, :mimerl, "1.2.0", "67e2d3f571088d5cfd3e550c383094b47159f3eee8ffa08e64106cdf5e981be3", [:rebar3], [], "hexpm", "f278585650aa581986264638ebf698f8bb19df297f66ad91b18910dfc6e19323"},
"mock": {:hex, :mock, "0.3.8", "7046a306b71db2488ef54395eeb74df0a7f335a7caca4a3d3875d1fc81c884dd", [:mix], [{:meck, "~> 0.9.2", [hex: :meck, repo: "hexpm", optional: false]}], "hexpm", "7fa82364c97617d79bb7d15571193fc0c4fe5afd0c932cef09426b3ee6fe2022"},
StanczakDominik marked this conversation as resolved.
Show resolved Hide resolved
"nostrum": {:hex, :nostrum, "0.8.0", "36f5a08e99c3df3020523be9e1c650ad926a63becc5318562abfe782d586e078", [:mix], [{:certifi, "~> 2.8", [hex: :certifi, repo: "hexpm", optional: false]}, {:gun, "~> 2.0", [hex: :gun, repo: "hexpm", optional: false]}, {:jason, "~> 1.2", [hex: :jason, repo: "hexpm", optional: false]}, {:kcl, "~> 1.4", [hex: :kcl, repo: "hexpm", optional: false]}, {:mime, "~> 1.6 or ~> 2.0", [hex: :mime, repo: "hexpm", optional: false]}], "hexpm", "ce6861391ff346089d32a243fa71c0cb8bff79ab86ad53e8bf72808267899aee"},
"oban": {:hex, :oban, "2.15.2", "8f934a49db39163633965139c8846d8e24c2beb4180f34a005c2c7c3f69a6aa2", [:mix], [{:ecto_sql, "~> 3.6", [hex: :ecto_sql, repo: "hexpm", optional: false]}, {:ecto_sqlite3, "~> 0.9", [hex: :ecto_sqlite3, repo: "hexpm", optional: true]}, {:jason, "~> 1.1", [hex: :jason, repo: "hexpm", optional: false]}, {:postgrex, "~> 0.16", [hex: :postgrex, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "0f4a579ea48fc7489e0d84facf8b01566e142bdc6542d7dabce32c10e664f1e9"},
"openskill": {:git, "https://github.com/StanczakDominik/openskill.ex.git", "163a72f7423b8aa964909ea6aa3e9943739d87a6", [branch: "master"]},
Expand Down
72 changes: 72 additions & 0 deletions test/teiserver/battle/balance_lib_internal_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
defmodule Teiserver.Battle.BalanceLibInternalTest do
@moduledoc """
Can run all balance tests via
mix test --only balance_test
"""
use Teiserver.DataCase, async: true
@moduletag :balance_test
Copy link
Contributor

@geekingfrog geekingfrog Jun 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not convinced by this tag. Test tags should be used for logical grouping only. Like tag: cluster when you have several tests spanning multiple files testing one feature.
If you only want to test one file, you can always run mix test path/to/the/file

I think that tag could make sense for all balance related tests. But in this case, let's make another PR that tags all the relevant tests at once. And the tag should only be :balance since in this context, it's clear this is about testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The other balance tests are tagged already though. So it's hitting multiple files.

alias Teiserver.Battle.BalanceLib

test "Able to standardise groups with incomplete data" do
[user1, user2, user3, user4, user5] = create_test_users()

groups = [
%{user1.id => 19, user2.id => 20},
%{user3.id => 18},
%{user4.id => 15},
%{user5.id => 11}
]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not a fan at all of the use of mocks. It forces you to have a deep knowledge of the internals of the stuff you're testing. You recognise that with the comment a bit bellow:

loser_picks algo will hit the databases so let's just test with split_one_chevs

It's also a departure from the other tests elsewhere.

For these reason, I'd rather have something that creates the mock data in the DB, and then exercise the code under test.

Using DataCase, async: true, I propose the following:

    [user1, user2, user3, user4, user5] =
      Enum.map(1..5, fn k ->
        Teiserver.TeiserverTestLib.new_user("User_#{k}")
      end)

    groups = [
      %{user1.id => 19, user2.id => 20},
      %{user3.id => 18},
      %{user4.id => 15},
      %{user5.id => 11}
    ]

    fixed_groups = BalanceLib.standardise_groups(groups)

    assert fixed_groups == [
             %{
               user1.id => %{name: "User_1", rank: 0, rating: 19},
               user2.id => %{name: "User_2", rank: 0, rating: 20}
             },
             %{user3.id => %{name: "User_3", rank: 0, rating: 18}},
             %{user4.id => %{name: "User_4", rank: 0, rating: 15}},
             %{user5.id => %{name: "User_5", rank: 0, rating: 11}}
           ]

and similar for the other test. You could even consider pulling the creation of users in a setup function for the module.

Let me know if you have any question over this code.

Copy link
Member Author

@jauggy jauggy Jun 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just tested this and any db changes are persisted between tests which means I cannot call TeiserverTestLib.new_user at beginning of each test. That function will create a user with a random name if it finds the user already exists.

It's really weird. If I call the test multiple times it will eventually fail.


fixed_groups = BalanceLib.standardise_groups(groups)

assert fixed_groups == [
%{
user1.id => %{name: "User_1", rank: 0, rating: 19},
user2.id => %{name: "User_2", rank: 0, rating: 20}
},
%{user3.id => %{name: "User_3", rank: 0, rating: 18}},
%{user4.id => %{name: "User_4", rank: 0, rating: 15}},
%{user5.id => %{name: "User_5", rank: 0, rating: 11}}
]

# loser_picks algo will hit the databases so let's just test with split_one_chevs
result = BalanceLib.create_balance(fixed_groups, 2, algorithm: "split_one_chevs")
assert result != nil
end

test "Handle groups with incomplete data in create_balance loser_pics" do
[user1, user2, user3, user4, user5] = create_test_users()

groups = [
%{user1.id => 19, user2.id => 20},
%{user3.id => 18},
%{user4.id => 15},
%{user5.id => 11}
]

# loser_picks algo will hit the databases so let's just test with split_one_chevs
result = BalanceLib.create_balance(groups, 2, algorithm: "loser_picks")
assert result != nil
end

test "Handle groups with incomplete data in create_balance split_one_chevs" do
[user1, user2, user3, user4, user5] = create_test_users()

groups = [
%{user1.id => 19, user2.id => 20},
%{user3.id => 18},
%{user4.id => 15},
%{user5.id => 11}
]

# loser_picks algo will hit the databases so let's just test with split_one_chevs
result = BalanceLib.create_balance(groups, 2, algorithm: "split_one_chevs")
assert result != nil
end

defp create_test_users do
Enum.map(1..5, fn k ->
Teiserver.TeiserverTestLib.new_user("User_#{k}")
end)
end
end
2 changes: 1 addition & 1 deletion test/teiserver/battle/split_one_chevs_internal_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ defmodule Teiserver.Battle.SplitOneChevsInternalTest do
Can run tests in this file only by
mix test test/teiserver/battle/split_one_chevs_internal_test.exs
"""
use Teiserver.DataCase, async: true
use ExUnit.Case
@moduletag :balance_test
alias Teiserver.Battle.Balance.SplitOneChevs

Expand Down
2 changes: 1 addition & 1 deletion test/teiserver/battle/split_one_chevs_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ defmodule Teiserver.Battle.SplitOneChevsTest do
Can run tests in this file only by
mix test test/teiserver/battle/split_one_chevs_test.exs
"""
use Teiserver.DataCase, async: true
use ExUnit.Case
@moduletag :balance_test
alias Teiserver.Battle.BalanceLib

Expand Down
Loading