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

StackDepthException.failedCodeStackDepth returns invalid value 0 when using Future + passing custom implicit position #2286

Open
unkarjedy opened this issue Sep 25, 2023 · 4 comments

Comments

@unkarjedy
Copy link

unkarjedy commented Sep 25, 2023

Run the test below.
It contains 4 failed tests.

ThisBuild / scalaVersion := "2.13.10"

lazy val root = (project in file("."))
  .settings(
    name := "untitled33",
    libraryDependencies ++= Seq(
      "org.scalatest" %% "scalatest" % "3.2.15"
    ),
  )

tests.scala

package org.example

import org.scalactic.source.Position
import org.scalatest.Assertion
import org.scalatest.exceptions.StackDepthException
import org.scalatest.funsuite.AsyncFunSuite

import scala.concurrent.Future

class MyTest1 extends AsyncFunSuite {
  private def go1(implicit pos: Position): Future[Assertion] = {
    Future.successful(
      try assertResult("test")("best") catch {
        case e: StackDepthException =>
          println("failedCodeStackDepth1: " + e.failedCodeStackDepth); throw e
      }
    )
  }

  private def go2: Future[Assertion] = {
    Future.successful(
      try assertResult("test")("best") catch {
        case e: StackDepthException =>
          println("failedCodeStackDepth2: " + e.failedCodeStackDepth); throw e
      }
    )
  }

  private def go3: Future[Assertion] = {
    Future {
      try assertResult("test")("best") catch {
        case e: StackDepthException =>
          println("failedCodeStackDepth3: " + e.failedCodeStackDepth); throw e
      }
    }
  }

  private def go4(implicit pos: Position): Future[Assertion] = {
    Future {
      try assertResult("test")("best") catch {
        case e: StackDepthException =>
          println("failedCodeStackDepth4: " + e.failedCodeStackDepth); throw e
      }
    }
  }

  private def go5(implicit pos: Position): Future[Assertion] = {
    Utils.foo()
  }

  test("test OK 1") {
    go1
  }
  test("test OK 2") {
    go2
  }
  test("test OK 3") {
    go3
  }
  test("test BAD 1") {
    go4
  }
  test("test BAD 2") {
    go5
  }
}

The output is as follows:

failedCodeStackDepth1: 8
failedCodeStackDepth2: 6
failedCodeStackDepth3: 6
failedCodeStackDepth4: 0
failedCodeStackDepth5: 0

Notice that in the "test BAD 1" it fails to detect failedCodeStackDepth and returns 0 instead.
Future { } introduces an asynchronous boundary, and parameter implicit pos: Position holds a line which is different from the line inside Future {}. Due to this in this place org.scalatest.exceptions.StackDepthExceptionHelper#getStackDepth it can't find a stack trace element with the same line number as in pos, because inside Future { } async boundary, the line number is different.

This issue leads to an issue in IntelliJ Scala Plugin, where we rely on the value of failedCodeStackDepth
https://youtrack.jetbrains.com/issue/SCL-21627/clicking-on-a-hyperlink-in-a-StackTrace-of-a-failed-test-navigates-to-a-wrong-file-Assertions.scala

In theory, in such case we could just pick the line with matching only the file name and not the line.
image

Test "test BAD 2" is similar, but in this case we can't do this trick because stacktrace doesn't contain the original file name with the tests

In theory, in this case, we could just pick the line with matching only the file name and not the line.

image
mutcianm pushed a commit to JetBrains/intellij-scala that referenced this issue Sep 26, 2023
…ng StackDepthException #SCL-21627 fixed

See scalatest/scalatest#2286 for the details.
It's better not to show any class name at all than to show a wrong one (`Assertions`).
Navigation will still work based on the file name.
However, we don't have a file path, we only have a short file name.
When we have multiple files with the same name, IDEA will show a chooser tooltip so the user can manually choose which file to open
mutcianm pushed a commit to JetBrains/intellij-scala that referenced this issue Sep 26, 2023
…ng StackDepthException #SCL-21627 fixed

See scalatest/scalatest#2286 for the details.
It's better not to show any class name at all than to show a wrong one (`Assertions`).
Navigation will still work based on the file name.
However, we don't have a file path, we only have a short file name.
When we have multiple files with the same name, IDEA will show a chooser tooltip so the user can manually choose which file to open
@cheeseng
Copy link
Contributor

@unkarjedy @mutcianm @bvenners hmm, unfortunately unless we do the stack trace look up at the point of the Position is created, we can't refer back to the stack trace of the passed Position where it is created. Theoretically, we can either compute the stack depth at the point of the creation of the Position, or, we save the stack trace along in the Position, but both are very expensive thing to do, I think. I wonder if intellij can use the Position directly when it is available?

@unkarjedy
Copy link
Author

I wonder if intellij can use the Position directly when it is available?

The position only contains the file name, not the file path.
The path is only present if SCALACTIC_FILL_FILE_PATHNAMES env var is set

pleaseDefineScalacticFillFilePathnameEnvVar=Please set the environment variable SCALACTIC_FILL_FILE_PATHNAMES to yes at compile time to enable this feature.

Not sure if we should silently add it during the compilation.
It might introduce extra unexpected issues.

For now I am using the solution described in JetBrains/intellij-scala@b63f204

Navigation will still work based on the file name.
However, we don't have a file path, we only have a short file name.
When we have multiple files with the same name, IDEA will show a chooser tooltip so the user can manually choose which file to open

@unkarjedy
Copy link
Author

I think we might close this then

@nafg
Copy link
Contributor

nafg commented Oct 26, 2023

Why is SCALACTIC_FILL_FILE_PATHNAMES not the default, and why can it only be set as an environment variable?

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

No branches or pull requests

3 participants