Various improvements #1
Conversation
|
I've split this up into logical separate changes and I'd love feedback! |
…the previous stdout log instead of executing tests.
|
Meh, I should've assigned this to myself. Somehow, I didn't get notified by your commits split-up. I'll look into it today. |
| suite = config['suite'] | ||
| else: | ||
| subdir = None | ||
| suite = None |
There was a problem hiding this comment.
These subdir and suite declarations could be removed. After making this check, we should have subdir=config.get('subdir'), suite=config.get('suite') in the constructor below.
There was a problem hiding this comment.
It's nice to not include the keys in the config file when no specific suite or subdirectory is desired, and it's not clear to me if there's an API that returns default values for nonexistent keys.
There was a problem hiding this comment.
Yes, so, I was thinking of something like,
if 'subdir' in config and 'suite' not in config:
sys.exit(1)
# defaults to None
watcher = IntermittentWatcher(..., suite=config.get('suite'), subdir=config.get('subdir'))| if is_dummy: | ||
| print '\033[1m\033[93mRunning in dummy mode: API will not be used!\033[0m' | ||
| if os.path.exists(log_path): | ||
| with open(log_path, 'r') as fd: | ||
| self.results = json.load(fd) | ||
| else: | ||
| self.results = {} |
There was a problem hiding this comment.
Nit: This should be moved to the top along with the other declarations. Let's override if the log exists already.
| with open(STDOUT_LOG, 'wb') as f: | ||
| f.write(out) | ||
|
|
||
| if out.find(OUTPUT_HEAD) == -1: |
There was a problem hiding this comment.
Nit: Have a out = '' and result = out.find(OUTPUT_HEAD) above please.
| @@ -42,6 +53,8 @@ def execute(self, command): | |||
| sys.stdout.flush() | |||
| out += line | |||
| proc.wait() | |||
| if not suppress and proc.returncode != 0: | |||
| raise subprocess.CalledProcessError(proc.returncode, command, out) | |||
There was a problem hiding this comment.
I'm curious. What's the purpose of having a suppress here? We don't seem to use this option anywhere?
There was a problem hiding this comment.
We do - all of the git commands use the implicit value of suppress=False, meaning that errors encountered while running them will abort execution.
There was a problem hiding this comment.
Wonderful. I didn't see that coming.
| @@ -96,7 +125,7 @@ def run(self): | |||
| self.results[test]['subtest'][subtest]['data'] = result | |||
|
|
|||
| self.log('Cleaning up recordings...') | |||
| for test, result in self.results.iteritems(): | |||
| for test, result in list(self.results.iteritems()): | |||
There was a problem hiding this comment.
I'd gone for iteritems only for the sake of having an iterator here. Why do we need a list? If we really need it, we could just have self.results.items()
There was a problem hiding this comment.
The code in the for loop modifies self.results, which breaks if an iterator is in use.
There was a problem hiding this comment.
Right, self.results.items() then :)
These are changes that I've made that allowed me to run this for several days uninterrupted using the no-api mode. I want to split them up into logical commits before merging, but I figured I should store them somewhere besides the machine under my desk.