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

Already on GitHub? Sign in to your account

SI-7781 REPL stack trunc shows cause #2877

Merged
merged 4 commits into from Sep 4, 2013

Conversation

Projects
None yet
4 participants
Contributor

som-snytt commented Aug 26, 2013

The handy stack trace truncation in REPL doesn't show cause like a regular trace.

This commit fixes that and also adds the usual indicator for truncation, viz, "... 33 more".

The example from the ticket produces:

    scala> rewrapperer
    java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: Point of failure
      at .rewrapper(<console>:9)
      at .rewrapperer(<console>:10)
      ... 32 elided
    Caused by: java.lang.RuntimeException: java.lang.RuntimeException: Point of failure
      at .wrapper(<console>:8)
      ... 34 more
    Caused by: java.lang.RuntimeException: Point of failure
      at .sample(<console>:7)
      ... 35 more

Suppressed exceptions on Java 7 are handled reflectively.

    java.lang.RuntimeException: My problem
      at scala.tools.nsc.util.StackTraceTest.repressed(StackTraceTest.scala:56)
      ... 27 elided
      Suppressed: java.lang.RuntimeException: Point of failure
        at scala.tools.nsc.util.StackTraceTest.sample(StackTraceTest.scala:29)
        at scala.tools.nsc.util.StackTraceTest.repressed(StackTraceTest.scala:54)
        ... 27 more

Review by @qerub

Contributor

som-snytt commented Aug 26, 2013

Rebased again in case that helps Jenkins not fail, presumably spuriously. It compiles for me, with all the fixin's.

Owner

retronym commented Aug 26, 2013

Looks to me like this requires Java7 to build.

  val suppressable = isJavaAtLeast("1.7")
  type Suppressing = { def addSuppressed(t: Throwable): Unit }

  def repressed: String = try { sample } catch {
    case e: Throwable =>
      val t = new RuntimeException("My problem")
      if (suppressable) {
        val s: Suppressing = t

It would need an asInstanceOf to compile against both 6 and 7.

Contributor

som-snytt commented Aug 26, 2013

I thought I built it both ways. OK, thanks. So much for coding at 3 a.m.

Edit: It's in my history, but maybe I got the time line wrong at that time of morning.

bigant6 all.clean ; bigant6 ; bigant6 test.run

Contributor

som-snytt commented Aug 26, 2013

Possibly I didn't notice the build error, and then tests appeared to run OK because all.clean wasn't yet cleaning the junit dir, so the old test classes were still getting run. I noticed that when I changed the name of the test class, hence the second commit.

Contributor

som-snytt commented Aug 26, 2013

Updated. Thanks @retronym . test.junit runs, but I just saw a failure under test.run. It's unsupported class version, run/t6168b. Hopefully it's just something lame in my setup. The output says it used java 6 for build and test, but I've been lied to before. Occasionally, I've lied to myself.

Edit: sorry, kitteh, I owe you an apology. I take back all the mean things I muttered under my breath.

Footnote to self: looks like running ./partest from the test dir broke with the change to make it possible to run test/partest.

Update: partest uses partest.javac_cmd (javac) to run whatever happens to run, irrespective of JAVA_HOME. Despite all vagaries, I'm sure I used to run v6 tests. Maybe something changed. Or, maybe previously the javac on the path was always v6.

Contributor

som-snytt commented Aug 26, 2013

Updated to filter out the frame count from the result to check. I'd swear I had also run an optimised build at some point, but even so, "69 elided" could change with any backend change.

Tests can declare filters for check files, I recently recalled, but for now the SessionTest must do it manually.

Contributor

som-snytt commented Aug 27, 2013

In a different context, I just realized why the build system behaves as it does. It's Schroedinger's kitteh.

Member

gkossakowski commented Aug 30, 2013

@som-snytt: what's the status of this PR?

Member

gkossakowski commented Aug 30, 2013

Also, assigned to @retronym who already looked into that.

@retronym retronym was assigned Aug 30, 2013

Contributor

som-snytt commented Aug 30, 2013

@gkossakowski Thanks. The "... elided" marker is the cat's meow. I already posted what must be the first elided stack trace on SO, but for some reason I didn't get one of those badges.

This is just further work on what @qerub started, and at one point I said to myself, I can't believe it's -- whatever year it is -- and I'm writing code to print a stack trace! This is my second stack trace fix, after the script line nums.

I haven't reviewed this myself yet, but I know that with @retronym eyes on it, it won't end up crappy and might get spun into gold. They should call him @retrostiltskin.

Contributor

som-snytt commented Sep 2, 2013

Thanks @qerub . I was lazy that night because kitteh was so slow; I wanted to delete the extra parens too but didn't dare touch anything.

This tweak makes it a bit more natural.

There's also a question about when to force code and expected. I was previously annoyed that eval is an iterator; but now I see that everything should be lazy, for the reasonable case of a test of many, possibly infinite lines of source that will terminate usefully.

Currently, DirectTest still takes code as a String.

I just realized that it is github's annoying refresh that is sending me to the top of the page. I rare UI mis-step by github.

The default ant target wasn't building partest-extras. Partest and its extras are not just a test artifact, they are a first-class product of the build!

It was an error of the maven people who saw test as something subsequent to the product. But test is more like the compiler, either precedent or at least coterminous with other artifacts required for producing the product.

Contributor

som-snytt commented Sep 2, 2013

@qerub I just noticed you had updated the "session" transcript. That's interesting, because my idea was for the session to be a straight copy/paste. Right now, partest has no --update option to fix these inline session transcripts, but maybe someday. (Normalizing both expected and actual means it just works.)

som-snytt added some commits Aug 24, 2013

@som-snytt som-snytt SI-7781 REPL stack trunc shows cause
The handy stack trace truncation in REPL doesn't
show cause like a regular trace.

This commit fixes that and also adds the usual
indicator for truncation, viz, "... 33 more".

The example from the ticket produces:

```
scala> rewrapperer
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: Point of failure
  at .rewrapper(<console>:9)
  at .rewrapperer(<console>:10)
  ... 32 elided
Caused by: java.lang.RuntimeException: java.lang.RuntimeException: Point of failure
  at .wrapper(<console>:8)
  ... 34 more
Caused by: java.lang.RuntimeException: Point of failure
  at .sample(<console>:7)
  ... 35 more
```

Suppressed exceptions on Java 7 are handled reflectively.

```
java.lang.RuntimeException: My problem
  at scala.tools.nsc.util.StackTraceTest.repressed(StackTraceTest.scala:56)
  ... 27 elided
  Suppressed: java.lang.RuntimeException: Point of failure
    at scala.tools.nsc.util.StackTraceTest.sample(StackTraceTest.scala:29)
    at scala.tools.nsc.util.StackTraceTest.repressed(StackTraceTest.scala:54)
    ... 27 more
```
2fc528e
@som-snytt som-snytt Target junit.clean to clean junit artifacts
And all.clean will also junit.clean.
c88e8be
@som-snytt som-snytt SI-7781 Improve test and add comment
The test should normalize the elided message,
not strip it. (Thanks qerub; I was frustrated
with kitteh's turnaround that night, hence
unwilling to improve once it passed.)
534ced4
@som-snytt som-snytt SI-7781 Comments to SessionTest
Also, it would be nice if code and expected results were
calculated lazily. That would allow tests with infinite
code but which terminate on various conditions.
20b7ae6
Contributor

som-snytt commented Sep 3, 2013

Removed the last commit with a build tweak for partest-extras.

Contributor

qerub commented Sep 3, 2013

@qerub I just noticed you had updated the "session" transcript. That's interesting, because my idea was for the session to be a straight copy/paste.

Uh… That wasn't part of my original plan. Updated patch: qerub/scala@02a2349

This is the tweak to the @qerub tweak. The maps look more natural.

qerub replied Sep 3, 2013

Yeah, it's certainly an improvement! (Sorry, I didn't see this code until now.)

Member

gkossakowski commented Sep 4, 2013

@retronym: could you have a look and determine if we can merge this by tomorrow's end of the day (SF time)?

Owner

retronym commented Sep 4, 2013

LGTM. It is tricky stuff, but well tested.

@gkossakowski gkossakowski added a commit that referenced this pull request Sep 4, 2013

@gkossakowski gkossakowski Merge pull request #2877 from som-snytt/issue/repl-stack-trunc
SI-7781 REPL stack trunc shows cause
5044f1c

@gkossakowski gkossakowski merged commit 5044f1c into scala:master Sep 4, 2013

1 check passed

default pr-scala Took 94 min.
Details
Contributor

som-snytt commented Sep 4, 2013

I think @retronym meant, "It is tricky stuff, but, well, tested." Commas are significant.

The semantics of this code don't pop out as they ought. Unfortunately, you can't test the local defs. I'm sure that has been said before, but really, if you agitate for local defs, they must be testable like anything else. Note to self.

@som-snytt som-snytt deleted the som-snytt:issue/repl-stack-trunc branch Mar 3, 2014

@scabug scabug referenced this pull request in scala/bug Apr 7, 2017

Closed

Restore causes to REPL stack traces #7781

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment