Tests: standardize the way we check for thrown exceptions #1007
Conversation
|
Click here to submit a new iteration when this PR is ready for review. See this repository's contribution guide for more information. |
sijie123
changed the title
v1@sijie123 submitted v1 for review.
Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/1007/1/head:BRANCHNAMEwhere |
|
Range diff with #959 v8: |
There was a problem hiding this comment.
[1/5]
Assert: Standardize assert statements to use static import
Why was the chosen scope "Assert"? The scope is all of our test code, so either use "tests:" or omit the scope completely.
Also, these aren't assert statements (they have a specific meaning)
Should be something like:
Standardize Assert#assertThrows(...) calls to use static import
We use a mixture of calling the static Assert.assertThrows(..) and calling a statically imported Assert#assertThrows. This can make it confusing for readers, as we have 2 different ways of calling the same assertThrows. Let's standardise, across all tests, to use a statically imported Assert#assertThrows. This also has the benefit of making our code less verbose and easier to read.
There was a problem hiding this comment.
[2/5]
LogicManagerTest: Split assertCommandBehavior into assertCommandSuccess and assertCommandFailure When given an exception, assertCommandBehavior ensures that the correct exception is thrown. When given null, assertCommandBehavior ensures that no exception is thrown. While the function cleverly avoids the use of if/else statements, it is difficult for a reader to understand the flow of assertCommandBehavior. It is also responsible for two distinct tasks and violates the single responsibility principle. Additionally, assertCommandBehavior is not descriptive - what behaviour are we assuring ourselves? When assertCommandBehavior was first introduced, no documentation was made on the design choice. It is likely that this was implemented because JUnit did not provide an API to check for thrown exceptions. With a single method able to check for both exceptions and its lack thereof, it made sense to implement assertCommandBehavior to reduce code repetition. Now, with Assert#assertThrows, the call to checking for thrown exceptions is just one line. The cost in difficulty in understanding how assertCommandBehavior works and its violation of Single Responsibility Principle now outweighs the benefits of DRY.
Code repetition doesn't necessarily imply a DRY-violation, unless it can be
shown that the two pieces of "knowledge" are the same. In this case, as has
been shown above, "asserting that a command is successful" and "asserting that
a command is not successful" are obviously not the same.
Let's separate assertCommandBehavior into assertCommandSuccess and assertCommandFailure instead, to make our code easier to understand and more in line with the Single Responsibility Principle.
There was a problem hiding this comment.
[3/5]
Thrown exception checking: Migrate to use AssertThrows We use a mix of 4 different ways to test exception throwing. We use: * try/catch - catch and ensure we receive the correct exception. * Assert#assertThrows * JUnit#assertThrows * JUnit#ExpectedException.
The # syntax was slightly abused here, but OK.
All these methods semantically do the same thing. To streamline and standardise our code, we should only use one way to check for thrown exceptions. Rationale: developers only need to learn one style of implementation. The try/catch method is implemented differently in each file that requires it and violates the DRY principle. JUnit#assertThrows provides a shorter syntax and is easier to read as compared to ExpectedException. We should thus aim to use the assertThrows style. In Assert.java, we implement custom methods that wraps over JUnit#assertThrows to provide additional functionality such as testing for correctness of error messages shown to users. With Assert#assertThrows providing a superset of required functionality, it makes sense to use Assert#assertThrows as the standardised way to test for thrown exceptions. Let's mechanically convert all our exception checking to use Assert#assertThrows.
Mechanical conversion (as used in the contribution guide, which I think
you are referring to here) means that you have a one-shot script that
reviewers can run on their own computers to check if their result
matches the commit's diff. This doesn't seem to be the case here since
the changes all have small variations.
Now, that's completely fine, but:
Note: Two more classes - StringUtilTest and IndexTest - implement encapsulation over checking for thrown exceptions, and are more complex to migrate. These files will be migrated in separate commits to keep the scope of this commit small.
Given that reviewers have to review the whole diff anyway, it seems like
[4/5] and [5/5] should be squashed into here, especially since their
commit messages are repeating whatever is said here / what can be easily
seen from their diffs.
sijie123
force-pushed
the
simplifyThrows
branch
2 times, most recently
from
9420f54
to
288d048
Compare
v2@sijie123 submitted v2 for review.
(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/1007/2/head:BRANCHNAMEwhere |
There was a problem hiding this comment.
I spotted the following stuff which may/may not be intended. (I can see that either way, others may have differing opinions) Let me know if you want to change them, but for me I think it's good enough.
Except maybe the toModelType() which certainly does look like it's being inconsistent, but we also don't have an official rule of when to use the :: syntax.
| * Also confirms that {@code expectedModel} is as specified. | ||
| * @see #assertCommandBehavior(Class, String, String, Model) | ||
| * Executes the command and confirms that | ||
| * - no exceptions are thrown <br> |
There was a problem hiding this comment.
We are now just propagating the exception, do we still consider that "confirming that no exceptions are thrown"?
src/test/java/seedu/address/storage/JsonSerializableAddressBookTest.java
Outdated
Show resolved
Hide resolved
sijie123
force-pushed
the
simplifyThrows
branch
from
288d048
to
15ac83e
Compare
v3@sijie123 submitted v3 for review.
(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/1007/3/head:BRANCHNAMEwhere |
sijie123
force-pushed
the
simplifyThrows
branch
from
15ac83e
to
2e6a419
Compare
|
Oops. Accidentally CIHR-ed with a bug in it. Please ignore v3 and look at v4 instead.
|
v4@sijie123 submitted v4 for review.
(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/1007/4/head:BRANCHNAMEwhere |
|
@sijie123 Thanks, I think you'll need to squash this into [3/3] as well though: diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java
index 5291aac9..9dfdb20a 100644
--- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java
+++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java
@@ -150,12 +150,12 @@ public class ParserUtilTest {
}
@Test
- public void parseTag_null_throwsNullPointerException() throws Exception {
+ public void parseTag_null_throwsNullPointerException() {
assertThrows(NullPointerException.class, () -> ParserUtil.parseTag(null));
}
@Test
- public void parseTag_invalidValue_throwsParseException() throws Exception {
+ public void parseTag_invalidValue_throwsParseException() {
assertThrows(ParseException.class, () -> ParserUtil.parseTag(INVALID_TAG));
}
@@ -173,12 +173,12 @@ public class ParserUtilTest {
}
@Test
- public void parseTags_null_throwsNullPointerException() throws Exception {
+ public void parseTags_null_throwsNullPointerException() {
assertThrows(NullPointerException.class, () -> ParserUtil.parseTags(null));
}
@Test
- public void parseTags_collectionWithInvalidTags_throwsParseException() throws Exception {
+ public void parseTags_collectionWithInvalidTags_throwsParseException() {
assertThrows(ParseException.class, () -> ParserUtil.parseTags(Arrays.asList(VALID_TAG_1, INVALID_TAG)));
}
|
sijie123
force-pushed
the
simplifyThrows
branch
from
2e6a419
to
e164f8d
Compare
|
Hmm... How did I miss that one out >< |
|
CIHR pls. |
v5@sijie123 submitted v5 for review.
(📚 Archive) (📈 Interdiff between v4 and v5) (📈 Range-Diff between v4 and v5) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/1007/5/head:BRANCHNAMEwhere |
|
[1/3] JsonAdaptedTest#95-100 @Test
public void toModelType_nullAddress_throwsIllegalValueException() {
JsonAdaptedPerson person = new JsonAdaptedPerson(VALID_NAME, VALID_PHONE, VALID_EMAIL, null, VALID_TAGS);
String expectedMessage = String.format(MISSING_FIELD_MESSAGE_FORMAT, Address.class.getSimpleName());
Assert.assertThrows(IllegalValueException.class, expectedMessage, person::toModelType);
}This test (along with a few others) are still using |
pyokagan
changed the title
There was a problem hiding this comment.
[3/3]
In Assert.java, we implement custom methods that wraps over
JUnit#assertThrows to provide additional functionality such as testing
for correctness of error messages shown to users.
I don't think so :P https://github.com/se-edu/addressbook-level4/blob/master/src/test/java/seedu/address/testutil/Assert.java
We use a mixture of calling the static Assert.assertThrows(...) and calling a statically imported Assert#assertThrows. This can make it confusing for readers, as we have 2 different ways of calling the same assertThrows. Let's standardise, across all tests, to use a statically imported Assert#assertThrows. This also has the benefit of making our code less verbose and easier to read.
…ss and assertCommandFailure When given an exception, assertCommandBehavior ensures that the correct exception is thrown. When given null, assertCommandBehavior ensures that no exception is thrown. While the function cleverly avoids the use of if/else statements, it is difficult for a reader to understand the flow of assertCommandBehavior. It is also responsible for two distinct tasks and violates the single responsibility principle. Additionally, assertCommandBehavior is not descriptive - what behaviour are we assuring ourselves? When assertCommandBehavior was first introduced, no documentation was made on the design choice. It is likely that this was implemented because JUnit did not provide an API to check for thrown exceptions. With a single method able to check for both exceptions and its lack thereof, it made sense to implement assertCommandBehavior to reduce code repetition. Now, with Assert#assertThrows, the call to checking for thrown exceptions is just one line. The cost in difficulty in understanding how assertCommandBehavior works and its violation of Single Responsibility Principle now outweighs the benefits of the reduced code repetition. Let's separate assertCommandBehavior into assertCommandSuccess and assertCommandFailure instead, to make our code easier to understand and more in line with the Single Responsibility Principle.
Thanks for the catch. I've gone one more round to make sure that all these static imports are included. Should be good now.
Thanks. I cherry-picked this one over, so I forgot that I had not updated Assert.java in this PR. |
sijie123
force-pushed
the
simplifyThrows
branch
from
e164f8d
to
d85533a
Compare
v6@sijie123 submitted v6 for review.
(📚 Archive) (📈 Interdiff between v5 and v6) (📈 Range-Diff between v5 and v6) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/1007/6/head:BRANCHNAMEwhere |
There was a problem hiding this comment.
[3/3]
All references to JUnit#assertThrows should be removed, no?
Thrown exception checking: Migrate to use AssertThrows We use a mix of 4 different ways to test exception throwing. We use: * try/catch - catch and ensure we receive the correct exception. * Assert#assertThrows * JUnit#assertThrows * JUnit#ExpectedException.
here,
All these methods semantically do the same thing. To streamline and standardise our code, we should only use one way to check for thrown exceptions. Rationale: developers only need to learn one style of implementation. The try/catch method is implemented differently in each file that requires it and violates the DRY principle. JUnit#assertThrows provides a shorter syntax and is easier to read as compared to ExpectedException. We should thus aim to use the assertThrows style.
and here.
In Assert.java, we implement custom methods that provide additional functionality such as testing for correctness of error messages shown to users. With Assert#assertThrows providing a superset of functionality as compared to all other ways, it makes sense to use Assert#assertThrows as the standardised way to test for thrown exceptions. Let's convert all our exception checking to use Assert#assertThrows. In doing so, some tests no longer need to declare to throw exceptions. Let's clean up our code to remove these unnecessary declarations. Additionally, two more classes - StringUtilTest and IndexTest - implement methods to encapsulate the idea of checking for thrown exceptions. Now that we can directly call Assert#assertThrows, these abstractions provide little benefit. Let's remove them to simplify our code base.
|
I meant to say that we use JUnit's Assertions.assertThrows in some places, and that we should change these instances to Assert#assertThrows. That said,
I do think that this has an unnecessary reference to JUnit. |
We use a mix of 4 different ways to test exception throwing. We use: * try/catch - catch and ensure we receive the correct exception. * Assert#assertThrows * JUnit#assertThrows * JUnit#ExpectedException. All these methods semantically do the same thing. To streamline and standardise our code, we should only use one way to check for thrown exceptions. Rationale: developers only need to learn one style of implementation. The try/catch method is implemented differently in each file that requires it and causes unnecessary code duplication. The assertThrows style, in comparison, provides a shorter syntax and is easier to read as compared to the ExpectedException style or the try/catch style. We should thus aim to use the assertThrows style. In Assert.java, we implement custom methods that provide additional functionality such as testing for correctness of error messages shown to users. With Assert#assertThrows providing a superset of functionality as compared to all other ways, it makes sense to use Assert#assertThrows as the standardised way to test for thrown exceptions. Let's convert all our exception checking to use Assert#assertThrows. In doing so, some tests no longer need to declare to throw exceptions. Let's clean up our code to remove these unnecessary declarations. Additionally, two more classes - StringUtilTest and IndexTest - implement methods to encapsulate the idea of checking for thrown exceptions. Now that we can directly call Assert#assertThrows, these abstractions provide little benefit. Let's remove them to simplify our code base.
sijie123
force-pushed
the
simplifyThrows
branch
from
d85533a
to
5cabb37
Compare
I thought @Zhiyuan-Amos pointed out in #1007 (review) that JUnit 4 doesn't have assertThrows? |
Yup, JUnit 4 doesn't have assertThrows. I'll need to be careful not to imply that >< This Assertions.assertThrows exists because our existing code already uses it in some places (this commit introduced one). Back then, we already include the JUnit 5 library, so the tests could use Assertions.assertThrows. |
Oh I see, we are using JUnit5's assertThrows in one place. Well, the wonders of having two versions of JUnit at a single time I guess. |
v7@sijie123 submitted v7 for review.
(📚 Archive) (📈 Interdiff between v6 and v7) (📈 Range-Diff between v6 and v7) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/1007/7/head:BRANCHNAMEwhere |
We have many different ways of checking for thrown exceptions. These are not standardized, and make it difficult for a reader because they have to learn the different ways of throwing exceptions.
Let's standardize the way we test error handling behavior, specifically towards using Assert#assertThrows.