New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checkmate Tests #67

Merged
merged 16 commits into from Dec 30, 2015

Conversation

Projects
None yet
2 participants
@jonathanpike
Copy link
Contributor

jonathanpike commented Dec 23, 2015

I've added more tests for checkmate, which were taken from classic checkmate patterns. Unfortunately, that revealed some flaws in our checkmate method.

Checkmate essentially involves 3 checks:

  1. Can a piece obstruct the attacker?
  2. Can a piece capture the attacker?
  3. Can the king move away?

In our case, it also involves checking the state of the game. For example, if you check if any of your pieces can capture the attacker, one needs to keep in mind that there be something like a dovetail check, where if the first piece is captured, the second piece already has your King in check (and therefore, you're in checkmate).

Using any of these three checks individually without also checking the state of the game after the move has been carried out will give false results.

After looking at our options, I do believe that using something like a Transaction is still our best bet, since it will capture the state of the game after the move has been carried out. I've refactored that method, but you'll see that it still doesn't pass all of the tests. I've also written 2 of the 3 checks independently, but they suffer from the state problem that I mentioned.

Any ideas would be great!

@jonathanpike

This comment has been minimized.

Copy link
Contributor

jonathanpike commented Dec 23, 2015

I got it working! I think that the code needs to be refactored, as checkmate is pretty complex, but it passes the tests!

What I noticed when I was running my tests manually in Rails console was that the pieces were not rolling back to their original positions when the transaction ended. Solution? I store their positions prior to the transaction and then make sure to update attributes after the transaction to keep them where they should be. This solves the problem of the transaction seemingly checking invalid moves (they weren't invalid -- the piece had just moved).

Any comments about how to best refactor, or any suggestions for more tests would be appreciated.

@jonathanpike

This comment has been minimized.

Copy link
Contributor

jonathanpike commented Dec 24, 2015

I just refactored a whole lot of stuff and committed it to this branch. Here are a few highlights:

  1. checkmate is now much simpler. I moved the transaction to a private method, which cleared things up.
  2. try_to_move is also much simpler. I split off some of the functionality into separate methods. I also refactored the entire piece model by moving some methods into private.
  3. I moved check and checkmate tests into a separate file, to keep the game tests file more organized. I also did the same thing for the en passant tests, to keep the pawn tests more organized.
orig_row, orig_col = piece.row_position, piece.col_position
status = false unless checkmate_check(piece, row, col)
# If transaction moves piece, move it back to original position
piece.update_attributes(row_position: orig_row, col_position: orig_col)

This comment has been minimized.

@takehiromouri

takehiromouri Dec 30, 2015

This might actually potentially cause bugs since for pawns, we are storing the past updates using ActiveRecord Dirty. If we use transactions, everything is rolled back so this problem doesn't exist.

I figured out why transactions wasn't working - we simply had to reload the pieces with .reload I think

@jonathanpike

This comment has been minimized.

Copy link
Contributor

jonathanpike commented Dec 30, 2015

Thanks to @takehiromouri for figuring out that you could use reload rather than update_attributes! Great catch!

I've also found a bug with castling -- it would check all Rooks on the board, not just Rooks that belong to you. I've fixed that bug here.

@jonathanpike jonathanpike force-pushed the Checkmate_Tests branch from 708d386 to 5e92107 Dec 30, 2015

check_status = false if determine_check == false
fail ActiveRecord::Rollback
end
next unless piece.reload.valid_move?(row, col)

This comment has been minimized.

@takehiromouri

takehiromouri Dec 30, 2015

I think we need to add the whole
Piece.transaction do
piece.reload.try_to_move(row, col)
check_status = false if determine_check == false
fail ActiveRecord::Rollback
end
block here :)

This comment has been minimized.

@jonathanpike

jonathanpike Dec 30, 2015

Contributor

Look at line 94 -- the checkmate_check method. You'll see what you're looking for.

This comment has been minimized.

@takehiromouri

takehiromouri pushed a commit that referenced this pull request Dec 30, 2015

Takehiro Mouri

@takehiromouri takehiromouri merged commit 7a809f4 into master Dec 30, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jonathanpike jonathanpike deleted the Checkmate_Tests branch Dec 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment