Code review comment for lp:~jamesodhunt/apport/bug-1256268

Hey James,

thanks for working on this!

1) It's better to move TestSuiteUserInterface into a separate test/ui.py and import it from test_ui.py and the new test.

2) The usual idiom for in-process changes is that the defaults are in a global variable and the test changes the global variable. That would work if the logic was in apport/report.py itself, but as it's in a separate hook the only way that comes to my mind is to check an environment variable in the hook.

I wonder if we actually want a percentage memory treshold. A process is "broken" if it unexpectedly takes 1 GB of memory regardless of whether you only have 1 or 64 GB of RAM. My gut feeling is that it should rather be flagged if the apported process is in the top 3 memory users, or perhaps also just if is using > 500 MB RAM.

Global: Can you please rename "hog" to perhaps "leaks", to avoid animal references and slang?

22 +def get_pid_path(pid):

That's fairly complicated; why isn't reading the /proc/pid/exe symlink sufficient? Apport does that in other places like add_proc_info(), so if you want to reuse any of its logic, call it on a temporary empty dictionary and use report['ExecutablePath'] or perhaps also InterpreterPath.

47 + path = [x for x in env if x.startswith('PATH=')]

part of get_pid_path(), so it might go away entirely, but if not let's rather call "which". shutils.which will make this easier in the future, but we still need to support python 2.x.

61 +def run_top(max_lines, sort_field):

I wonder if running "ps" instead of "top" might be easier to parse? You can precisely define the output fields, sort order, etc. But if "top" works for you, that's fine.

142 + @report: report,

Please don't put trailing commas after parameter descriptions.

379 + self.assertTrue(set(['MemoryHogs']).issubset(set(self.ui.report.keys())))

This is better written as

    self.assertIn('MemoryHogs', self.ui.report)

However, this ought to be more specific to actually ensure that you grabbed the test process, not any process.

This should also get some negative tests, to ensure that well-behaved processes do not turn up in those new keys.

Thanks!