You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "Taher-Ghaleb (via GitHub)" <gi...@apache.org> on 2023/08/21 05:34:44 UTC
[GitHub] [accumulo] Taher-Ghaleb opened a new pull request, #3712: Refactor assertTrue(!=) with assertNotEquals & Add explanatory messages
Taher-Ghaleb opened a new pull request, #3712:
URL: https://github.com/apache/accumulo/pull/3712
I am working on research that investigates test smell refactoring in which we identify alternative implementations of test cases, study how commonly used these refactorings are, and assess how acceptable they are in practice.
The first smell is when inappropriate assertions are used, while there exist better alternatives. For example, in [SummaryReportTest.java](https://github.com/apache/accumulo/commit/a657b6ee75d7c6a0556d0059dc3bc1317594d34b#diff-d0c6de0d5f496d76d1c71a7b39cb5ddc4060d8c68ddcf9893fd1f7fd5395d626), I refactored `assertTrue(report.getReportTime() != 0);` using `assertNotEquals` instead.
The second smell is known as _Assertion Roulette_, where a test method has multiple asserts without explanatory messages, which can sometimes make it difficult to identify which assert has failed. Therefore, I added exploratory messages to the asserts in the two test methods in [SummaryReportTest.java](https://github.com/apache/accumulo/commit/a657b6ee75d7c6a0556d0059dc3bc1317594d34b#diff-d0c6de0d5f496d76d1c71a7b39cb5ddc4060d8c68ddcf9893fd1f7fd5395d626).
I would like to get your feedback on these particular test smells and their refactorings. Thanks in advance for your input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Taher-Ghaleb commented on pull request #3712: Refactor assertTrue(!=) with assertNotEquals & Add explanatory messages
Posted by "Taher-Ghaleb (via GitHub)" <gi...@apache.org>.
Taher-Ghaleb commented on PR #3712:
URL: https://github.com/apache/accumulo/pull/3712#issuecomment-1686413417
Thanks for your responses. I assumed I have already sorted the imports manually, but will also check locally. I removed the unneeded import for now.
What is your opinion about the changes made in this pull request, please? I would appreciate your feedback for my research work. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Taher-Ghaleb commented on pull request #3712: Refactor assertTrue(!=) with assertNotEquals & Add explanatory messages
Posted by "Taher-Ghaleb (via GitHub)" <gi...@apache.org>.
Taher-Ghaleb commented on PR #3712:
URL: https://github.com/apache/accumulo/pull/3712#issuecomment-1686368957
Sorry for failing the build, but I don't get why it complains about "Imports are not sorted", while they look fine. Please advise. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3712: Refactor assertTrue(!=) with assertNotEquals & Add explanatory messages
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3712:
URL: https://github.com/apache/accumulo/pull/3712#issuecomment-1687050243
> Thanks, @EdColeman and @ctubbsii, for your feedback.
>
> I have removed the assertion messages as suggested. I wanted to raise the point on having multiple assert statements in a single test method, which is known in the research literature as a test smell called _ Assertion Roulette_.
I'm not familiar with that literature, but after a quick Google search, I understand it to be not about multiple assertions themselves, but multiple assertions without sufficient description to identify which of the assertions failed for a given test. However, the general concerns about that are already addressed by using a test framework like JUnit, where the assertion failures already identify the specific line in the code where the assertion failed.
For example, the following would be an example of Assertion Roulette:
```java
public void testMethodAssertionRouletteBadSmall() {
if (condition1) {
throw new AssertionError();
}
if (condition1) {
throw new AssertionError();
}
if (condition2) {
throw new AssertionError();
}
if (condition3) {
throw new AssertionError();
}
}
```
But, the following would not be, because JUnit adds sufficient information about which specific assertion failed and for what reason:
```java
public void testMethodIsOkay() {
assertThat(condition1);
assertThat(condition2);
assertThat(condition3);
}
```
We can add more details in JUnit's API, but often, we don't need to, because it's already implied by the choice of which assertion method to use from JUnit.
So, I don't think our code is a good example of Assertion Roulette.
There are some places in our code that might be, though, where we do something like:
```java
public void testMethodAssertionRouletteBadSmall() {
manyConditions.forEach(condition -> assertThat(condition));
}
```
These are probably better off with something more like:
```java
public void testMethodAssertionRouletteIsBetter() {
manyConditions.forEach(condition -> assertThat(condition, condition.description()));
}
```
But even then, we don't do that kind of thing very often, and if something does fail in a loop like that, it's still plenty valuable enough to track down the problem, without needing to bloat the test code with more complexity to avoid the bad smell.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3712: Refactor assertTrue(!=) with assertNotEquals & Add explanatory messages
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3712:
URL: https://github.com/apache/accumulo/pull/3712#issuecomment-1686401567
You can run the following locally to run the same checks:
```
mvn clean verify package javadoc:jar -Perrorprone -DskipITs=true -DskipTests=true
```
But, I think the issue may be that this import is not needed:
```
import static org.junit.jupiter.api.Assertions.assertTrue;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #3712: Refactor assertTrue(!=) with assertNotEquals & Add explanatory messages
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #3712:
URL: https://github.com/apache/accumulo/pull/3712#issuecomment-1686377871
> Sorry for failing the build, but I don't get why it complains about "Imports are not sorted", while they look fine. Please advise. Thanks.
Running `mvn clean package -DskipTests` should sort the imports and make sure the code is properly formatted. After running that, you should be able to push the changes and have that build error go away.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #3712: Refactor assertTrue(!=) with assertNotEquals & Add explanatory messages
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #3712:
URL: https://github.com/apache/accumulo/pull/3712
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3712: Refactor assertTrue(!=) with assertNotEquals & Add explanatory messages
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3712:
URL: https://github.com/apache/accumulo/pull/3712#issuecomment-1686904222
The use of `assertNotEquals` instead of `assertTrue..!= ...!0` seems fine. I am not sure about usefulness of the explanatory messages in all cases. The premise that the message adds clarify might not always be true.
When an assert fails, it provides the line as well as the actual / expected value. So
```
assertNotNull(report, "The report should not be null");
```
Is not adding much to the failure message. The same with
```
assertNotEquals(0, report.getReportTime(), "Report time should not be 0");
```
If will fail with a value not equal to 0 and show expected / received value. The fact that now it would include that it was report time in the message? Might help, but it is pretty obvious from the code and would need to examine the code anyway, so it does not seem like it adds much.
The case of
```
assertEquals(Map.of(), report.getStatusCounts(), "Status counts should be empty");
```
is not clear cut. One, it is expecting an empty map - so it might be more accurate to state that `Status counts should be an empty map - but that is also redundant with the assertEquals, so again may not be adding clarity.
Using the message when it adds clarity seems like something to keep in mind and should be encouraged. When it is just repeating the obvious that something was null, not null or equal / not equal, well, it may marginally make the reported test failure easier to read "foo was not null"... if you only read the test output, but to understand the error in context, you still need to go to the code and then the extra text is not really adding much and is now one more thing to keep in sync / correct for something that is rather self documenting in the first place.
Just my opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Taher-Ghaleb commented on pull request #3712: Refactor assertTrue(!=) with assertNotEquals & Add explanatory messages
Posted by "Taher-Ghaleb (via GitHub)" <gi...@apache.org>.
Taher-Ghaleb commented on PR #3712:
URL: https://github.com/apache/accumulo/pull/3712#issuecomment-1687022619
Thanks, @EdColeman and @ctubbsii, for your feedback.
I have removed the assertion messages as suggested. I wanted to raise the point on having multiple assert statements in a single test method, which is known in the research literature as a test smell called _ Assertion Roulette_. One of the suggested refactoring of such practice was to add explanatory messages to the assertions. I understand my messages were not informative enough, but I just wanted to get your opinion about the refactoring approach itself, whether you usually do it in practice or if you have other better alternatives.
Thank you again for your input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org