This is an archive of the discontinued Mercurial Phabricator instance.

tests: comprehensively test exit handling
Needs RevisionPublic

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

Download Raw Diff

Details

Reviewers

durin42
marmoute

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptMay 6 2018, 12:11 AM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

indygreg added a child revision: D3445: dispatch: validate return type from dispatch() return value.May 6 2018, 12:11 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:

Reimplement CPython's exit/error handling in Rust
Reimplement CPython's exit/error handling in Python in dispatch [so the end state is more well-defined]
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.

indygreg added a child revision: D3537: dispatch: minor code refactor.May 12 2018, 12:48 AM

indygreg removed a child revision: D3445: dispatch: validate return type from dispatch() return value.May 12 2018, 12:49 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.

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

Baymax does not catch stuff in Accepted state, but this diff is over 2 years old, so resubmit if still relevant.

This revision now requires changes to proceed.Apr 22 2020, 11:58 AM

Herald added a subscriber: mercurial-patches. · View Herald TranscriptApr 22 2020, 11:58 AM

Revision Contents
Changeset List

			Path	Packages
M			tests/test-dispatch.t (91 lines)

Commit	Parents	Author	Summary	Date
		Gregory Szorc		May 5 2018, 10:28 PM

Status	Author	Revision
Accepted	kevincox	D3454 Assign to result from block.
Accepted	indygreg	D3447 rust: make exit handling consistent with `hg`
Accepted	indygreg	D3446 dispatch: move sys.exit() to caller (API)
Needs Revision	indygreg	D3445 dispatch: validate return type from dispatch() return value
Closed	indygreg	D3537 dispatch: minor code refactor
Needs Revision	indygreg	D3444 tests: comprehensively test exit handling

Diff 8459

tests/test-dispatch.t

	chg: abort: failed to getcwd (errno = *) (glob)			chg: abort: failed to getcwd (errno = *) (glob)
	abort: error getting current working directory: * (glob)			abort: error getting current working directory: * (glob)
	sh: 0: getcwd() failed: $ENOENT$			sh: 0: getcwd() failed: $ENOENT$
	Since the exact behavior depends on the shell, only check it returns non-zero.			Since the exact behavior depends on the shell, only check it returns non-zero.
	$ HGDEMANDIMPORT=disable hg version -q 2>/dev/null \|\| false			$ HGDEMANDIMPORT=disable hg version -q 2>/dev/null \|\| false
	[1]			[1]

	#endif			#endif

				Test various exit codes and methods

				$ cd $TESTTMP

				$ cat >> $TESTTMP/exitmodes.py <<'EOF'
				> from mercurial import pycompat, registrar, commands
				> cmdtable = {}
				> command = registrar.command(cmdtable)
				> @command(b'exit', [], b'[MODE] [VALUE]', norepo=True)
				> def exitmodes(ui, mode, value=None):
				> if mode == b'returnint':
				> return int(value)
				> elif mode == b'returnstring':
				> # sysstr is to make testing easier, since Python 2 and 3 will
				> # format bytes and unicode types differently when printed.
				> return pycompat.sysstr(value)
				> elif mode == b'returnnone':
				> return None
				> elif mode == b'returnempty':
				> return
				> elif mode == b'systemexitint':
				> raise SystemExit(int(value))
				> elif mode == b'systemexitstring':
				> raise SystemExit(pycompat.sysstr(value))
				> elif mode == b'systemexitnoarg':
				> raise SystemExit()
				> elif mode == b'systemexitnone':
				> raise SystemExit(None)
				> elif mode == b'returndict':
				> return {'key1': 'value1'}
				> elif mode == b'systemexitdict':
				> raise SystemExit({'key1': 'value1'})
				> else:
				> raise Exception('unknown mode: %s' % mode)
				> EOF

				$ cat >> $HGRCPATH <<'EOF'
				> [extensions]
				> exitmodes = $TESTTMP/exitmodes.py
				> EOF

				$ hg exit returnint 42
				[42]

				$ hg exit returnstring 'some message'
				Traceback (most recent call last):
				File "/hg", line , in <module> (glob)
				dispatch.run()
				File "/mercurial/dispatch.py", line , in run (glob)
				sys.exit(status & 255)
				TypeError: unsupported operand type(s) for &: 'str' and 'int'
				[1]

				$ hg exit returnnone

				$ hg exit returnempty

				$ hg exit returndict
				Traceback (most recent call last):
				File "/hg", line , in <module> (glob)
				dispatch.run()
				File "/mercurial/dispatch.py", line , in run (glob)
				sys.exit(status & 255)
				TypeError: unsupported operand type(s) for &: 'dict' and 'int'
				[1]

				$ hg exit systemexitint 42
				[42]

				$ hg exit systemexitstring 'failure message'
				Traceback (most recent call last):
				File "/hg", line , in <module> (glob)
				dispatch.run()
				File "/mercurial/dispatch.py", line , in run (glob)
				sys.exit(status & 255)
				TypeError: unsupported operand type(s) for &: 'str' and 'int'
				[1]

				$ hg exit systemexitnoarg

				$ hg exit systemexitnone

				$ hg exit systemexitdict
				Traceback (most recent call last):
				File "/hg", line , in <module> (glob)
				dispatch.run()
				File "/mercurial/dispatch.py", line , in run (glob)
				sys.exit(status & 255)
				TypeError: unsupported operand type(s) for &: 'dict' and 'int'
				[1]

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	8459			May 6 2018, 12:11 AM	★	★