Add unit tests for game state #122
Conversation
These tests verify the behaviour of the game state as displayed on the main scoreboard (except Jammer names). Not included: - Undo. This will be added together with the switch to a snapshot model in a separate PR. - Game Statistics (Penalties, Lineups) - Settings other than WFTDA sanctioned (with a few exceptions).
| protected boolean checkStop() { | ||
| return (getTime() == (isCountDirectionDown() ? getMinimumTime() : getMaximumTime())); | ||
| protected boolean isDisplayChange(long current, long last) { | ||
| //on count down clocks are rounded down for display, on count up they are rounded down. |
There was a problem hiding this comment.
This comment reads oddly, and I don't understand why the directions have different logic.
| @Test | ||
| public void syncTimeTest() { | ||
| public void test_reset() { |
There was a problem hiding this comment.
Can we be consistent in how we name tests/methods? camelCase seems to be our standard.
There was a problem hiding this comment.
Eclipse auto generates tests with testMethodName or testMethodNameArgumentTypes (when there are multiple methods with the same name). Where I have multiple tests for one method I use testMethodName_scenario as I find that makes it easier to see at a glance what is being tested here. I'll change the older tests in ClockModel to that format.
| clock.addScoreBoardListener(new ConditionalScoreBoardListener(clock, Clock.EVENT_DIRECTION, listener)); | ||
|
|
||
| clock.setCountDirectionDown(true); | ||
| advance(0); |
There was a problem hiding this comment.
advance(0) seems a bit weird, maybe it'd be clearer to say that you're waiting for the events to propagate?
There was a problem hiding this comment.
I copied that from you. :)
And after using it 5 times it becomes pretty natural.
|
|
||
| @Test | ||
| public void testStartOvertime_default() { | ||
| ClockModel pc = sbm.getClockModel(Clock.ID_PERIOD); |
There was a problem hiding this comment.
We should probably have these as part of the object, as they're used a lot.
| ClockModel lc = sbm.getClockModel(Clock.ID_LINEUP); | ||
| ClockModel tc = sbm.getClockModel(Clock.ID_TIMEOUT); | ||
| ClockModel ic = sbm.getClockModel(Clock.ID_INTERMISSION); | ||
| ((DefaultSettingsModel) sbm.getSettings()).set("Clock." + Clock.ID_LINEUP + ".OvertimeTime", "60000"); |
There was a problem hiding this comment.
You can use getSettingsModel
| assertEquals(5, event.getValue()); | ||
| assertEquals(0, event.getPreviousValue()); | ||
|
|
||
| //values higher than score are truncated |
There was a problem hiding this comment.
s/truncated/clamped/
Truncation is a type of rounding.
|
And yes, it was rather tedious. |
|
Thanks! |
| advance(0); | ||
|
|
||
| assertFalse(pc.isRunning()); | ||
| assertEquals(2, pc.getNumber()); |
There was a problem hiding this comment.
This is failing. I'm not sure what this is meant to be testing, as there's no jam running when you stop it.
There was a problem hiding this comment.
stopJam() is not accurately named anymore as it also stops Timeouts or starts a lineup clock before period start. This test is for the latter behaviour. (My clock code overhaul will be renaming the function to stopJamTO())
The failure is caused by the change in #123: The new period number is no longer based off the old period number but off the intermission number, which the test doesn't set to 1, as it should be after period 1. A fix is included in the clock overhaul (which I will be submitting shortly).
There was a problem hiding this comment.
I was wondering if it was a merge conflict.
There was a problem hiding this comment.
And small correction: The fix for the test is already in #124, not in the upcoming rewrite. (Which I now decided to hold back as it's buit atop #124, which in turn is built atop this PR, amassing a lot of old commits that show up as part of the new PR though they aren't. I want to do a rebase to get the spurious commits removed before submitting. But this is best done after #124 is merged.)
There was a problem hiding this comment.
That needs rebasing before merging, as I'm not quite sure what Github will do otherwise.
There was a problem hiding this comment.
Are you talking about #124? Then we should probably move the discussion there.
These tests verify the behaviour of the game state as displayed on the main
scoreboard (except Jammer names).
Not included:
in a separate PR.
In addition the PR contains fixes for a few minor bugs uncovered by the tests.