Re: Review Request 26802: Set a default for the error log dir
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26802/#review58217 --- This patch does not apply cleanly on master (53f4e73), do you need to rebase? - Aurora ReviewBot On Oct. 16, 2014, 8:22 p.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26802/ --- (Updated Oct. 16, 2014, 8:22 p.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Bugs: AURORA-827 https://issues.apache.org/jira/browse/AURORA-827 Repository: aurora Description --- Set a default for the error log dir This has been a weird issue to wrap my head around, it's really using lots of low-level systems (writing tracebacks to files) so I feel like I've done some weird things in the tests. Feedback and critque welcome + appreciated. Diffs - src/main/python/apache/aurora/client/cli/__init__.py da9d5b6ba4d22ba1f444341b97bbcfaf7889a4a8 src/test/python/apache/aurora/client/cli/BUILD d33e86643a59879c115876c98bd1dc19aa7ae61c src/test/python/apache/aurora/client/cli/test_aurora_command_line.py PRE-CREATION Diff: https://reviews.apache.org/r/26802/diff/ Testing --- [tw-172-25-132-201 aurora (yasumoto/error_log_dir_default)]$ ./pants src/test/python/apache/aurora/client/cli:aurora_command_line Build operating on top level addresses: set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, aurora_command_line)]) test session starts = platform darwin -- Python 2.7.5 -- py-1.4.25 -- pytest-2.6.3 plugins: cov, timeout collected 1 items src/test/python/apache/aurora/client/cli/test_aurora_command_line.py . == 1 passed in 0.63 seconds == src.test.python.apache.aurora.client.cli.aurora_command_line . SUCCESS Thanks, Joe Smith
Review Request 26802: Set a default for the error log dir
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26802/ --- Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji. Bugs: AURORA-827 https://issues.apache.org/jira/browse/AURORA-827 Repository: aurora Description --- Set a default for the error log dir This has been a weird issue to wrap my head around, it's really using lots of low-level systems (writing tracebacks to files) so I feel like I've done some weird things in the tests. Feedback and critque welcome + appreciated. Diffs - src/main/python/apache/aurora/client/cli/__init__.py da9d5b6ba4d22ba1f444341b97bbcfaf7889a4a8 src/test/python/apache/aurora/client/cli/BUILD d33e86643a59879c115876c98bd1dc19aa7ae61c src/test/python/apache/aurora/client/cli/test_aurora_command_line.py PRE-CREATION Diff: https://reviews.apache.org/r/26802/diff/ Testing --- [tw-172-25-132-201 aurora (yasumoto/error_log_dir_default)]$ ./pants src/test/python/apache/aurora/client/cli:aurora_command_line Build operating on top level addresses: set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, aurora_command_line)]) test session starts = platform darwin -- Python 2.7.5 -- py-1.4.25 -- pytest-2.6.3 plugins: cov, timeout collected 1 items src/test/python/apache/aurora/client/cli/test_aurora_command_line.py . == 1 passed in 0.63 seconds == src.test.python.apache.aurora.client.cli.aurora_command_line . SUCCESS Thanks, Joe Smith
Re: Review Request 26802: Set a default for the error log dir
On Oct. 16, 2014, 10 a.m., Bill Farner wrote: src/main/python/apache/aurora/client/cli/__init__.py, line 114 https://reviews.apache.org/r/26802/diff/1/?file=722890#file722890line114 Can you try to remove noqa, and if it is suppressing an error, leave a comment indicating why the pyflakes errors are suppressed? Yep, these should've been classmethods. On Oct. 16, 2014, 10 a.m., Bill Farner wrote: src/main/python/apache/aurora/client/cli/__init__.py, line 118 https://reviews.apache.org/r/26802/diff/1/?file=722890#file722890line118 Ditto also, more concise: return Context.ERROR_LOG_DIR or '.' Done. On Oct. 16, 2014, 10 a.m., Bill Farner wrote: src/test/python/apache/aurora/client/cli/BUILD, line 140 https://reviews.apache.org/r/26802/diff/1/?file=722891#file722891line140 remove extra newline Done. On Oct. 16, 2014, 10 a.m., Bill Farner wrote: src/test/python/apache/aurora/client/cli/test_aurora_command_line.py, line 34 https://reviews.apache.org/r/26802/diff/1/?file=722892#file722892line34 I'm not sure what the right answer is here, and we may be too far down the monkeypatch road to change tacks in this effort, but overriding builtins feels like a special kind of abstraction violation. Do any other reviewers know the lay of the land well enough to suggest a better way? Ideally we would be injecting this functionallity rather than patching it, but i don't want to turn a small fix into a giant yak shave. Maxim Khutornenko wrote: Seems like the appropriate use of patching given the current implementation. Refactoring here would only hurt readability and would rather feel unnatural. Joshua Cohen wrote: +1, I'm not an expert on what is or isn't pythonic, but this certainly seems in line with how I've done testing in other dynamic languages. I'm not sure cluttering up the interface with explicit injection points for dependencies is warranted/necessary when you can just patch them in tests? Unless we switch to using interfaces and injection, patching is indeed a ~recommended approach. - Joe --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26802/#review56929 --- On Oct. 15, 2014, 11:02 p.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26802/ --- (Updated Oct. 15, 2014, 11:02 p.m.) Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji. Bugs: AURORA-827 https://issues.apache.org/jira/browse/AURORA-827 Repository: aurora Description --- Set a default for the error log dir This has been a weird issue to wrap my head around, it's really using lots of low-level systems (writing tracebacks to files) so I feel like I've done some weird things in the tests. Feedback and critque welcome + appreciated. Diffs - src/main/python/apache/aurora/client/cli/__init__.py da9d5b6ba4d22ba1f444341b97bbcfaf7889a4a8 src/test/python/apache/aurora/client/cli/BUILD d33e86643a59879c115876c98bd1dc19aa7ae61c src/test/python/apache/aurora/client/cli/test_aurora_command_line.py PRE-CREATION Diff: https://reviews.apache.org/r/26802/diff/ Testing --- [tw-172-25-132-201 aurora (yasumoto/error_log_dir_default)]$ ./pants src/test/python/apache/aurora/client/cli:aurora_command_line Build operating on top level addresses: set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, aurora_command_line)]) test session starts = platform darwin -- Python 2.7.5 -- py-1.4.25 -- pytest-2.6.3 plugins: cov, timeout collected 1 items src/test/python/apache/aurora/client/cli/test_aurora_command_line.py . == 1 passed in 0.63 seconds == src.test.python.apache.aurora.client.cli.aurora_command_line . SUCCESS Thanks, Joe Smith
Re: Review Request 26802: Set a default for the error log dir
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26802/#review56983 --- I have no time to review this. Please remove me from review. - Zameer Manji On Oct. 16, 2014, 10:51 a.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26802/ --- (Updated Oct. 16, 2014, 10:51 a.m.) Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji. Bugs: AURORA-827 https://issues.apache.org/jira/browse/AURORA-827 Repository: aurora Description --- Set a default for the error log dir This has been a weird issue to wrap my head around, it's really using lots of low-level systems (writing tracebacks to files) so I feel like I've done some weird things in the tests. Feedback and critque welcome + appreciated. Diffs - src/main/python/apache/aurora/client/cli/__init__.py da9d5b6ba4d22ba1f444341b97bbcfaf7889a4a8 src/test/python/apache/aurora/client/cli/BUILD d33e86643a59879c115876c98bd1dc19aa7ae61c src/test/python/apache/aurora/client/cli/test_aurora_command_line.py PRE-CREATION Diff: https://reviews.apache.org/r/26802/diff/ Testing --- [tw-172-25-132-201 aurora (yasumoto/error_log_dir_default)]$ ./pants src/test/python/apache/aurora/client/cli:aurora_command_line Build operating on top level addresses: set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, aurora_command_line)]) test session starts = platform darwin -- Python 2.7.5 -- py-1.4.25 -- pytest-2.6.3 plugins: cov, timeout collected 1 items src/test/python/apache/aurora/client/cli/test_aurora_command_line.py . == 1 passed in 0.63 seconds == src.test.python.apache.aurora.client.cli.aurora_command_line . SUCCESS Thanks, Joe Smith
Re: Review Request 26802: Set a default for the error log dir
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26802/#review56994
---
Tests failed:
src/test/python/apache/aurora/client/cli/test_create.py::TestClientCreateCommand::test_create_job_fail_and_write_log
Fatal error running command; traceback can be found in ./aurora-23.error-log
FAILED
FAILURES
___
TestClientCreateCommand.test_create_job_fail_and_write_log
___
self = test_create.TestClientCreateCommand
testMethod=test_create_job_fail_and_write_log
def test_create_job_fail_and_write_log(self):
Check that when an unknown error occurs during command execution,
the command-line framework catches it, and writes out an error log
file
containing the details of the error, including the command-line
arguments
passed to aurora to execute the command, and the stack trace of the
error.
mock_context = FakeAuroraCommandContext()
with contextlib.nested(
patch('time.time', return_value=23),
patch('apache.aurora.client.cli.jobs.Job.create_context',
return_value=mock_context)):
api = mock_context.get_api('west')
api.create_job.side_effect = UnknownException()
with temporary_file() as fp:
fp.write(self.get_valid_config())
fp.flush()
cmd = AuroraCommandLine()
result = cmd.execute(['job', 'create', '--wait-until=RUNNING',
'--error-log-dir=./logged-errors', 'west/bozo/test/hello',
fp.name])
assert result == EXIT_UNKNOWN_ERROR
with open(./logged-errors/aurora-23.error-log, r) as logfile:
error_log = logfile.read()
E IOError: [Errno 2] No such file or directory:
'./logged-errors/aurora-23.error-log'
Within `~/.aurora/errors/aurora-23.error-log` (note that it is not
`./aurora-23.error-log` as the client output purports:
ERROR LOG: command arguments = ['job', 'create', '--wait-until=RUNNING',
'west/bozo/test/hello', '--error-log-dir=./error-logs', '/tmp/tmp0p7ioZ']
Traceback (most recent call last):
File /tmp/tmpA9GsN3/apache/aurora/client/cli/__init__.py, line 446, in
execute
return self._execute(args)
File /tmp/tmpA9GsN3/apache/aurora/client/cli/__init__.py, line 400, in
_execute
noun, context = self._parse_args(args)
File /tmp/tmpA9GsN3/apache/aurora/client/cli/__init__.py, line 357, in
_parse_args
context = noun.create_context()
File /tmp/tmpA9GsN3/.deps/mock-1.0.1-py2-none-any.whl/mock.py, line
955, in __call__
return _mock_self._mock_call(*args, **kwargs)
File /tmp/tmpA9GsN3/.deps/mock-1.0.1-py2-none-any.whl/mock.py, line
1010, in _mock_call
raise effect
Exception: Argh
- Bill Farner
On Oct. 16, 2014, 8:22 p.m., Joe Smith wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26802/
---
(Updated Oct. 16, 2014, 8:22 p.m.)
Review request for Aurora, Maxim Khutornenko and Bill Farner.
Bugs: AURORA-827
https://issues.apache.org/jira/browse/AURORA-827
Repository: aurora
Description
---
Set a default for the error log dir
This has been a weird issue to wrap my head around, it's really using lots
of low-level systems (writing tracebacks to files) so I feel like I've done
some weird things in the tests. Feedback and critque welcome + appreciated.
Diffs
-
src/main/python/apache/aurora/client/cli/__init__.py
da9d5b6ba4d22ba1f444341b97bbcfaf7889a4a8
src/test/python/apache/aurora/client/cli/BUILD
d33e86643a59879c115876c98bd1dc19aa7ae61c
src/test/python/apache/aurora/client/cli/test_aurora_command_line.py
PRE-CREATION
Diff: https://reviews.apache.org/r/26802/diff/
Testing
---
[tw-172-25-132-201 aurora (yasumoto/error_log_dir_default)]$ ./pants
src/test/python/apache/aurora/client/cli:aurora_command_line
Build operating on top level addresses:
set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD,
aurora_command_line)])
test
session starts
