Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes the signature of intercept to return the Throwable. #38

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jupco
Copy link

@jupco jupco commented Oct 2, 2019

Returning the exception that was intercepted allows the library users to inspect the exception a little bit more, so they don't have to rely on just the expected type and can compare, as an example, the error message.

Would you prefer to have this new signature as a new function for compat issues maybe?

…o be able to inspect more the expected exception
}

test("fail()") {
def x = 1
intercept[AssertionError] { if (x == 1) fail() }
()
}
Copy link
Owner

@eed3si9n eed3si9n Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therein lies the problem. scala-verify (and its parent Minitest) strictly checks for the Unit type, and it's not fun to put () right after you actually did the checking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sucks. I think it's much better to switch the API to not use exceptions and other side effects.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is a side effect in the test framework. The logs, asserting, intercepting the exception etc.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to make this whole thing more purely functional would be to make assert(...) return Assertion but in reality, I find it more convenient to be able to to do

  test("something") {
    if (something) {
      assert("bla" == foo)
    }
    List(1, 2, 3) foreach { x = > assert("bla" == bar(x)) }
  }

and trying to fold them with and, or worse forget them.

Copy link
Contributor

@dwijnand dwijnand Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pure functional way to avoid that I think would look something like this (in Scala):

test("something")(for {
  () <- assert("bla" == foo).when(something)
  () <- List(1, 2, 3).traverse(x => assert("bla" == bar(x)))
} yield ())

which, if you prefer, you could formulate as:

test("something")(testFoo >> testBar)
def testFoo = assert("bla" == foo).when(something)
def testBar = List(1, 2, 3).traverse(x => assert("bla" == bar(x)))

and/or you could extract the inner bar assert.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ... Good luck convincing me that the purely functional test is more readable or even more maintainable.

TBH purely functional testing seems actually harmful to me. I don't even begin to understand what could possibly be the benefit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usual reasons: easier to reason, reuse, refactor and create abstractions over.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er ... Concrete example? Because from the above, and from everything I've seen about purely functional testing, it makes things harder to reason about, reuse, refactor and create abstractions over ... Let alone getting them right in the first place by not forgetting to chain one of the asserts in the middle of a loop!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling value discard warning definitely helps to avoid those issues.

@@ -20,7 +20,7 @@ trait Assertion extends asserts.AssertEquals[Unit] {
override protected def stringAssertEqualsListener = PowerAssert.stringAssertEqualsListener
lazy val assert: PowerAssert = new PowerAssert()

def intercept[E <: Throwable: ClassTag](callback: => Unit)(implicit pos: SourceLocation): Unit = {
def intercept[E <: Throwable: ClassTag](callback: => Unit)(implicit pos: SourceLocation): Throwable = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably even better to return E here, which you can do by casting with the E value.

@eed3si9n
Copy link
Owner

eed3si9n commented Oct 2, 2019

As a precedence, ScalaTest's intercept does that - http://www.scalatest.org/user_guide/using_assertions#expectedExceptions

Except, ScalaTest provides assertThrows and intercept. Given that this is a sugar that the user can easily implement using try-catch anyway, I want to be cautious on trying to stay minimal.

@ekrich
Copy link

ekrich commented Nov 22, 2019

I agree with returning the exception for inspection. When working on java stdlib in Scala Native e.g. it is helpful to inspect the error message to make sure it matches the JVM error message. Although, not part of the API, in practice having the same message is helpful to not create surprises.

Base automatically changed from master to main March 19, 2021 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants