Fix make test (Issue #14)

[yoyoma2]

As the title states, this pull request fixes "make test" thus closing Issue #14. The failing tests were all using the ace of spades as the current card in both the source and destination stacks which can never be a valid move. The valid_move() function actually works fine as long as the test code sends valid cards when it expects a valid return code.

NB: This pull request only touches test code and makes absolutely no changes to game code.

[greno4ka]

Yes, the function valid_move is correct, so obviously the problem in tests. I wanted to make the same changes, but dont you think, that these modifications will change the sense of these tests? Have you looked on other tests?

[yoyoma2]

dont you think, that these modifications will change the sense of these tests? Have you looked on other tests?

Since only the ace of spades was used as source and destination card in all of the failed tests they were all performing the same test (ace clubs to ace clubs = fail). By using different cards, these tests now perform the test intended by their function names. Changing all of the assert() statements to always expect failure would be an easier fix but that would not accomplish the intention of the tests.

Other tests could in fact be improved to be more complete but this change only fixes the failed tests.

Added a bunch of memory free calls to fix all the numerous memory leaks reported by valgrind.

[greno4ka]

Yeah, nice changes! Let's hope, that @mpereira can see them as soon as posible and pull them in new release=)

[mpereira]

Yes, don't worry I'm seeing the recent PRs. I'll be taking a look at them soon 🙂

[yoyoma2]

Noticed the first card of the first manoeuvre stack was often an ace of diamonds on startup. It turns out the shuffle_deck() function wasn't shuffling the last position. You can still be lucky and another card gets randomly shuffled to that position but start the game 52 times and you'll see the ace of diamonds too often in the first manoeuvre stack. Removing the -1 insures the entire deck gets shuffled. Also on the following line the adding and subtracting of the variable i was deleted since it does nothing to the result of the modulo calculation.

[greno4ka]

That's really cool observation!

[mpereira]

Hi @yoyoma2,

Thanks for your PR. I had some time today to take a careful look at your changes and they look great! Nice spot with the off-by-one issue in shuffle_deck. It would be nice to have some tests for it at some point...

I'll merge this and do a release.

[mpereira]

By the way, would you be able to share the valgrind command you used? I'll add a Makefile target for it, and soon hook this repository with CI.

[yoyoma2]

I just put valgrind before the non-stripped/debuggable executable as below. Then played the game and a memory report is printed to the terminal on exiting the game. All leaks were gone.

$ valgrind ttysolitaire
==24173== Memcheck, a memory error detector
==24173== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==24173== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==24173== Command: ttysolitaire
==24173== 
==24173== 
==24173== HEAP SUMMARY:
==24173==     in use at exit: 259,066 bytes in 347 blocks
==24173==   total heap usage: 1,941 allocs, 1,594 frees, 541,218 bytes allocated
==24173== 
==24173== LEAK SUMMARY:
==24173==    definitely lost: 0 bytes in 0 blocks
==24173==    indirectly lost: 0 bytes in 0 blocks
==24173==      possibly lost: 0 bytes in 0 blocks
==24173==    still reachable: 259,066 bytes in 347 blocks
==24173==         suppressed: 0 bytes in 0 blocks
==24173== Rerun with --leak-check=full to see details of leaked memory
==24173== 
==24173== For counts of detected and suppressed errors, rerun with: -v
==24173== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
$

[yoyoma2]

If you are automating tests then this command is more useful. It also gave zero memory errors after the pull request but I'm not sure it tests as many code paths as the real game.

$ valgrind ttysolitaire_test 
==24428== Memcheck, a memory error detector
==24428== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==24428== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==24428== Command: ttysolitaire_test
==24428== 
==24428== 
==24428== HEAP SUMMARY:
==24428==     in use at exit: 0 bytes in 0 blocks
==24428==   total heap usage: 639 allocs, 639 frees, 11,992 bytes allocated
==24428== 
==24428== All heap blocks were freed -- no leaks are possible
==24428== 
==24428== For counts of detected and suppressed errors, rerun with: -v
==24428== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
$

[mpereira]

Yeah, for CI running valgrind on the test executable might be more fitting. Thanks for sharing.

[yoyoma2]

Oh and the "in use at exit" in the game's case is not ttysolitaire's fault but a normal side effect of using ncurses as explained here.

[mpereira]

Thanks for the context 🙂