jkomoros / boardgame

An in-progress framework in golang to easily build boardgame Progressive Web Apps
Apache License 2.0
31 stars 4 forks source link

Ways to diff goldens and save new goldens #701

Closed jkomoros closed 4 years ago

jkomoros commented 6 years ago

Originally captured in #648

When a golden doesn't match, there should be an easy way to see where the difference was visually in moves. (There's another issue for that rewinding moves)

And then also a way to accept the new golden (that's likely just a way for golden error to output a NAME_new.json that you can choose to take or get rid of.

jkomoros commented 6 years ago

Can you pass flags to go test that are ignored by go test but available to the command? Because golden could check for them and save ones it disagrees with if it fails. (This should be a boolean into golden.Compare(), and the generated golden_test.go should have the behavior to read from the flag and pass true or false appropriately. That makes it easy to override if it doesn't do what you want, and minimizes magic)

jkomoros commented 6 years ago

One thing you often want is the same player moves, just with the new run of fixup moves inside. So you want to take an existing golden and say, replay all player moves, ignore everything else, and generate a new golden.

Maybe a golden.RegenerateFromPlayerMoves(old.json) that does this and then outputs old_regenerated.json. Then need some way to trigger this from CLI. Maybe we switch the create command to be boardgame-util golden create and then replay to boardgame-util golden replay <GAMEID.json>

Although wait, does this ever make sense? If different fix up moves apply then for example different cards will be in the draw stack, and all of the rest of th emoves afterwards will likely be logically wrong. Seems like it would be hard to convince yourself robustly that that didn't happen, so maybe this is never a good idea.

jkomoros commented 5 years ago

It does make sense in cases where the underlying state serialization format changes and you want to update the goldens with no semantic change. For example, the ShuffleCount change in 379 required a lot of error-prone by-hand surgery on goldens to do.

This is also simpler: if you know goldens are good before the commit and that the commit shouldn't change the logic, you can just tell it to save the alternate and switch to those systematically

jkomoros commented 4 years ago

Even with #764 this is still a really big pain to do manually. Splicing in a single new fixup move for example requires manually fixing up all of the Initatitor fields for all other moves in that run (since they all pointed back to an off-by-one) and then going through and manually upping by one the IDsLastSeen for every other move after that move was injected (barf!)

Even just something that allows you to interactively step through and diff goldens and say, "Yes, that move was OK" would be great.

But yeah, the mode where it outputs a new golden if it disagrees and a flag is passed would be best.

jkomoros commented 4 years ago

Basically what it should do in the update case is apply any SeatPlayers, Timers, or player moves, skipping ahead to the next of those when the game is waiting. It should skip over any moves in the golden record that aren't that, ticking off moves that do match (ignoring versions and initiators and times).

The logic is considerably different than normal compare, because you need a new file encoding storage object to start.

jkomoros commented 4 years ago

Filesystem storage layer gets an inMemory config in its NewStorageManager. If true, then it ignores basePath, and instead has filesystem.records that are never saved to disk, and records for any game not yet seen this session will get a blank record. RecordForID is exposed; if you use it in memory mode, then you're responsible for fetching the record you care about and persisting to disk when you want.

Note that this will likely require api.Build to be tweaked to support passing an extra argument. api.build.StorageType.Constructor() will need to emit a ', false` as well. And boardgame_util.cmd_golden will need to pass ", false" (since it also passes in a literal storage arg)

jkomoros commented 4 years ago
jkomoros commented 4 years ago

The logic for RegenerateGolden will be non-trivial.

Conceptually, when playing forward a game, you apply a stream of fixup moves until you get to a stopping point. A stopping point is when the run of fixups stops. The stopping point will you leave you at one of three options: 1) a player move, 2) a SeatPlayer move, 3) a timer fire.

Conceptually the live game will stop, and then you need to see the next move to apply, which is the next stopping point in the golden.

This would be very hard in general, having to do with minimal edit distance between moves. But luckily we have a secret weapon: the Initiator field. In the golden, we just need to advance the lastVerifiedMove counter to the next time that Initiator is itself.

Ideally we'd still also want a gutcheck that the skips are natural, by e.g. visualizing the diff between moves in the golden and live game's moves between then.

This will work in general, but not if there's a new stopping point move introduced inside of what was a contiguous run before. (But that seems quite rare... you'd have to change the stopping logic to wait for a player move)

In general, inserting fixup moves into existing fixup moves runs can be done automaticlaly. But introducing new playerMoves or SeatPlayer moves at discontinugous moments can't be done with this methodology. You'd need a way to describe where to insert a new player move, without affecting the golden (the rest of the golden will have e.g. IDsLastSeen set to the wrong value).

So the procedure is, if you're inserting new Fixupmoves in existing runs, just do updategoldens. If you want to inject new initator moves, (e.g. SeatPlayer, or any normal player move) then you need to manually fix up the moves list to inject the move where you want it to go. Crucially, you do NOT have to update the state lists, because those states aren't verified anyway. Note an annoying thing about this: the golden will be overwritten, so your manual fix up will be discarded, so if it did the wrong thing you'll have to keep manually patching it in (barf).