Page MenuHomePhabricator

tests: comprehensively test exit handling
AcceptedPublic

Authored by indygreg on May 6 2018, 12:11 AM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Summary

Rust hg currently has some test failures around exit handling.
This commit establishes some centralized tests around exit handling so
that we can unify the behavior of our Rust frontend and python.

There are some added tests that use a value other than an integer
or None for the exit code. The docs for sys.exit() say such a value
is allowed. However, Mercurial currently crashes in this case. Upcoming
commits will teach Mercurial to handle these values.

This does introduce a few Python 3 test failures. However, this test
already has a few failures. And the failures being introduced should
mostly go away with subsequent commits. So I think this is acceptable.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.May 6 2018, 12:11 AM
yuja added a subscriber: yuja.May 6 2018, 8:29 AM

I generally like the direction of this series, but I think there's no point
to extend Mercurial's exit code handling to support all weird Python types.

Only ints and (None for 0) are ever valid.

In D3444#54922, @yuja wrote:

I generally like the direction of this series, but I think there's no point
to extend Mercurial's exit code handling to support all weird Python types.
Only ints and (None for 0) are ever valid.

The reason I did this is because from the context or rhg, we don't have CPython's default exit handling to fall back on. Our choices are:

  1. Reimplement CPython's exit/error handling in Rust
  2. Reimplement CPython's exit/error handling in Python in dispatch [so the end state is more well-defined]
  3. Do something crude in rhg when we hit special cases that CPython would normally deal with.

Since extensions could do weird things, I figured 2 was the best option.

But I'll take a look at this series again and re-evaluate if we can simplify things. I just thought I'd brain dump on what I'm thinking.

yuja added a comment.May 12 2018, 1:52 AM
> I generally like the direction of this series, but I think there's no point
>  to extend Mercurial's exit code handling to support all weird Python types.
>
> Only ints and (None for 0) are ever valid.
The reason I did this is because from the context or `rhg`, we don't have CPython's default exit handling to fall back on. Our choices are:
1. Reimplement CPython's exit/error handling in Rust
2. Reimplement CPython's exit/error handling in Python in dispatch [so the end state is more well-defined]
3. Do something crude in `rhg` when we hit special cases that CPython would normally deal with.
Since extensions could do weird things, I figured 2 was the best option.

I don't fully understand the situation in rhg, but maybe we can instead
fix scmutil.callcatch() to either translate non-int SystemExit to integer
or raise ProgrammingError as a bad use of sys.exit() in hg. No SystemExit
should be raised outside of the callcatch().

I'm against allowing command functions to return any objects other than
int or None because it's useless.

durin42 accepted this revision.
durin42 added a subscriber: durin42.

This LG, esp with D3445 which appears to be its child, but D3445 still needs rebased AFAICT

This revision is now accepted and ready to land.May 31 2018, 3:18 PM