Persistence disk + Victory Condition NullPointer fix

doner-kebard / card-game-backend

Backend for Döner Kebard's first (yet unnamed) card game

MIT License

1 stars 0 forks source link

Persistence disk + Victory Condition NullPointer fix #19

Closed kenan-rhoton closed 5 years ago

kenan-rhoton commented 5 years ago

Description

Remove Redis to avoid unnecessary dependencies and move to saving things in the file system.

Details

No longer using carmine. Instead we now use Nippy to serialize (freeze) objects into byte arrays and to deserialize (thaw) those byte arrays back to objects, makeing it feasible to save a game to disk.

Hotspots

A bug was discovered regarding victory conditions, and its fix is included in here. The bug was an exception happening when requesting a game that had be created but had no opponent.

Checklist for contribution requirements

[x] The tests pass
[ ] The code has tests
[ ] The tests are well-designed
[x] The code is readable
[x] The code is well-organized
[x] Any rule changed or included is also on the Manual

kenan-rhoton commented 5 years ago

I've thought about it, but it's hard to test things with file input/output... I'll see if I come up with anything

Masclins commented 5 years ago

Could we have a testfile on data/ to check it's correctly readed? Or in the very same test, write it, read it and remove it at the end.

kenan-rhoton commented 5 years ago

The problem is that persistence should be Plug N' Play (which made moving from Redis to Filesystem-based persistence absolutely painless), so we can't actually test the implementation (the tests shouldn't know where the data is getting stored).

However, if the tests don't know the implementation, they can't "clean up" after themselves, so they will, in the end, leave "garbage games" saved... which is not necessariliy 100% a problem, but is something we need do with care, or players could end up playing in invalid games (for example if we ever implement matchmaking instead of just being a share link based match)

kenan-rhoton commented 5 years ago

also, we can't have any testfiles on data/ because it's on the .gitignore (which it should definitely be on :wink: )

kenan-rhoton commented 5 years ago

Soooo... I had this really convoluted plan during the weekend to make it so we could specify different storages to properly test persistence and how to organize the tests correctly, and then I realized... the only reason persistence wasn't being tested was that we were specifically excluding it through mocking and cloverage rules.

So there, removed mocking, and ta-da! Auto-tested because it is used by the API anyway. Also found a small bug in my implementation in the process and fixed it.