Skip to content

Improve secret-handshake assertions with AssertJ #2061

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from Dec 8, 2021
Merged

Improve secret-handshake assertions with AssertJ #2061

merged 2 commits into from Dec 8, 2021

Conversation

jmrunkle
Copy link
Contributor

@jmrunkle jmrunkle commented Dec 3, 2021 •
edited
Loading

pull request

Fixes #2060

Basically, the current assertions are horribly broken, but this fixes that.

Reviewer Resources:

Track Policies

@jmrunkle jmrunkle requested a review from a team December 3, 2021 03:44
@ericbalawejder
Copy link
Contributor

ericbalawejder commented Dec 3, 2021 •
edited
Loading

I see. The tests were checking for a collection type as well as the contents? Collections.singletonList(), .emptyList() as well as the Signals? I'm having difficulty reproducing this error. I'm using a stream collector .collect(Collectors.toUnmodifiableList()); with 0 - 4 Signals.

ericbalawejder
ericbalawejder reviewed Dec 3, 2021
View reviewed changes
exercises/practice/secret-handshake/src/test/java/HandshakeCalculatorTest.java
public void setUp() {
handshakeCalculator = new HandshakeCalculator();
}
private final HandshakeCalculator handshakeCalculator = new HandshakeCalculator();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the object has no state, it doesn't matter if we remove the initialization from the @Before block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and even if it did have state, each @Test is actually run independently so you would only possibly get an issue if they were running in parallel. Frankly, if a student's solution has any mutable state, that is a bad implementation of this exercise. It may eventually be worth adding a test that does multiple calculations in a row and making sure it always has the correct results across multiple runs (eg. not re-using a mutable list for each calculation).

@jmrunkle
Copy link
Contributor Author

jmrunkle commented Dec 4, 2021

The tests were checking for a collection type as well as the contents? Collections.singletonList(), .emptyList() as well as the Signals? I'm having difficulty reproducing this error. I'm using a stream collector .collect(Collectors.toUnmodifiableList()); with 0 - 4 Signals.

I believe that it considers equality based on the equals method, so as long as your result matches returns true for equals then it should pass the test (but I am pretty sure that is too restrictive). The user who submitted the issue was using an ArrayList, so you could try something like this:

.collect(Collectors.toCollection(ArrayList::new))

@jmrunkle
Copy link
Contributor Author

jmrunkle commented Dec 8, 2021

Anything else @ericbalawejder?

ericbalawejder
ericbalawejder approved these changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@ericbalawejder ericbalawejder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertions are more clear but I still can't reproduce the error.

@jmrunkle jmrunkle merged commit 31b5f7a into exercism:main Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericbalawejder ericbalawejder ericbalawejder approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix assertions for secret-handshake
2 participants
@jmrunkle @ericbalawejder