-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the missing API error messages should be included here in this commit (at least the API generated ones, Rules generated ones may go separately)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A success test for two cards correctly played by both players is missing, right?
Also invalid player-id/game-id |
backend/src/api/base.clj
Outdated
@@ -13,12 +13,16 @@ | |||
(player-view/get-game-as-player (persistence/fetch-game game-id) player-id)) | |||
|
|||
(defn play-card-as-player | |||
"Plays a card give and returns the game sate as seen by the player" | |||
[game-id player-id card-id row-id & target] | |||
(let [game-state (persistence/fetch-game game-id)] | |||
(if (= (:status (get-game game-id player-id)) messages/play) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if game-state
is nil
? (Not in the database)
p2 (:player-id (base/add-player game-id))] | ||
|
||
(expect {:error messages/invalid-id} | ||
(base/play-card-as-player 9999999999 p1 0 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenan-rhoton apparently "mocking" is making this test not as good as it should be
[game-id player-id card-id row-id & target] | ||
(let [game-state (persistence/fetch-game game-id)] | ||
(if (and (= game-id (:game-id game-state)) | ||
(or (= player-id (first (:player-ids game-state))) | ||
(= player-id (second (:player-ids game-state))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this can be done prettier... But how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open an issue to clean this up cause it would involve changes to mocking, so I dont forget
This should complete the tests of current
api/base.clj
, solving a point from #55Resolves #97 (at least the needed messages we came up with)