warnUnused is reporting on Assertions

[bcarter97]

Hello, since 0.4.2 I am seeing Scalatest tests reporting unused on assertions that aren't the final value in a test, e.g.

  // ... rest of test
  config.topicLoader.idleTimeout shouldBe 1.second // This errors
  config.topicLoader.bufferSize.value shouldBe 10 // This errors too
  config.topicLoader.clientId.value shouldBe "test-client-id" // This is fine
}

/src/test/scala/unit/ConfigSpec.scala:113:38: unused value of type org.scalatest.Assertion (add `: Unit` to discard silently)
[error]       config.topicLoader.idleTimeout shouldBe 1.second 

I have tried disabling scalaFix in case it was conflicting but no change.

Is this intended behaviour?

[DavidGregory084]

Hmm thanks for the report - I think this is due to the addition of -Wnonunit-statement in #115 (which I seem to have missed in the release notes, sorry about that). I think this is because Scalatest assertions return a value of type Assertion as well as throwing an exception on failure, which unfortunately triggers this warning because the value is not usually used.
I think a lot of users were eager to see this setting added so I'm not so sure what to do here!

[armanbilge]

I think -Wnonunit-statement really only makes sense for pure FP code. Given that this is sbt-tpolecat, that's a reasonable default? 😅

[henricook]

Do people write FP tests/test assertions? 😅 I'm probably just going to find a way to override/disable this flag if I can rather than approach the new set of 500+ errors in the Test / portion of my project

[henricook]

For anyone else in the same boat who wants to disable the -Wnonunit-statement flag that sbt-tpolecat adds:

scalacOptions ~= { options: Seq[String] =>
  options.filterNot(
    Set(
      "-Wnonunit-statement"
    )
  )
}

The Set is overkill but this is a scaffold I use to remove 1 or more flags. Sometimes I remove a flag based on an environment variable, like turning off -Wunused:imports while I'm prototyping with a PROTOTYPE=true env var.

You could maybe try and only do this for your Test profiles to see if your main code body can benefit from it. In my case we have some places where we had to use Java libraries without Scala wrappers that mean this flag causes more hassle than its worth.

[wim82]

Reporting the same here. We have multiple test cases with multiple assertions in it, so the change in #115 is hurting us quite bad :) Since we operate with a lot of smaller projects, upgrading the plugin and duplicating the workaround @henricook provided is not ideal.

Not sure what would be the desired behaviour from sbt-tpolecat point of view...

edit: the suggestion below by armanbilge is not really where we want to go - as we use both FP and OOP style :)

[armanbilge]

Do people write FP tests/test assertions?

It's not that crazy, something like this:

for {
  foo <- doFoo()
  _ <- IO(assert(...))
  _ <- IO(assert(...))
} yield ()

[armanbilge]

Btw, @som-snytt gave some advice in the Scala Discord, about how to silence this warning for specific classes.

that seems to be cat=other-pure-statement, where site= the enclosing definition. You would have match the name of a class that goes unused in the msg=MyClass.
As I just did, scalac -Wconf:help
-Wconf:cat=other&msg=MyClass&site=MyClient:s where s for silent. IIRC.

[som-snytt]

I will make improvements for 2.13.11, so let me know if there is a useful heuristic here.

It is certainly reasonable to make checks in Test less strict, but also "tests are code," etc.

[channingwalton]

Do people write FP tests/test assertions?

Yes, with libs like munit-cats-effect.

[DavidGregory084]

Do people write FP tests/test assertions?

It's not that crazy, something like this:

for {
  foo <- doFoo()
  _ <- IO(assert(...))
  _ <- IO(assert(...))
} yield ()

With ScalaTest async testing + CE they often look like this:

val assertion = for {
  _ <- doSomething
  result <- doSomethingElse
} yield result shouldBe someKnownValue

assertion.unsafeToFuture()

see e.g. https://github.com/hmrc/transit-movements-guarantee-balance/blob/main/test/services/BalanceRequestCacheServiceSpec.scala#L140

[DavidGregory084]

What @henricook suggests is a totally fine way to go about disabling bits of sbt-tpolecat, although you can also use the settings DSL to do this now if you just want a one-liner, e.g.

Test / tpolecatExcludeOptions += ScalacOptions.warnNonUnitStatement

[everson]

FYI, this is also triggering for Spec2/Scala 2.13 unit tests such as:

package com.foo.app

import org.specs2.mutable.Specification

class UDFsSpec extends Specification {

  import UDFs._

  "concatDiscardingEmpty" >> {
    "ignore empty Strings and nulls" >> {
      val expected = "A:B:C"
      val result   = concatDiscardingEmpty(":", List("A", null, "B", "", "C "))
      result ==== expected
    }
  }
}

I get:

[error] ...UDFsSpec.scala:8:27: unused value of type org.specs2.specification.core.Fragment (add `: Unit` to discard silently)
[error]   "concatDiscardingEmpty" >> {
[error]                           ^
[error] two errors found

[som-snytt]

I took a look, and foo must be(bar) in ScalaTest results in an Assertion object, so there's nothing to be done on the diagnostic side.

If you want to use ScalaTest this way, write your "block of asserts" as a tuple of expressions:

  "Domain" should "have a name" in (
    domain.status must be(true),
    domain.name must be("testing"),
  )

or use -Wconf:msg=unused value of type org.scalatest.Assertion:s or turn off -Wnonunit-statement in Test. (or ask sbt-tpolecat to do that for you)

[DavidGregory084]

I've added a (perhaps slightly too prominent) warning about this to the README now - so let's close this :D

[alexklibisz]

I also had this issue and the warning in the Readme was useful, thanks for that.

However, I was still having an extra issue with IntelliJ. I guess the IntelliJ SBT plugin does not parse the Test / scalacOptions separate from the standard scalacOptions, so I had to suppress warnings for the test files, like this:

scalacOptions ++= Seq(
  "-Wconf:cat=other-pure-statement:src=test/.*.scala:silent"
)

More about warning suppression here: https://www.scala-lang.org/2021/01/12/configuring-and-suppressing-warnings.html

No need to comment or fix anything else. Just jotting this down in case anyone else runs into it.

[caoilte]

scalacOptions ++= Seq(
  "-Wconf:cat=other-pure-statement:src=test/.*.scala:silent"
)

This is a great fix for Intellij/Scala2 but appears be unworkable in Scala3 currently. Possibly we'll get more information in this issue: scala/scala3#18804

[som-snytt]

I commented on the issue. I didn't try anything out, but if it's fixed on 3.4.1, you can lobby for backport to LTS, which stands for Lobbied something.