Add a test against infinite prompt during cluster setup #1031
Conversation
bamarni
changed the title
bamarni
force-pushed
the
DCOS-15590
branch
2 times, most recently
from
885c37e
to
26db8dc
Compare
bamarni
changed the title
|
|
||
| try: | ||
| returncode, stdout, _ = exec_command( | ||
| ['dcos', 'cluster', 'setup', |
There was a problem hiding this comment.
All arguments at the same level should either be on the same line or split onto individual lines, e.g. not
returncode, stdout, _ = exec_command(
['dcos', 'cluster', 'setup',
'https://dcos.snakeoil.mesosphere.com'],
timeout=30, stdin=subprocess.DEVNULL)
but rather
returncode, stdout, _ = exec_command(
['dcos', 'cluster', 'setup', 'https://dcos.snakeoil.mesosphere.com'],
timeout=30,
stdin=subprocess.DEVNULL)
or
returncode, stdout, _ = exec_command(
['dcos',
'cluster',
'setup',
'https://dcos.snakeoil.mesosphere.com'],
timeout=30,
stdin=subprocess.DEVNULL)
It makes it much easier to look at and understand without haveing to spend alot of time reading the code.
| @@ -36,7 +36,7 @@ def exec_command(cmd, env=None, stdin=None): | |||
|
|
|||
| # This is needed to get rid of '\r' from Windows's lines endings. | |||
| stdout, stderr = [std_stream.replace(b'\r', b'') | |||
| for std_stream in process.communicate()] | |||
| for std_stream in process.communicate(timeout=timeout)] | |||
There was a problem hiding this comment.
Ideally, this would be split out into it's own separate commit. With the test change in its own commit as well.
This will make the subprocess getting killed if it doesn't terminate
within {timeout} seconds and an exception will be raised.
Prior to 1254a2f and when running "dcos cluster setup" command as non-interactive, the process would run and ask for user input forever. This adds an integration test which fails if the process doesn't exit by itself with an error within 15 seconds. https://jira.mesosphere.com/browse/DCOS-15590
|
I've just updated the PR, I was missing a cleanup in the helper and added a link about this. I just noticed that this test only passes if I use the cloud formation template for an EE cluster. However for an open source or custom cluster, the |
|
ok to test |
| @@ -683,7 +697,8 @@ def test_install_no(): | |||
| b'catalog-terms-conditions/#community-services\n' | |||
| b'A sample pre-installation message\n' | |||
| b'Continue installing? [yes/no] Exiting installation.\n' | |||
| ) | |||
| ), | |||
| returncode=1 | |||
There was a problem hiding this comment.
Hmm. At first I thought this seemed strange to return a 1 if the user supplied no as valid input. The reason being that the command exits cleanly, whether the user inputs yes or no (the only difference being whether the package was actually installed or not). I would have expected a 1 only if there was some internal error that made the command exit prematurely.
On thinking about it a bit more though, I agree with the semantics you have in place. Use a 0 to indicate that the command successfully carried out what you asked it to; use a 1 to indicate that it didn't.
An example demonstrating these semantics in a standard unix environment:
$ ls
foo bar baz
$ ls | grep foo
$ echo $?
0
$ ls | grep qux
$ echo $?
1
Prior to 1254a2f and when running
"dcos cluster setup" command as non-interactive, the process would run
and ask for user input forever.
This adds an integration test which fails if the process doesn't exit by
itself with an error within 30 seconds.
https://jira.mesosphere.com/browse/DCOS-15590