boardgameio / boardgame.io

State Management and Multiplayer Networking for Turn-Based Games
https://boardgame.io
MIT License
10.03k stars 709 forks source link

Game over data is never added to match metadata #747

Closed adngdb closed 4 years ago

adngdb commented 4 years ago

During the onUpdate function, when a match is over, the gameover data from ctx is supposed to be added to the match's metadata and saved. However, that can never happen, because there is a block earlier in the function that returns early if state.ctx.gameover is defined.

Here's the code that updates the metadata with gameover: https://github.com/nicolodavis/boardgame.io/blob/3c8777bcd733fffc5a8efab7b0312ffb40ac8366/src/master/master.ts#L309-L317

And here's the block that returns early, preventing the previous code to ever run: https://github.com/nicolodavis/boardgame.io/blob/3c8777bcd733fffc5a8efab7b0312ffb40ac8366/src/master/master.ts#L232-L235

This issue covers:

I'm going to work on this, feel free to assign it to me.

adngdb commented 4 years ago

Interestingly, there are unit tests for this, but they only test the case when the end of the game is declared from an event (endGame). In my case, and if my analysis is correct, the end of the game happens with the endIf function of my Game object, and that doesn't pass through onUpdate.

delucis commented 4 years ago

@adngdb is this using the 0.40 prerelease?

The earlier check (L232) should only block further updates once gameover has already been stored (the state referenced here is the state returned from the database). It's there to prevent updates to finished games.

If the game ends via endIf that should also be processed inside the reducer used in onUpdate.

If you're able to reproduce this in a test that would be great.

adngdb commented 4 years ago

@adngdb is this using the 0.40 prerelease?

Yep.

The earlier check (L232) should only block further updates once gameover has already been stored (the state referenced here is the state returned from the database). It's there to prevent updates to finished games.

If the game ends via endIf that should also be processed inside the reducer used in onUpdate.

Yeah, after reading the code a bit more, I came to the conclusion that it should theoretically happen like that. But the gameover data is never exposed in my game using branch 0.40, and I don't understand why.

If you're able to reproduce this in a test that would be great.

That's precisely what I'm trying to do at the moment, but I'm having trouble figuring out how to test getting the metadata from the database. I'm reading through tests to see if there's an example, but if you have a pointer that might be helpful. :relaxed:

delucis commented 4 years ago

Just as a quick check, have you confirmed your game’s endIf is running as you’d expect and updating ctx.gameover? So the state contains gameover, but the metadata doesn’t?

adngdb commented 4 years ago

Yes, I've verified by enabling the debug console and looked at the ctx object there: it does contain the correct gameover value. Here's a screen with the local ctx and the result of the last API query for matches:

gameover-missing

delucis commented 4 years ago

OK — cool. In terms of testing, there’s an example of mocking the database in the API tests:

https://github.com/nicolodavis/boardgame.io/blob/2bd81e1c9bc2e7370654b07f4a85f5d085db27d3/src/server/api.test.ts#L19-L64

Perhaps you could use this to check if the relevant methods are being called where you would expect with what you would expect from the master?

adngdb commented 4 years ago

I managed to write a unit test that reproduces what I believe is the problematic scenario, and that test… passes. :disappointed: I assume this might actually be a problem on my side, so that's where I'll look next. Thanks for the help!

Here's the test, in case I missed something:

  test('writes gameover to metadata via endIf trigger', async () => {
    const gameOverArg = 'gameOverArg';
    const game = {
      moves: {
        foo: () => {},
      },
      turn: {
        stages: {
          respond: {
            moves: {
              stop: () => ({ finished: true }),
            }
          }
        },
      },
      endIf: (G) => {
        if (G.finished) {
            return gameOverArg;
        }
      },
    };
    const id = 'gameWithMetadata';
    const db = new InMemory();
    const dbMetadata = {
      gameName: 'tic-tac-toe',
      setupData: {},
      players: { '0': { id: 0 }, '1': { id: 1 } },
      createdAt: 0,
      updatedAt: 0,
    };
    db.setMetadata(id, dbMetadata);
    const masterWithMetadata = new Master(game, db, TransportAPI(send, sendAll));
    await masterWithMetadata.onSync(id, '0', 2);

    // Let the second player make the move.
    const event = ActionCreators.gameEvent('setActivePlayers', { others: 'respond' }, '0');
    await masterWithMetadata.onUpdate(event, 0, id, '0');
    // Call the move to end the game.
    const action = ActionCreators.makeMove('stop', null, '1');
    await masterWithMetadata.onUpdate(action, 1, id, '1');
    // Make sure the game is over.
    expect(sendAllReturn().args[1].ctx.gameover).toEqual(gameOverArg);
    // Verify that the gameover data is in the metadata.
    const { metadata } = db.fetch(id, { metadata: true });
    expect(metadata.gameover).toEqual(gameOverArg);
  });
adngdb commented 4 years ago

I created a repo with the simplest code base I could to reproduce the problem: https://github.com/adngdb/bgio-gameover-metadata

To test the problem, you can run the game (server + client), create a match with one player, join and start the match. Then click on the "Pass" button, which will end the game.

Gameover: shows the current value of ctx.gameover. You will see that it is empty before clicking, and has a value after clicking. However, if you look at the network panels of your browser, and checks the responses of the API queries, you will see that the matches list request doesn't return that gameover data at all.

I haven't been able to get further than that, and I have no clue what the problem might be. If anyone can help with this, at least to give me pointers as to where to look, I'll be glad for the help. (But as a side-note, I won't be able to work on this again in the next 10 days or so.)

adngdb commented 4 years ago

I don't understand this problem. I'm fairly sure I have the right boardgame.io code (version 0.40-alpha, I checked in my node_modules folder and it has the correct code), I believe I did not make any usage mistakes in my game's code (see the example I posted in my previous comment), but I used the FlatFile storage system and gameover never makes it to the metadata in my storage files. I'm even able to replicate the issue with a simpler test case, by directly calling ctx.events.endGame() in a move.

Given that there are plenty of unit tests that show the feature works, I suspect the problem comes from an integration problem with an actual server (since that is the only difference I could make between the passing unit tests and my test scenario).

Do you have any suggestions of where I should look at to debug this issue?

delucis commented 4 years ago

Hi Adrian, sorry I didn’t have time to look through this. I’ve just been playing around with your demo repo and it looks to me like the client never connects to the local game server (via Websockets, the game creation API works).

I added a move to add state to G. It shows up on the client thanks to optimistic updates, but also never makes it to the storage, so I’m guessing here the problem isn’t necessarily the gameover metadata logic? Not quite sure why the socket connection is failing though. Is this the same behaviour you’re seeing or do you see ctx.gameover in the state storage?

delucis commented 4 years ago

@adngdb Along the same lines as my comment in #789, using your test repo but with 0.40.0, I see the correct gameover metadata in API requests:

Screenshot 2020-09-01 at 12 22 46

Could you check to see if the new release resolves this issue?

adngdb commented 4 years ago

Yes, release 0.40.0 indeed solve that issue! Thanks a lot! :relaxed: