report absolute file and line number of failing test #630
Comments
|
Hi Sam, Are you familiar with the Location API in ScalaTest's event model? Its purpose is to allow IDEs to hop to the relevant line of code for events for which that is useful. It is an ADT defined here: http://doc.scalatest.org/2.2.4/index.html#org.scalatest.events.Location What is missing from what you're asking for is the fully qualified file name. We just provide the simple filename, because that's all that is available in the stack trace. The TestFailedException (and other StackDepth exceptions) also include a stack depth that points right to the offending stack frame where the assertion took place. Since the port to Scala.js, though, we have been feeling the need to use a macro to extract the filename and line number. Macros weren't available when we started ScalaTest, so the way we've implemented this is by inspecting stack traces. On Scala.js, the stack trace on both Node and Phantom are pretty useless, because they are for the optimized single source file. On Rhino we can get a good stack depth, but it is different from that of the JVM, and was a huge pain when we were porting ScalaTest to Scala.js. I also fear it isn't stable, and am just hoping the Scala.js powers that be won't break all our stack depths in future versions of Scala.js. If we can switch to a macro approach that would mean good info could be had even on Node and Phantom, and it would be less of a problem if they break our stack depths. We'll still need them I think, and we'll still have to fix them, but at least the actual filename and line number will be grabbed from the macro. What I think the other IDEs do is just rely on the fact that simple filenames clash rarely, and if they do, they deal somehow. I don't know off the top of my head how they deal with multiple files with the same simple filename, but it is an easy test to try. Anyway, I would imagine Ensime can do the same technique with names clashes, then you can just integrate with the exiting Location API (and use stack depth). Ther eis one other API I wanted to integrate with Ensime, which is the Finders API. That is what allows users of IntelliJ and Eclipse to right click in test source code and get a pop up menu that lets them run just what was clicked on. |
|
I wasn't aware of it, but that is certainly something we could look into in ENSIME! Wow, very impressive. We can actually resolve the source file when given an FQN and the filename as in a However, the ideal solution is to have something that works even in plain 'ol |
|
(incase you aren't aware of the emacs conventions, |
|
This is a really useful idea. I've got double-click in my terminal configured to recognize |
|
is this something that would need to be implemented in |
|
I'd be interested in help with this if it is simple to implement. Where is the place where output is generated? |
|
That would be great. Our Framework implementation is here: The standard out reporter is here: I think this should be under a Runner flag, not the default, probably. Bill |
|
wow, scalatest is much bigger than I ever thought it was! |
|
Just braindumping, jump in if you have any thoughts: I need to find where the |
|
That's in StringReporter, which is a superclass of StandardOutReporter: StandardOutReporter extends PrintReporter, which extends StringReporter, which has the goods. By the way I'd probably recommend submitting PRs against 3.0.x. I have a 2.2.6 staged, which I think will go out Monday. Most likely what will be the last 2.x release. Mostly just some documentation needed for the 3.0.0-RC1 release, most of which I'm hoping to finish this weekend. Thanks. Bill |
|
@xeno-by what terminal is that? |
|
I think this is the relevant part ( case TestFailed(ordinal, message, suiteName, suiteId, suiteClassName, testName, testText, recordedEvents, throwable, duration, formatter, location, rerunnable, payload, threadName, timeStamp) =>
val tff: Vector[Fragment] = fragmentsOnError(Resources.failedNote, Resources.testFailed _, message, throwable, formatter, Some(suiteName), Some(testName), duration,
presentUnformatted, presentAllDurations, presentShortStackTraces, presentFullStackTraces, AnsiRed)
val ref = recordedEventFragments(recordedEvents, AnsiRed, presentUnformatted, presentAllDurations, presentShortStackTraces, presentFullStackTraces)
tff ++ ref
but I'm not entirely sure what a What would be the best way to write a test for this? Any examples in the current tests? |
|
sorry, the attached PR is about the best I can do because my computer is not powerful enough to compile/test scalatest. I had thought putting the functionality in |
|
it would be good to have something similar for exceptions, but I'm guessing you don't have the |
|
Oh, sorry I never replied to your question yesterday about writing tests. We do have tests for StringReporter, and in fact that's what Fragment was introduced to make easier. It allowed us to test something that was returned (a vector of fragments) rather than try to capture text printed to standard output or a file. Examples of such tests is down in scalatest-test/src/test/scala/org/scalatest/tools/StringReporter* What do you mean something similar for exceptions? Do you mean some extra text printed for emacs? |
|
ok, it turns out my machine is not powerful to compile / run all the tests anyway, but I was thinking that it would be good to get similar String output when exceptions are thrown as well as when tests fail. |
|
@fommil Just a quick note to let you know that after much hard slogging (mostly by Chee Seng) we are about done with a refactor in which we grab the source code location via a macro instead of relying on a calculated or hard-coded stack depth. This was yet another major overhaul essentially forced upon us by Scala.js. But good news for this issue is that we now have the absolute path available as well as the simple filename and line number. We didn't add it to the exceptions yet, but I'm going to try and do that before RC1 which I want out before ScalaDays NYC. I'll ping you here once it is available for you to try it. Once it is all done you'll even get nice pointers to offending code even when running Scala.js on Node and Phantom. |
|
oh that is so fantastic, thank you! I'll add support to I've been forced to use junit recently. It's really hit home how much of a difference scalatest has made. |
|
I tried out 3.0.0-RC1 and I can see the filename and line number, great! But:
the output I see where the is the information I need, but I have no way of picking that out of the noise of stack traces. If it looked like this then that'd be good |
|
it would be even better if that was on the original |
|
Hi Sam, We didn't change anything yet in the output, actually. ScalaTest has been outputting the simple file name and line number for years, like this: [info] bang (FamilyFormatsSpec.scala:13) Where that information came from was the stack trace. There's a trait called StackDepth that has two fields, failedCodeFileName, failedCodeLineNumber, and a combo, failedCodeFileNameAndLineNumberString. (Sadly that last one didn't win the longest name in the Scala ecosystem contest. Some name in Slick was longer.) Anyway, where that information was grabbed as far back as 8 or 9 years ago was by inspecting the stack frames of the exception, and figuring out which stack frame represented the failing assertion or problem code. The stack frame usually had the simple file name and line number, but it did not have the fully qualified pathname of the file, which is what you need. We called the integer index into the stack frame array of that particular stack frame the "stack depth." Once we started porting ScalaTest to JavaScript, it became quickly obvious that this approach of inspecting the stack was less useful than on the JVM, because on Node.js and Phantom.js, there's a step whereby all JavaScript is collected into one big honking file, and that's what runs. So the filename that is reported this way was that big honking file's filename, and the line number was the line number inside that big honking file, and that was completely useless. It did work on Rhino, which runs on top of the JVM, but unfortunately runs 10 times slower compared to Node or Phantom. So slow that it is usually going to be impractical to use it. Still we spent months getting the stack depths to work on Rhino and not being comfortable they wouldn't work on Node or Phantom, where we figured most projects would ultimately need to run their tests. I was also worried a change to Scala.js would break our stack depth code, because it really is using implementation of Scala.js not interface, and sure enough that fear was realized. Sebastien D released a new version of Scala.js, and when we upgraded we had over 7000 failing stack depth tests. So we had to spend another several months overhauling ScalaTest to support Scala.js by changing how we get the filename and linenumber to the user, and at the same time kill two other birds with this one massive boulder. One bird was this very enhancement request. The main way we get this info now is through a macro that grabs the enclosing position. It is this guy: Note that there's a filePathname in there. That is the fully qualified file name that you desire. Although the fully qualifed file name is not available by inspecting stack frames, it is available if you ask the compiler in a macro. But you have to ask, and the way to do that is grab an implicit source.Position any place we might throw a StackDepth exception. So that was a major overhaul that took many weeks. This may give you an idea of how much impact that had: Once we had that grabbed, we needed to add it to the exception hierarchy somehow. It took us a while to figure out how to do it, and that's just being polished off this morning. Essentially, the stack depth exceptions now take either a source.Position or (as before) a function that given a StackDepthException will produce an integer stack depth. All the old constructors that take stack depth or that stack depth exception function will still be there, so that keeps any old code working, but now there are new constructors that either take a source position or that either. Here's an example: So now there's two tasks remaining to do this enhancement. First Im planning on adding a filePathname: Option[String] val in LineInFile, probably today. That will get the full pathname to reporters, including string reporters. The last enhancement is to Runner and the sbt Framework, which is to add a new command line switch that turns on an extra line of information in the string reporter output. That extra line of info will contain the info you need to one-click hop to the failure in Ensime. It will be an extra line, so it can just be something like: (/Users/fommil/projects/ensime/src/main/scala/org/ensime/FamilyFormatsSpec.scala:13) *** PATH *** I think in our earlier conversations you said this does need to show up in the textual output, correct? You can't just create a reporter and look at the LineInFile directly. LineInFile is here by the way: It will soon have an Option[String] with the filePathname. By the way, the other bird this source.Position boulder squashes is the problem of pointing to the desired stack depth from third-party assertions. This can be as simple as making a helper assertion in user code to add-on libraries like scalaz-scalatest. I never wanted to do this for @bwmcadams for example: https://github.com/scalatest/scalatest/pull/465/files Reason is that it was just too fragile to make public. What I prefer people do in such situations instead of monkey patching is just copy the source code into their own project and use it that way. But source.Position makes all this go away, because you don't need stack depth at all anymore. You can just do this: Now that file name and line number reported will be places where myHelperAssert is called, not the assert inside myHelperAssert. So that's also something I've wanted to solve for years, and this last major overhaul primarily motivated by Scala.js solves that problem and makes possible this enhancement for Ensime. |
|
I guess I was so keen to find something, I found it 😄 Looking forward to the next release! I have proposed a patch to make scalamock compatible. |
|
Have you considered using Haoyi's sourcecode library? On Sat, Jun 4, 2016, 3:08 PM Sam Halliday notifications@github.com wrote:
|
|
No, for a few reasons. One is that I've tried very hard to avoid Scala dependencies in general, because of binary compatibility difficulties that brings. Another is that although sourcecode can grab more info than we need, it doesn't offer the simple filename (so you'd need to parse it out of the full path, dealing with file separators), and the line number and file name must be grabbed with two different implicit lookups. We just need simple file name, full path name, and line number. The things here: Because we are adding implicit lookups, they need to be as fast as possible, and so we wanted to be able to control what it does. And we want all that, and only that, grabbed in one implicit lookup to minimize the compile time impact. Writing the Position macro was quite quick. What took many weeks of work was using it all over ScalaTest, i.e., replacing stack depth that worked on the JVM but not JavaScript with Position, which works everywhere, and doing it in such a way as there is little or no impact to existing users (i.e., no broken code). Since I think sourcecode came first, though, I did add a citation in the Scaladoc: |
|
@fommil Can you parse just by looking for parens and a colon? I.e., if we just change this: [info] bang (FamilyFormatsSpec.scala:13) Into this: [info] bang (/path/to/my/file/FamilyFormatsSpec.scala:13) Would you be able to parse just looking for parens and the colon? Bill |
|
we're already parsing for this (ever since I seen it this weekend) https://github.com/ensime/emacs-sbt-mode/blob/master/sbt-mode.el#L223-L225 but the problem is that it looks so similar to a stacktrace that it is quite delicate to make sure we catch it, so having some other indicator on the line would be good. Right now the only thing that is saving this from looking like a stack trace is the space between Having full file paths is a big win, especially on large projects 👍 |
|
Out of curiosity, what have you been doing with it since the weekend? I.e., don't you need the full path name to the source file? |
|
@fommil Ah, I see what you mean about the stack trace. It also does (<filename>: <lineNum>). I think we could get away with just changing that format. Hmm. What we did was enable a new option, V, to be given to string reporters. (V stands for Vi of course, which rules!) If V is specified we can not only show the full path there, but use a different format so that it is clear this is the droid you are looking for. Could do something like: or: Will that work if the path has spaces in it? What you'd look for is "* " a ":" and " **". In between "* " and ":" could be spaces possibly, and they would be part of the path. Between ":" and " **" should be an integral line number. |
|
Hi Sam, Actually, what I think looks better is: [info] bang That's if the V option is set. If not, then it will look like it already does, which is: [info] bang (FamilyFormatsSpec.scala:13) The full path is on its own line, which will start with possibly some white space for indentation then "At: <path to file>:<line number>". The reason I think this should go on its own line is it can be very long, and putting it on the same line as the error message will clutter the error message and make it harder to read. Will that work for you? |
|
Better if it's one line, I like the version with two asterisks (although it would be better if on the same line as the FAILED) |
Many text editors can perform automatic "jump to error" and so on if the absolute file and line number is included in a warn/error message.
It would be incredibly useful for emacs/Vim/sublime/atom integration if failing scalatests were reported with full path and line number (emacs even supports column number).
The failing line in the test need not be automatically discovered (unrolling the stacktrace can be a pretty tough job), it is enough to have the line that begins the test.
For example, today if my test fails I see this:
Note the failing line 20 in the error message is just some utility, not the actual test, and the filename is not absolute. We could potentially infer the filename using ENSIME, but that means this won't work without an ENSIME server.
but if it was like this, then I'd be able to jump to that failing test easily:
Then the filepath and line can be extracted with a pattern match.
Some people might find it useful to have the filename/line on ignored tests as well.
I've tried my best to get something working in https://github.com/hvesalai/sbt-mode/pull/29 but my changes only allow the user to jump the cursor to the failure message, it doesn't open up the failed test's source code.
The text was updated successfully, but these errors were encountered: