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

Use error/1,2 instead of exit/1 #31

Merged
merged 9 commits into from
Aug 8, 2014
Merged

Use error/1,2 instead of exit/1 #31

merged 9 commits into from
Aug 8, 2014

Conversation

erszcz
Copy link
Member

@erszcz erszcz commented Jul 25, 2014

This plays much nicer with ct_tty_hook.erl included in new ejabberd_tests. Instead of getting just the error reason, we also get the stacktrace showing exact location of the error.

I didn't test in the shell/Common Test results all possible/expected outputs these changes cause, but this is not a change in functionality and only affects the amount of information in error logs. If any readability problems or duplicated logs show up we can fix them on the go.

Basically, the difference boils down to this example:

exit/1:

> catch exit(asd).
{'EXIT',asd}

error/1:

> catch error(asd).
{'EXIT',{asd,[{erl_eval,do_apply,6,
                        [{file,"erl_eval.erl"},{line,573}]},
              {erl_eval,expr,5,[{file,"erl_eval.erl"},{line,357}]},
              {shell,exprs,7,[{file,"shell.erl"},{line,674}]},
              {shell,eval_exprs,7,[{file,"shell.erl"},{line,629}]},
              {shell,eval_loop,3,[{file,"shell.erl"},{line,614}]}]}}

error/2:

> catch error(asd, [123, qwe, zxc]).
{'EXIT',{asd,[{erl_eval,do_apply,
                        [123,qwe,zxc],
                        [{file,"erl_eval.erl"},{line,573}]},
              {erl_eval,expr,5,[{file,"erl_eval.erl"},{line,357}]},
              {shell,exprs,7,[{file,"shell.erl"},{line,674}]},
              {shell,eval_exprs,7,[{file,"shell.erl"},{line,629}]},
              {shell,eval_loop,3,[{file,"shell.erl"},{line,614}]}]}}

There are more places in Escalus code base where exit/1,2 is used, but I believe it's appropriate there as it is deliberately used to kill a process other than the current one.

@pzel
Copy link
Contributor

pzel commented Jul 25, 2014

Great code, man!
That said -- we should be adding small unit tests for escalus features, so regressions don't creep in.

Essentially, your pull request comment could be 'codified' into a test or two.

You reply:

  • Yeah, whatever -> Simon merges
  • Yeah, in a sec -> SImon merges happily with your test(s) added to the pull request

This is internal API and the build system doesn't support convenient
testing of this kind of stuff right now.
This plays much nicer with `ct_tty_hook.erl` included in new
`ejabberd_tests`.
Instead of getting just the error reason, we also get the stacktrace
showing exact location of the error.
Basically, the difference boils down to this example:

`exit/1`:

    > catch exit(asd).
    {'EXIT',asd}

`error/1`:

    > catch error(asd).
    {'EXIT',{asd,[{erl_eval,do_apply,6,
                            [{file,"erl_eval.erl"},{line,573}]},
                  {erl_eval,expr,5,[{file,"erl_eval.erl"},{line,357}]},
                  {shell,exprs,7,[{file,"shell.erl"},{line,674}]},
                  {shell,eval_exprs,7,[{file,"shell.erl"},{line,629}]},
                  {shell,eval_loop,3,[{file,"shell.erl"},{line,614}]}]}}

`error/2`:

    > catch error(asd, [123, qwe, zxc]).
    {'EXIT',{asd,[{erl_eval,do_apply,
                            [123,qwe,zxc],
                            [{file,"erl_eval.erl"},{line,573}]},
                  {erl_eval,expr,5,[{file,"erl_eval.erl"},{line,357}]},
                  {shell,exprs,7,[{file,"shell.erl"},{line,674}]},
                  {shell,eval_exprs,7,[{file,"shell.erl"},{line,629}]},
                  {shell,eval_loop,3,[{file,"shell.erl"},{line,614}]}]}}
@erszcz
Copy link
Member Author

erszcz commented Aug 7, 2014

Yeah, whatever ;p

pzel pushed a commit that referenced this pull request Aug 8, 2014
Use `error/1,2` instead of `exit/1`
@pzel pzel merged commit 18c621f into master Aug 8, 2014
@erszcz erszcz deleted the error-locations branch October 20, 2014 13:04
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

Successfully merging this pull request may close these issues.

2 participants