Simplify tests

[leesjensen]

Several TAs were having trouble parsing the castling tests and the equality tests.

We should review these and see if they can be simplified.

   @Test
    @DisplayName("HashCode Testing")
    public void hashTest() {
        Assertions.assertEquals(original.hashCode(), equivalent.hashCode(),
                className + ".hashCode() returned different values for equivalent " + itemsPlural);
        for (var different : allDifferent) {
            Assertions.assertNotEquals(original.hashCode(), different.hashCode(),
                    className + ".hashCode() returned the same value for different " + itemsPlural);
        }
    }

    @Test
    @DisplayName("Equals & HashCode Testing")
    public void hashSetTest() {
        Set<T> set = new HashSet<>();
        set.add(original);

        // Manually test insertion of original & equal items
        Assertions.assertTrue(set.contains(original),
                "[" + className + "] Original item should exist in collection after adding original item");
        Assertions.assertTrue(set.contains(equivalent),
                "[" + className + "] Equivalent item should exist in collection after only adding original item");
        Assertions.assertEquals(1, set.size(),
                "[" + className + "] Collection should contain only 1 item after a single insert");
        set.add(equivalent);
        Assertions.assertEquals(1, set.size(),
                "[" + className + "] Collection should still contain only 1 item after adding equivalent item");

        // Programmatically test insertion of all different items
        int expectedSetSize = 1;
        for (var different : allDifferent) {
            Assertions.assertFalse(set.contains(different),
                    "[" + className + "] Different item should not be present in set before insertion");
            set.add(different);
            expectedSetSize++;
            Assertions.assertEquals(expectedSetSize, set.size(),
                    "[" + className + "] New item was counted as different during insertion");
        }

    }

[ZakkeryDaRebel]

I understand needing to simplify the equals tests to make them a bit more easy to understand. But what was the problem with the castling tests again? I was talking with @johno4472 and he was saying the issue was just that the failed tests weren't clear enough.

Taking a look at the castling tests, we can change the error messages to say "You shouldn't be able to castle queen side but you said you could" or "You should be able to castle king side but you didn't include it as a valid move". Would that make the tests easier to understand? But honestly, the castling tests don't look too complex, was there a specific part that anyone had problems with?

[Maillman]

I'm just going to put this out there in case anyone wants to look at it.

Potential Idea

If we are planning to keep the EqualsTestingUtility class (which IMO we probably don't want to keep, but I'm putting this out there anyway), then by using a similar approach that I used for #41, I modified the EqualsTestingUtility file on a branch (modify-equals-testing-utility) of my JavalinChess repository so the error messages themselves were a little more helpful. So instead of a user who is starting to implement their ChessGame class and forgetting to reset the board, seeing an error message like this:

Equals & HashCode Testing

[ChessGame] Different item should not be present in set before insertion

HashCode Testing

ChessGame.hashCode() returned the same value for different games

Equals Testing

ChessGame.equals() returned true for different games

they will now see something like:

Equals & HashCode Testing

[ChessGame] Different item should not be present in set before insertion
Comparing Chess Game with Moved pawn:
chess.ChessGame@633ab8c2
against rest of
Chess Game with Preset board:
chess.ChessGame@a2b5b66e
Chess Game with Default board:
chess.ChessGame@633ab8c2

HashCode Testing

ChessGame.hashCode() returned the same value for different games
Comparing Chess Game with Default board:
chess.ChessGame@633ab8c2
against rest of
Chess Game with Moved pawn:
chess.ChessGame@633ab8c2

Equals Testing

ChessGame.equals() returned true for different games
Comparing Chess Game with Default board:
chess.ChessGame@633ab8c2
against rest of
Chess Game with Moved pawn:
chess.ChessGame@633ab8c2

It will attempt to print out the toString() of the item being compared, after giving a brief description of that item. I just didn't have a toString() for the ChessGame class.

Another thing to note: The modified implementation that I wrote is a little messy and somewhat rushed (I had done most of this last night for about 30 minutes) just to see what a functional implementation would look like.

Pros and Cons

This has the benefit of providing more helpful failure messages. However, this makes the EqualsTestingUtility file even harder to read, which definitely runs counter to what we are trying to achieve with this issue. All this really accomplishes is by providing more helpful failure messages, students are less likely to have to debug through the tests to see what the issue is.

What I would do

If it were up to me, I would probably nuke the entire EqualsTestingUtility file and not have our chess-related testing classes extending from the abstract EqualsTestingUtility class. But I just wanted to showcase a potential solution since there was none currently out there at the moment.

[ZakkeryDaRebel]

I think this is definitely one solution we can do. The current problem is that students are struggling to understand EqualsTestingUtility. Since all the tests are implementing it, we could easily remove that implementation to make things easier to debug. But then again, the tests work pretty well, and we would start introducing duplicate code. So if we want a simple fix, I feel like changing the failure messages like you have here will work really well. It helps the student not need to try and debug the test, because they will be told what the problem is, and they will have to debug their own code to see what the problem is. The more clear the Tests we write are, the less time they need to spend in our tests and instead in their own code.

[leesjensen]

I haven't fully thought this through, but I'm not sure printing out the chess game in the messages is going to make anything easier to read. Those things are pretty big.

I found the new messages hard to parse and perhaps more confusing than the original message even though the old messages lack context.

Do they even need to implement hashcode and equals on the Game? Does the game ever get stuck in a collection? Or is that only so the tests can determine equality?