dcapwell commented on code in PR #2104: URL: https://github.com/apache/cassandra/pull/2104#discussion_r1130035154
########## test/distributed/org/apache/cassandra/distributed/test/streaming/BoundExceptionTest.java: ########## @@ -0,0 +1,144 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.distributed.test.streaming; + +import java.io.IOException; +import java.util.HashSet; + +import com.google.common.base.Throwables; +import org.junit.Test; + +import org.apache.cassandra.streaming.StreamSession; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class BoundExceptionTest +{ + private static final int LIMIT = 2; + + @Test + public void testSingleException() { + try { + throw new RuntimeException("test exception"); Review Comment: why are you throwing? the stack trace is already filled ``` jshell> new NullPointerException().getStackTrace() $1 ==> StackTraceElement[11] { do_it$(java:7), java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104), java.base/java.lang.reflect.Method.invoke(Method.java:578), jdk.jshell/jdk.jshell.execution.DirectExecutionControl.invoke(DirectExecutionControl.java:209), jdk.jshell/jdk.jshell.execution.RemoteExecutionControl.invoke(RemoteExecutionControl.java:116), jdk.jshell/jdk.jshell.execution.DirectExecutionControl.invoke(DirectExecutionControl.java:119), jdk.jshell/jdk.jshell.execution.ExecutionControlForwarder.processCommand(ExecutionControlForwarder.java:144), jdk.jshell/jdk.jshell.execution.ExecutionControlForwarder.commandLoop(ExecutionControlForwarder.java:262), jdk.jshell/jdk.jshell.execution.Util.forwardExecutionControl(Util.java:77), jdk.jshell/jdk.jshell.execution.Util.forwardExecutionControlAndIO(Util.java:138), jdk.jshell/jdk.jshell.execution.RemoteExecutionControl.main(RemoteExecutionControl.java:70) } ``` so just adds more verbosity ########## test/distributed/org/apache/cassandra/distributed/test/streaming/BoundExceptionTest.java: ########## @@ -0,0 +1,144 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.distributed.test.streaming; + +import java.io.IOException; +import java.util.HashSet; + +import com.google.common.base.Throwables; +import org.junit.Test; + +import org.apache.cassandra.streaming.StreamSession; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class BoundExceptionTest +{ + private static final int LIMIT = 2; + + @Test + public void testSingleException() { + try { + throw new RuntimeException("test exception"); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: test exception\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testSingleException(BoundExceptionTest.java:38)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, LIMIT); + } + } + + @Test + public void testNestedException() { + try { + throw new RuntimeException(new IllegalArgumentException("the disk /foo/var is bad", new IOException("Bad disk somewhere"))); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: java.lang.IllegalArgumentException: the disk /foo/var is bad\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testNestedException(BoundExceptionTest.java:52)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n" + + "Caused by: java.lang.IllegalArgumentException: the disk /foo/var is bad\n" + + "\t... " + LIMIT + " more\n" + + "Caused by: java.io.IOException: Bad disk somewhere\n" + + "\t... " + LIMIT + " more\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, LIMIT); + } + } + + @Test + public void testExceptionCycle() + { + try { + throw new Exception("Test exception 1"); + } + catch (Exception e1) { + try { + throw new RuntimeException("Test exception 2"); + } + catch (Exception e2) { + e1.initCause(e2); + e2.initCause(e1); + + causeCycle(e1); Review Comment: this is confusing name, you just caused a cycle... this method does not do cause one... ########## test/distributed/org/apache/cassandra/distributed/test/streaming/BoundExceptionTest.java: ########## @@ -0,0 +1,144 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.distributed.test.streaming; + +import java.io.IOException; +import java.util.HashSet; + +import com.google.common.base.Throwables; +import org.junit.Test; + +import org.apache.cassandra.streaming.StreamSession; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class BoundExceptionTest +{ + private static final int LIMIT = 2; + + @Test + public void testSingleException() { + try { + throw new RuntimeException("test exception"); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: test exception\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testSingleException(BoundExceptionTest.java:38)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, LIMIT); + } + } + + @Test + public void testNestedException() { + try { + throw new RuntimeException(new IllegalArgumentException("the disk /foo/var is bad", new IOException("Bad disk somewhere"))); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: java.lang.IllegalArgumentException: the disk /foo/var is bad\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testNestedException(BoundExceptionTest.java:52)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n" + + "Caused by: java.lang.IllegalArgumentException: the disk /foo/var is bad\n" + + "\t... " + LIMIT + " more\n" + + "Caused by: java.io.IOException: Bad disk somewhere\n" + + "\t... " + LIMIT + " more\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, LIMIT); + } + } + + @Test + public void testExceptionCycle() + { + try { + throw new Exception("Test exception 1"); + } + catch (Exception e1) { + try { + throw new RuntimeException("Test exception 2"); + } + catch (Exception e2) { + e1.initCause(e2); + e2.initCause(e1); + + causeCycle(e1); + } + } + } + + private static void causeCycle(Exception e) { + try { + throw e; + } catch (Exception e1) { + try { + StreamSession.boundStackTrace(e1, LIMIT, new HashSet<>()); + } + catch (Exception e2) { + String expectedStackTrace = "java.lang.IllegalArgumentException: Exception cycle detected"; + assertTrue(Throwables.getStackTraceAsString(e2).contains(expectedStackTrace)); Review Comment: think you will like `org.assertj.core.api.Assertions#assertThat(java.lang.Throwable)` and `org.assertj.core.api.Assertions#assertThatThrownBy(org.assertj.core.api.ThrowableAssert.ThrowingCallable)`, makes testing exception state easier ########## test/distributed/org/apache/cassandra/distributed/test/streaming/BoundExceptionTest.java: ########## @@ -0,0 +1,144 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.distributed.test.streaming; + +import java.io.IOException; +import java.util.HashSet; + +import com.google.common.base.Throwables; +import org.junit.Test; + +import org.apache.cassandra.streaming.StreamSession; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class BoundExceptionTest +{ + private static final int LIMIT = 2; + + @Test + public void testSingleException() { + try { + throw new RuntimeException("test exception"); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: test exception\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testSingleException(BoundExceptionTest.java:38)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, LIMIT); + } + } + + @Test + public void testNestedException() { + try { + throw new RuntimeException(new IllegalArgumentException("the disk /foo/var is bad", new IOException("Bad disk somewhere"))); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: java.lang.IllegalArgumentException: the disk /foo/var is bad\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testNestedException(BoundExceptionTest.java:52)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n" + + "Caused by: java.lang.IllegalArgumentException: the disk /foo/var is bad\n" + + "\t... " + LIMIT + " more\n" + + "Caused by: java.io.IOException: Bad disk somewhere\n" + + "\t... " + LIMIT + " more\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, LIMIT); + } + } + + @Test + public void testExceptionCycle() + { + try { + throw new Exception("Test exception 1"); + } + catch (Exception e1) { + try { + throw new RuntimeException("Test exception 2"); + } + catch (Exception e2) { + e1.initCause(e2); + e2.initCause(e1); + + causeCycle(e1); + } + } + } + + private static void causeCycle(Exception e) { + try { + throw e; + } catch (Exception e1) { + try { + StreamSession.boundStackTrace(e1, LIMIT, new HashSet<>()); + } + catch (Exception e2) { + String expectedStackTrace = "java.lang.IllegalArgumentException: Exception cycle detected"; + assertTrue(Throwables.getStackTraceAsString(e2).contains(expectedStackTrace)); + } + } + } + + @Test + public void testEmptyStackTrace() { + try { + throw new NullPointerException("there are words here"); + } catch (Exception e) { + e.setStackTrace(new StackTraceElement[0]); + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.NullPointerException: there are words here\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, 0); + } + } + + @Test + public void testNullException() { Review Comment: why this test? this is already covered ########## test/distributed/org/apache/cassandra/distributed/test/streaming/BoundExceptionTest.java: ########## @@ -0,0 +1,144 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.distributed.test.streaming; + +import java.io.IOException; +import java.util.HashSet; + +import com.google.common.base.Throwables; +import org.junit.Test; + +import org.apache.cassandra.streaming.StreamSession; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class BoundExceptionTest +{ + private static final int LIMIT = 2; + + @Test + public void testSingleException() { + try { + throw new RuntimeException("test exception"); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: test exception\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testSingleException(BoundExceptionTest.java:38)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, LIMIT); + } + } + + @Test + public void testNestedException() { + try { + throw new RuntimeException(new IllegalArgumentException("the disk /foo/var is bad", new IOException("Bad disk somewhere"))); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: java.lang.IllegalArgumentException: the disk /foo/var is bad\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testNestedException(BoundExceptionTest.java:52)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n" + + "Caused by: java.lang.IllegalArgumentException: the disk /foo/var is bad\n" + + "\t... " + LIMIT + " more\n" + + "Caused by: java.io.IOException: Bad disk somewhere\n" + + "\t... " + LIMIT + " more\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, LIMIT); + } + } + + @Test + public void testExceptionCycle() + { + try { + throw new Exception("Test exception 1"); + } + catch (Exception e1) { + try { + throw new RuntimeException("Test exception 2"); Review Comment: why don't you just call `e.addSuppressed`? ########## test/distributed/org/apache/cassandra/distributed/test/streaming/BoundExceptionTest.java: ########## @@ -0,0 +1,144 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.distributed.test.streaming; + +import java.io.IOException; +import java.util.HashSet; + +import com.google.common.base.Throwables; +import org.junit.Test; + +import org.apache.cassandra.streaming.StreamSession; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class BoundExceptionTest +{ + private static final int LIMIT = 2; + + @Test + public void testSingleException() { + try { + throw new RuntimeException("test exception"); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: test exception\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testSingleException(BoundExceptionTest.java:38)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, LIMIT); + } + } + + @Test + public void testNestedException() { + try { + throw new RuntimeException(new IllegalArgumentException("the disk /foo/var is bad", new IOException("Bad disk somewhere"))); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: java.lang.IllegalArgumentException: the disk /foo/var is bad\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testNestedException(BoundExceptionTest.java:52)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n" + + "Caused by: java.lang.IllegalArgumentException: the disk /foo/var is bad\n" + + "\t... " + LIMIT + " more\n" + + "Caused by: java.io.IOException: Bad disk somewhere\n" + + "\t... " + LIMIT + " more\n"; Review Comment: are you aware what this is saying? This feels like you copy/pasted the output and added a test showing they match... `"\t... " + LIMIT + " more\n"` shouldn't be present ########## test/distributed/org/apache/cassandra/distributed/test/streaming/BoundExceptionTest.java: ########## @@ -0,0 +1,144 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.distributed.test.streaming; + +import java.io.IOException; +import java.util.HashSet; + +import com.google.common.base.Throwables; +import org.junit.Test; + +import org.apache.cassandra.streaming.StreamSession; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class BoundExceptionTest +{ + private static final int LIMIT = 2; + + @Test + public void testSingleException() { + try { + throw new RuntimeException("test exception"); + } catch (Exception e) { + StreamSession.boundStackTrace(e, LIMIT, new HashSet<>()); + String expectedStackTrace = "java.lang.RuntimeException: test exception\n" + + "\tat org.apache.cassandra.distributed.test.streaming.BoundExceptionTest.testSingleException(BoundExceptionTest.java:38)\n" + + "\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n"; + assertEquals(expectedStackTrace,Throwables.getStackTraceAsString(e)); + assertEquals(e.getStackTrace().length, LIMIT); Review Comment: this should not happen, this is showing that `boundStackTrace` mutates the throwable, which is dangerous... -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
