Perform client time sync on login

[baterflyrity]

Add current_time to the command welcome sent to the client on login.

Closes #889

[baterflyrity]

One failed test (tests/integration_tests/test_matchmaker.py→test_game_matchmaking_timeout[qstream]: asyncio.exceptions.TimeoutError) is irrelevant to this PR, so search someone fix for that in other issues.

[Askaholic]

Thanks for the contribution! The test failure was just a flaky test, there are still a few that can happen from time to time. I reran the tests which should probably take care of that.

[baterflyrity]

Also there are several now = datetime.utcnow() lines thus one would like to migrate them to server.timing too.

[Askaholic]

Also there are several now = datetime.utcnow() lines thus one would like to migrate them to server.timing too.

Ah, yea you should leave those. They actually are a bit of a special case because they are compared to datetime objects created automatically by sqlalchemy which are always timezone unaware. The datetime_now helper returns a timezone aware datetime object so it would not be able to be compared against the ban expiration time returned by sqlalchemy.

[baterflyrity]

Nice, all checks passed finally!

@Askaholic ure right, mocked server.timing plus added context manager helper for further cases.

Can this be merged now?

p.s.
Consider moving all datetime operation to server.timing because utc (un)awareness is not obvious within the code.

[baterflyrity]

@Askaholic , hey! Hope ure going well. What is the status of this PR?

[Askaholic]

Sorry, I haven’t had time to take care of this yet, my weekends have been pretty busy recently.

It looks great though! My only suggestion would be if you were up for it, to rework the fixed_time context manager to be a fixture that works much like the monkeypatch fixture. I think that would be slightly more ergonomic to work with. However, I’m also happy to merge as is, I just need to find some time on the weekend to do a final review and testing.

[baterflyrity]

Sure, just checking everything is fine.

It looks great though! My only suggestion would be if you were up for it, to rework the fixed_time context manager to be a fixture that works much like the monkeypatch fixture. I think that would be slightly more ergonomic to work with. However, I’m also happy to merge as is, I just need to find some time on the weekend to do a final review and testing.

Well, that is not quite true because there can be timing-relying tests (like check timeout/latency/performance of net io) hence fixture with hard fix all timestamps will fail. I have several ideas:

• add fixture alongside with context manager
• yield different easy-to-calculate timings e.g. 1, 2, 3
• make fixture with parameters

What do you think about these?

[Askaholic]

Is that an issue in any of these tests though? I think generally for timing related things internally we use either time.time() or loop.time() which wouldn't be affected by this fixture. I think the datetime_now function is mostly just used when we need to return an iso timestamp to the client, and there are a few places where it calculates long term expirations like the ban expiration.

I'm thinking for scenarios like testing the ban expiration it would actually be really nice to be able to do something like:

def test_ban_expired(fixed_time):
    fixed_time.set(100)
    
    # Insert 30 minute ban into database
    ...

    fixed_time.set(100 + 30)  # Whatever the units would be
    
    # Assert that ban has expired
    ...
    

[baterflyrity]

Np, if you assume no sequence timings are considered in tests.

Now your example would be:

def test_ban_expired(fixed_time):
    assert server.lobbyconnection.datetime_now().timestamp == 0.
    fixed_time(100)
    assert server.lobbyconnection.datetime_now().timestamp == 100.
    
    # Insert 30 minute ban into database
    ...

    fixed_time(100 + 30)  # Whatever the units would be
    assert server.lobbyconnection.datetime_now().timestamp == 130.
    
    # Assert that ban has expired
    ...