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

panics with new lines are not matched #85

Open
colin-kiegel opened this issue Mar 19, 2017 · 5 comments
Open

panics with new lines are not matched #85

colin-kiegel opened this issue Mar 19, 2017 · 5 comments

Comments

@colin-kiegel
Copy link
Contributor

The panic message of this is matched by atom-build-cargo

#[test]
fn test() {
    panic!("Hello world!")
}

However this is not matched due to the new lines

#[test]
fn todo() {
    panic!("TODO:
- write docs
- ... ")
}

Is it possible to include new lines into the regular expression of these panics?

@alygin
Copy link
Collaborator

alygin commented Mar 20, 2017

Output is parsed and matched against regular expressions line by line now, so adding new lines into the regex won't help. But the case can be handled in the tryParsePanic() method by detecting the start and the end of the panic message separately and returning the actual processed lines count. The method will be easy to refactor this way.

Though I'm not quite sure this is the right way to go. Supporting multiline panic messages will make the parser prone to several other breakages. For instance, if one day Rust changes the source code link format in the panic messages, we will start displaying incorrect data for all the panics in notification panels. It will also break on the lines that contain something similar to a source code link.

Wouldn't it be more convenient to use TODO-comments, or declare something like a todo! macro that will print all the todo-stuff to stderr and then panic with a simple one-line message?

@colin-kiegel
Copy link
Contributor Author

Yes, now that I know it, I can definitely workaround that. I actually started with multiple todo testcases #[test] fn write_docs() { unimplemented!() } etc. However in an attempt to reduce the clutter from multiple todos panicking all the time I switched to one multi-line panic. I completely agree that multi-line panics are somewhat bad style and should be avoided at least in published code.

However

  • I will probably not be the last one to run into this issue - there might be some multi-line panics out there somewhere
  • it feels surprising that atom-build-cargo gives no feedback about multi-line panics.

I would already consider it a huge improvement to partially match a multi-line panic, like "hey there was a panic, we couldn't capture all of details because the message was garbled, but it occurred in FILE:ROW:COL". Or just "there was at least one error we couldn't match, please look in the build output for further details".

On the other hand, it would make sense for cargo to present panics in json format in the first place, wouldn't it?

@colin-kiegel
Copy link
Contributor Author

There is actually a use case in assert-rs/assert_cli#14 (comment), where some assertion macro will compare the output of a shell command with some expected value and panic with a multiline diff-message.

I still see your problem in supporting this, but just want to mention that there are use cases.

@alygin
Copy link
Collaborator

alygin commented Mar 21, 2017

I'll try to figure something out. The vast majority of the messages are one-liners and they will effectively continue to be processed the same way they do now. But with multi-line messages you'll be at your own risk :)

And you're absolutely right about output from concurrent threads. But I think this is the case we have no chance to handle properly at the time.

@colin-kiegel
Copy link
Contributor Author

colin-kiegel commented Apr 29, 2017

@alygin Rust is going to introduce multi-line panics soon-ish for assert_eq:

It's supposed to look like this

---- log_packet::tests::syntax_error_test stdout ----
        thread 'log_packet::tests::syntax_error_test' panicked at 'assertion failed: `(left == right)`

left:  `"Syntax Error: a.rb:1: syntax error, unexpected end-of-input\n\n"`
right: `"Syntax error: a.rb:1: syntax error, unexpected end-of-input\n\n"`
         ~~~~~~ ^ ~~~~
', src/log_packet.rs:102
note: Run with `RUST_BACKTRACE=1` for a backtrace.

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

2 participants