You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/02/14 18:30:02 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request #2494: Remove faulty test: `SortedLogRecoveryTest.testMissingDefinition`
DomGarguilo opened a new pull request #2494:
URL: https://github.com/apache/accumulo/pull/2494
Closes #2489
The changes in this PR delete `SortedLogRecoveryTest.testMissingDefinition()`.
Based off the name of the test (testMissingDefinition) it seems that this test assumes that an exception will be thrown when there is no definition present. Instead, this information is just logged and does not cause an exception to be thrown:
```
[log.SortedLogRecovery] INFO : Tablet table<< is not defined in recovery logs [testlog]
```
Additionally, this test is incorrectly set up to handle the `fail()` statement which will just be caught in the catch block instead of making the test actually fail.
--
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 merged pull request #2494: Fix faulty test: `SortedLogRecoveryTest.testMissingDefinition`
Posted by GitBox <gi...@apache.org>.
DomGarguilo merged pull request #2494:
URL: https://github.com/apache/accumulo/pull/2494
--
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 #2494: Remove faulty test: `SortedLogRecoveryTest.testMissingDefinition`
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2494:
URL: https://github.com/apache/accumulo/pull/2494#issuecomment-1039453749
In that case, I think the test should be modified instead of removed, to ensure that it doesn't throw an exception in that case, to prevent a regression... unless there's already a test that does that.
--
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 #2494: Remove faulty test: `SortedLogRecoveryTest.testMissingDefinition`
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2494:
URL: https://github.com/apache/accumulo/pull/2494#issuecomment-1039443160
--
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 #2494: Remove faulty test: `SortedLogRecoveryTest.testMissingDefinition`
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2494:
URL: https://github.com/apache/accumulo/pull/2494#issuecomment-1039420840
> this test assumes that an exception will be thrown when there is no definition present
Did this code ever throw an exception in that case in the git history? If so, when and why did it change? Should it throw an exception?
--
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 #2494: Remove faulty test: `SortedLogRecoveryTest.testMissingDefinition`
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2494:
URL: https://github.com/apache/accumulo/pull/2494#issuecomment-1039443160
> Did this code ever throw an exception in that case in the git history? If so, when and why did it change? Should it throw an exception?
Yea it looks like it was changed from throwing and exception to logging [here](https://github.com/apache/accumulo/commit/ef9c0044e5e2de5e616c21527ed5df5baed3b254) which stems from [this Jira ticket](https://issues.apache.org/jira/browse/ACCUMULO-1033). According to the jira ticket, it was purposefully changed to logging. This was probably not picked up sooner because the test will always pass regardless of whether something is thrown or not.
--
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 merged pull request #2494: Fix faulty test: `SortedLogRecoveryTest.testMissingDefinition`
Posted by GitBox <gi...@apache.org>.
DomGarguilo merged pull request #2494:
URL: https://github.com/apache/accumulo/pull/2494
--
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 #2494: Remove faulty test: `SortedLogRecoveryTest.testMissingDefinition`
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2494:
URL: https://github.com/apache/accumulo/pull/2494#issuecomment-1039457212
> In that case, I think the test should be modified instead of removed, to ensure that it doesn't throw an exception in that case, to prevent a regression... unless there's already a test that does that.
Good point. I did not consider that. I'll make sure that case is covered one way or another.
--
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 #2494: Remove faulty test: `SortedLogRecoveryTest.testMissingDefinition`
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2494:
URL: https://github.com/apache/accumulo/pull/2494#issuecomment-1039420840
--
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