Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22557#discussion_r221056128
  
    --- Diff: 
core/src/test/scala/org/apache/spark/security/CryptoStreamUtilsSuite.scala ---
    @@ -164,6 +167,34 @@ class CryptoStreamUtilsSuite extends SparkFunSuite {
         }
       }
     
    +  test("error handling wrapper") {
    +    val wrapped = mock(classOf[ReadableByteChannel])
    +    val decrypted = mock(classOf[ReadableByteChannel])
    +    val errorHandler = new 
CryptoStreamUtils.ErrorHandlingReadableChannel(decrypted, wrapped)
    +
    +    when(decrypted.read(any(classOf[ByteBuffer])))
    +      .thenThrow(new IOException())
    +      .thenThrow(new InternalError())
    +      .thenReturn(1)
    +
    +    val out = ByteBuffer.allocate(1)
    +    intercept[IOException] {
    +      errorHandler.read(out)
    +    }
    +    intercept[InternalError] {
    +      errorHandler.read(out)
    +    }
    +    intercept[IOException] {
    +      errorHandler.read(out)
    +    }
    --- End diff --
    
    since you're throwing a mock IOException above, this would be a little more 
clear if here you checked the message too
    
    ```scala
    assert(intercept[IOException] {
      errorHandler.read(out)
    }.getMessage() === ("Cipher stream is closed."))
    ```
    
    (you can also use `assertThrows` if you're not looking at the exception at 
all, though doesn't really matter)


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Reply via email to