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/07 21:57:29 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request #2472: Convert try-catch to assertThrows
DomGarguilo opened a new pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472
There are many instances in our tests where we expect an exception to be thrown and use a try-catch block to ensure that it is. An example (where `IllegalArgumentException` will be thrown upon calling `new Range(tr)`) looks like this:
```java
try {
new Range(tr);
fail("Thrift constructor allowed invalid range");
} catch (IllegalArgumentException exc) {
/* good! */
}
```
This way if the expected exception is thrown, the catch block catches it and does nothing, otherwise the test will fail via the `fail()` statement. While this works, it can be more effectively handled using `assertThrows()`. Preserving functionality, the example above would be converted to:
```java
assertThrows("Thrift constructor allowed invalid range", IllegalArgumentException.class,
() -> new Range(tr));
```
These instances were found using the ErrorProne profile from #2447 with `-Xep:TryFailRefactoring` added as an arg in the ErrorProne plugin section in the pom.
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1036401176
> The ErrorProne plugin also pointed out a test that looks like it is not behaving as intended.
>
> https://github.com/apache/accumulo/blob/3ea979d5971016b9c2512a96cc5b6bd5d1969b2e/server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java#L348-L359
>
> It looks like the test is expecting the the call to `recover()` to fail and if it doesn't, the test will fail via the call to `fail()`. But in this case, the `fail()` statement will be caught as a `Throwable` and **not** cause the test to fail when reached, Functionally, the whole try catch block does nothing more than call `recover()`. I'm not certain im interpreting this correctly but if I am, I'm not sure how to handle this case.
Im still not too sure what to do in this case. I don't think I made it clear originally that the call to `recover()` does not throw an exception so the test hits the `fail()` every time (which is caught and ignored). This does not seem like it is the intended behavior of the test - it seems like the test expects the call to recover to throw an exception which does not happen.
--
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 a change in pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#discussion_r803777110
##########
File path: core/src/test/java/org/apache/accumulo/core/iterators/system/DeletingIteratorTest.java
##########
@@ -244,13 +245,12 @@ public void testFail() throws IOException {
SortedKeyValueIterator<Key,Value> it =
DeletingIterator.wrap(new SortedMapIterator(tm), false, Behavior.FAIL);
it.seek(new Range(), EMPTY_COL_FAMS, false);
- try {
+ assertThrows(IllegalStateException.class, () -> {
while (it.hasTop()) {
it.getTopKey();
it.next();
}
- fail();
- } catch (IllegalStateException e) {}
+ });
Review comment:
Addressed in 2c61494
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472
--
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 a change in pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#discussion_r803776584
##########
File path: core/src/test/java/org/apache/accumulo/core/clientImpl/TabletLocatorImplTest.java
##########
@@ -1281,12 +1281,8 @@ public void testAccumulo1248() {
setLocation(tservers, "tserver2", MTE, ke1, "L1", "I1");
setLocation(tservers, "tserver2", MTE, ke1, "L2", "I2");
- try {
- metaCache.locateTablet(context, new Text("a"), false, false);
- fail();
- } catch (Exception e) {
-
- }
+ assertThrows(Exception.class,
+ () -> metaCache.locateTablet(context, new Text("a"), false, false));
Review comment:
Addressed in 2c61494
--
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 edited a comment on pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo edited a comment on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1032094797
The call to recover() doesn’t throw any error. I forgot to mention that.
On Mon, Feb 7, 2022 at 7:41 PM Christopher Tubbs ***@***.***>
wrote:
> It looks like the test is expecting the the call to recover() to fail and
> if it doesn't, the test will fail via the call to fail(). But in this
> case, the fail() statement will be caught as a Throwable and *not* cause
> the test to fail when reached, Functionally, the whole try catch block does
> nothing more than call recover(). I'm not certain im interpreting this
> correctly but if I am, I'm not sure how to handle this case.
>
> Yeah, ErrorProne is correct. fail(), like all the JUnit assertions,
> triggers an AssertionError that is caught and reported. We shouldn't be
> catching Throwable here, because it will also catch and ignore that
> AssertionError. We should only catch Exception, at most. We need to
> figure out which exception is expected, and use assertThrows with that
> directly like you've done with the others.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/accumulo/pull/2472#issuecomment-1032093257>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ALMD2IIZ3PUPUEIAIMYRTPTU2BREZANCNFSM5NYV4DBA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1032093257
> It looks like the test is expecting the the call to `recover()` to fail and if it doesn't, the test will fail via the call to `fail()`. But in this case, the `fail()` statement will be caught as a `Throwable` and **not** cause the test to fail when reached, Functionally, the whole try catch block does nothing more than call `recover()`. I'm not certain im interpreting this correctly but if I am, I'm not sure how to handle this case.
Yeah, ErrorProne is correct. `fail()`, like all the JUnit assertions, triggers an `AssertionError` that is caught and reported. We shouldn't be catching `Throwable` here, because it will also catch and ignore that `AssertionError`. We should only catch `Exception`, at most. We need to figure out which exception is expected, and use `assertThrows` with that directly like you've done with the others.
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1035044296
The changes in 2c61494:
* Make the methods inside of the `assertThrows()` as narrow as possible
* Introduce variables for reuse
* Make expected exceptions more specific
* Extract common code in some tests by checking `assertThrows()` in a loop
* Remove portion of VisibilityEvaluatorTest that is already covered in ColumnVisibilityTest
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1036487379
> That's clearly a bug. You could create a new ticket for that. It definitely seems like the expectation is that the method call should fail, and it's not failing. It needs to be tracked down whether it should fail, and why it isn't, or if it shouldn't fail and this test should be removed because it has a faulty premise.
I created #2489 to track this issue.
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1039477361
I ran all the unit tests, and all the ITs that were changed, and everything works fine. I did see FateIT fail, but that is known flaky (#2474)
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1031987596
The ErrorProne plugin also pointed out a test that looks like it is not behaving as intended.
https://github.com/apache/accumulo/blob/3ea979d5971016b9c2512a96cc5b6bd5d1969b2e/server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java#L348-L359
It looks like the test is expecting the the call to `recover()` to fail and if it doesn't, the test will fail via the call to `fail()`. But in this case, the `fail()` statement will be caught as a `Throwable` and **not** cause the test to fail when reached, Functionally, the whole try catch block does nothing more than call `recover()`. I'm not certain im interpreting this correctly but if I am, I'm not sure how to handle this case.
--
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 edited a comment on pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo edited a comment on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1031973578
For those interested, this command will run all the tests touched by these changes:
`mvn clean verify -Dit.test=MultiTableBatchWriterIT,UsersIT,KerberosIT,AccumuloOutputFormatIT,AccumuloInputFormatIT,DeleteFailIT,ScannerContextIT,SampleIT,AccumuloClientIT,TimeoutIT,ThriftBehaviorIT,ConditionalWriterIT,InterruptibleScannersIT,NativeMapIT,CloneIT,ExistingMacIT,CleanUpIT,LargeSplitRowIT,ShellServerIT,LocatorIT,ConcurrentDeleteTableIT,BulkNewIT,CredentialsIT -Dtest=TransactionWatcherTest,SourceSwitchingIteratorTest,TransformingIteratorTest,RootTabletStateStoreTest,CertUtilsTest,CombinerTest,RFileTest,VisibilityEvaluatorTest,CompactionDriverTest,DeletingIteratorTest,SingletonManagerTest,FormatterConfigTest,AbstractLexicoderTest,AccumuloInputFormatTest,InMemoryMapTest,AddressUtilTest,ColumnVisibilityTest,TTimeoutTransportTest,ColumnSliceFilterTest,BasicCompactionStrategyTest,LocalityGroupUtilTest,RowIteratorTest,ScannerOptionsTest,RangeTest,TableOperationsHelperTest,PrepBulkImportTest,ServerDirsTest,Upgrader9to10Test,TabletFileTest,AccumuloOutputFormatTest,FilterTest,T
abletLocatorImplTest,SortedLogRecoveryTest,CredentialsTest -Dspotbugs.skip -Dtimeout.factor=1
`
EDIT: updated to include all files
--
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] Manno15 commented on a change in pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#discussion_r805073412
##########
File path: test/src/main/java/org/apache/accumulo/test/SampleIT.java
##########
@@ -64,11 +64,15 @@
import org.apache.accumulo.core.security.Authorizations;
import org.apache.accumulo.harness.AccumuloClusterHarness;
import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import com.google.common.collect.Iterables;
public class SampleIT extends AccumuloClusterHarness {
+ Logger log = LoggerFactory.getLogger(AccumuloClusterHarness.class);
+
Review comment:
```suggestion
```
--
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] Manno15 commented on a change in pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#discussion_r805073346
##########
File path: test/src/main/java/org/apache/accumulo/test/SampleIT.java
##########
@@ -64,11 +64,15 @@
import org.apache.accumulo.core.security.Authorizations;
import org.apache.accumulo.harness.AccumuloClusterHarness;
import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
Review comment:
```suggestion
```
--
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 a change in pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#discussion_r805876456
##########
File path: test/src/main/java/org/apache/accumulo/test/SampleIT.java
##########
@@ -64,11 +64,15 @@
import org.apache.accumulo.core.security.Authorizations;
import org.apache.accumulo.harness.AccumuloClusterHarness;
import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import com.google.common.collect.Iterables;
public class SampleIT extends AccumuloClusterHarness {
+ Logger log = LoggerFactory.getLogger(AccumuloClusterHarness.class);
+
Review comment:
nice catch!
--
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 edited a comment on pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo edited a comment on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1036401176
> The ErrorProne plugin also pointed out a test that looks like it is not behaving as intended.
>
> https://github.com/apache/accumulo/blob/3ea979d5971016b9c2512a96cc5b6bd5d1969b2e/server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java#L348-L359
>
> It looks like the test is expecting the the call to `recover()` to fail and if it doesn't, the test will fail via the call to `fail()`. But in this case, the `fail()` statement will be caught as a `Throwable` and **not** cause the test to fail when reached, Functionally, the whole try catch block does nothing more than call `recover()`. I'm not certain im interpreting this correctly but if I am, I'm not sure how to handle this case.
Im still not too sure what to do in this case. I don't think I made it clear originally that the call to recover() does not throw an exception so the test hits the fail() every time (which is caught and ignored). This does not seem like it is the intended behavior of the test - it seems like the test expects the call to recover to throw an exception which does not happen.
--
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 a change in pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#discussion_r801178788
##########
File path: core/src/test/java/org/apache/accumulo/core/clientImpl/TableOperationsHelperTest.java
##########
@@ -327,23 +327,15 @@ public void testAttachIterator() throws Exception {
"table.iterator.minc.otherName.opt.key=value",
"table.iterator.scan.otherName=20,some.classname",});
- try {
- t.attachIterator("table", setting);
- fail();
- } catch (AccumuloException e) {
- // expected, ignore
- }
+ IteratorSetting finalSetting = setting; // effectively final var needed for lambda below
Review comment:
Could probably remove the explanation by just being explicit about the final, when you encounter these. For example:
```java
final var finalSetting = setting;
```
However, a better idea is to use new variables. I looked at the lines above, and it looks like there's no good reason they need to reuse the variable name. It just as easily could have had IteratorSetting setting1, setting2, etc., or more specific names if there are any that make sense.
##########
File path: core/src/test/java/org/apache/accumulo/core/clientImpl/TabletLocatorImplTest.java
##########
@@ -1281,12 +1281,8 @@ public void testAccumulo1248() {
setLocation(tservers, "tserver2", MTE, ke1, "L1", "I1");
setLocation(tservers, "tserver2", MTE, ke1, "L2", "I2");
- try {
- metaCache.locateTablet(context, new Text("a"), false, false);
- fail();
- } catch (Exception e) {
-
- }
+ assertThrows(Exception.class,
+ () -> metaCache.locateTablet(context, new Text("a"), false, false));
Review comment:
It'd be great if this code was more specific about which exception it was expecting.
##########
File path: core/src/test/java/org/apache/accumulo/core/iterators/system/SourceSwitchingIteratorTest.java
##########
@@ -286,10 +286,8 @@ public void testSetInterrupt() throws Exception {
flag.set(true);
- try {
- ssi.seek(new Range("r1"), new ArrayList<>(), false);
- fail("expected to see IterationInterruptedException");
- } catch (IterationInterruptedException iie) {}
+ assertThrows(IterationInterruptedException.class,
+ () -> ssi.seek(new Range("r1"), new ArrayList<>(), false));
Review comment:
Presumably, these are failing on the `seek` method, and not on `new Range`. But the code isn't clear. It could be written like:
```suggestion
var range = new Range("r1");
var newList = new ArrayList<?>();
assertThrows(IterationInterruptedException.class, () -> ssi.seek(range, newList, false));
```
In general, it's best to assert the throws on the narrow-est method possible. The old code is sub-par for this reason as well, so it's not a problem you introduced. But, it would be a good opportunity to clean these up.
##########
File path: core/src/test/java/org/apache/accumulo/core/iterators/system/DeletingIteratorTest.java
##########
@@ -244,13 +245,12 @@ public void testFail() throws IOException {
SortedKeyValueIterator<Key,Value> it =
DeletingIterator.wrap(new SortedMapIterator(tm), false, Behavior.FAIL);
it.seek(new Range(), EMPTY_COL_FAMS, false);
- try {
+ assertThrows(IllegalStateException.class, () -> {
while (it.hasTop()) {
it.getTopKey();
it.next();
}
- fail();
- } catch (IllegalStateException e) {}
+ });
Review comment:
This could probably be improved if it were to only assertThrows on the specific method that is being called. It's not clear here if it is expected to be throw on `hasTop`, `getTopKey`, or `next`. It's also not clear if it's supposed to be thrown on the first pass through the loop, or a subsequent one.
--
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 a change in pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#discussion_r803777655
##########
File path: core/src/test/java/org/apache/accumulo/core/iterators/system/SourceSwitchingIteratorTest.java
##########
@@ -286,10 +286,8 @@ public void testSetInterrupt() throws Exception {
flag.set(true);
- try {
- ssi.seek(new Range("r1"), new ArrayList<>(), false);
- fail("expected to see IterationInterruptedException");
- } catch (IterationInterruptedException iie) {}
+ assertThrows(IterationInterruptedException.class,
+ () -> ssi.seek(new Range("r1"), new ArrayList<>(), false));
Review comment:
Addressed in 2c61494
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1036394383
I looked through all the instances of `fail()` manually and I think I changed all that I plan to for this PR. This should be ready for review now.
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1036444359
> Im still not too sure what to do in this case. I don't think I made it clear originally that the call to `recover()` does not throw an exception so the test hits the `fail()` every time (which is caught and ignored). This does not seem like it is the intended behavior of the test - it seems like the test expects the call to recover to throw an exception which does not happen.
That's clearly a bug. You could create a new ticket for that. It definitely seems like the expectation is that the method call should fail, and it's not failing. It needs to be tracked down whether it should fail, and why it isn't, or if it shouldn't fail and this test should be removed because it has a faulty premise.
--
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 a change in pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#discussion_r803776257
##########
File path: core/src/test/java/org/apache/accumulo/core/clientImpl/TableOperationsHelperTest.java
##########
@@ -327,23 +327,15 @@ public void testAttachIterator() throws Exception {
"table.iterator.minc.otherName.opt.key=value",
"table.iterator.scan.otherName=20,some.classname",});
- try {
- t.attachIterator("table", setting);
- fail();
- } catch (AccumuloException e) {
- // expected, ignore
- }
+ IteratorSetting finalSetting = setting; // effectively final var needed for lambda below
Review comment:
Addressed in 2c61494
--
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 a change in pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#discussion_r804030905
##########
File path: core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java
##########
@@ -59,25 +60,6 @@ public void testVisibilityEvaluator() throws VisibilityParseException {
for (String marking : new String[] {"five", "one&five", "five&one", "((one|foo)|bar)&goober"}) {
assertFalse(marking, ct.evaluate(new ColumnVisibility(marking)));
}
-
- // test missing separators; these should throw an exception
- for (String marking : new String[] {"one(five)", "(five)one", "(one)(two)", "a|(b(c))"}) {
- assertThrows(marking + " failed to throw", BadArgumentException.class,
- () -> ct.evaluate(new ColumnVisibility(marking)));
- }
-
- // test unexpected separator
- for (String marking : new String[] {"&(five)", "|(five)", "(five)&", "five|", "a|(b)&",
- "(&five)", "(five|)"}) {
- assertThrows(marking + " failed to throw", BadArgumentException.class,
- () -> ct.evaluate(new ColumnVisibility(marking)));
- }
-
- // test mismatched parentheses
- for (String marking : new String[] {"(", ")", "(a&b", "b|a)"}) {
- assertThrows(marking + " failed to throw", BadArgumentException.class,
- () -> ct.evaluate(new ColumnVisibility(marking)));
- }
Review comment:
I think these were accidentally deleted
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1035235209
I'm finding more instances that could be converted so I'd wait to review if anyone was planning on it soon.
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1039477361
I ran all the unit tests, and all the ITs that were changed, and everything works fine. I did see FateIT fail, but that is known flaky (#2474)
--
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 edited a comment on pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo edited a comment on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1036401176
> That's clearly a bug. You could create a new ticket for that. It definitely seems like the expectation is that the method call should fail, and it's not failing. It needs to be tracked down whether it should fail, and why it isn't, or if it shouldn't fail and this test should be removed because it has a faulty premise.
I created #2489 to track this issue.
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1031973578
For those interested, this command will run all the tests touched by these changes:
`mvn clean verify -Dit.test=MultiTableBatchWriterIT,UsersIT,KerberosIT,AccumuloOutputFormatIT,AccumuloInputFormatIT,DeleteFailIT,ScannerContextIT,SampleIT,ThriftBehaviorIT,ConditionalWriterIT,NativeMapIT,CloneIT,ExistingMacIT,CleanUpIT,LargeSplitRowIT,ShellServerIT,LocatorIT,ConcurrentDeleteTableIT,BulkNewIT,CredentialsIT -Dtest=TransactionWatcherTest,SourceSwitchingIteratorTest,TransformingIteratorTest,RootTabletStateStoreTest,CertUtilsTest,CombinerTest,RFileTest,VisibilityEvaluatorTest,DeletingIteratorTest,SingletonManagerTest,FormatterConfigTest,AccumuloInputFormatTest,InMemoryMapTest,AddressUtilTest,TTimeoutTransportTest,ColumnSliceFilterTest,LocalityGroupUtilTest,RowIteratorTest,ScannerOptionsTest,RangeTest,TableOperationsHelperTest,Upgrader9to10Test,TabletFileTest,AccumuloOutputFormatTest,FilterTest,TabletLocatorImplTest,SortedLogRecoveryTest,CredentialsTest -Dspotbugs.skip -Dtimeout.factor=1
`
--
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 a change in pull request #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#discussion_r804042251
##########
File path: core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java
##########
@@ -59,25 +60,6 @@ public void testVisibilityEvaluator() throws VisibilityParseException {
for (String marking : new String[] {"five", "one&five", "five&one", "((one|foo)|bar)&goober"}) {
assertFalse(marking, ct.evaluate(new ColumnVisibility(marking)));
}
-
- // test missing separators; these should throw an exception
- for (String marking : new String[] {"one(five)", "(five)one", "(one)(two)", "a|(b(c))"}) {
- assertThrows(marking + " failed to throw", BadArgumentException.class,
- () -> ct.evaluate(new ColumnVisibility(marking)));
- }
-
- // test unexpected separator
- for (String marking : new String[] {"&(five)", "|(five)", "(five)&", "five|", "a|(b)&",
- "(&five)", "(five|)"}) {
- assertThrows(marking + " failed to throw", BadArgumentException.class,
- () -> ct.evaluate(new ColumnVisibility(marking)));
- }
-
- // test mismatched parentheses
- for (String marking : new String[] {"(", ")", "(a&b", "b|a)"}) {
- assertThrows(marking + " failed to throw", BadArgumentException.class,
- () -> ct.evaluate(new ColumnVisibility(marking)));
- }
Review comment:
I intentionally removed them because they are not actually testing `VisibilityEvaluator` functionality. I left a note [here](https://github.com/apache/accumulo/pull/2472#issuecomment-1035044296), but these tests were failing on the creation of `new ColumnVisibility(marking)` which is already tested in ColumnVisibilityTest. Here are the three spots in ColumnVisibilityTest that cover these cases.
https://github.com/apache/accumulo/blob/1dc72fce2c781dee597c8c11876a3bc6c321c199/core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java#L125-L136
--
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 #2472: Convert try-catch to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2472:
URL: https://github.com/apache/accumulo/pull/2472#issuecomment-1032094797
The call to recover() doesn’t throw any error. I forgot to mention that.
On Mon, Feb 7, 2022 at 7:41 PM Christopher Tubbs ***@***.***>
wrote:
> It looks like the test is expecting the the call to recover() to fail and
> if it doesn't, the test will fail via the call to fail(). But in this
> case, the fail() statement will be caught as a Throwable and *not* cause
> the test to fail when reached, Functionally, the whole try catch block does
> nothing more than call recover(). I'm not certain im interpreting this
> correctly but if I am, I'm not sure how to handle this case.
>
> Yeah, ErrorProne is correct. fail(), like all the JUnit assertions,
> triggers an AssertionError that is caught and reported. We shouldn't be
> catching Throwable here, because it will also catch and ignore that
> AssertionError. We should only catch Exception, at most. We need to
> figure out which exception is expected, and use assertThrows with that
> directly like you've done with the others.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/accumulo/pull/2472#issuecomment-1032093257>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ALMD2IIZ3PUPUEIAIMYRTPTU2BREZANCNFSM5NYV4DBA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
Dominic Garguilo
--
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