Fix create balance when incomplete data

[jauggy]

Fixes #314

Unit Test Steps

mix test --only balance_test

Test Steps

Run local browser. Login as admin. Go to Admin > Matches > Show a match > Go to balance tab
It should have no errors

[jauggy]

Changes to this file are just formatting and also since we don't hit the db, we use ExUnit instead

[geekingfrog]

Nice fix, left a few comments about general code quality, but the functionality looks good.

[geekingfrog]

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.

[jauggy]

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

[geekingfrog]

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.

[jauggy]

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.

[geekingfrog]

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.

[jauggy]

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

[jauggy]

@geekingfrog have updated the unit test to use the db instead of mocks. But if I try and run it multiple times it will fail. try

mix test path/to/balance_lib_internal_test.exs

Try running this multiple times and it should fail. I'm not sure how to fix. You can see example below

failure.mp4

[StanczakDominik]

I've applied the mix format stuff from master :)

[StanczakDominik]

Looks good and I tested it on the lobbies that previously failed; the test failure on multiple runs is another issue, so I think it's fine to ignore until we fix it at the root cause. As long as the tests pass "once" (which is why having CI that we can take as mostly-a-source-of-truth is helpful) after dropping the database, I'm fine with this.

Thank you! :)

[jauggy]

Have added #317
so that someone can fix the tests not clearing. I couldn't figure it out.