This is the only piece of automationutils which doesn't have a clear place; any ideas of where we can move it?

I filed bug 1087474 about a refactoring we had talked about, maybe it can wind up there?

That seems fine. Another option might be some sort of mozdebug module that has stuff like this and killAndGetStack.

This and processLeakLog are really the only things left in automationutils. Shall we go ahead and move these into moztest? Or create a mozdebug module per comment #2?

Well processLeakLog is now in his own package, mozleak. We discussed on irc and it seems like a mozscreenshot package is also the way to go for dumpScreen, since there are no other place in mozbase where it could make sense. So in fact dumpScreen does use another function, printstatus, and printstatus is used by mochitest, reftest and automations.py.in: https://dxr.mozilla.org/mozilla-central/search?q=printstatus&case=true&redirect=true So first thing is to remove that - once this is done, moving dumpScreen will be easy. I discussed with jgraham, and it seems that the best way to do that is to integrate this functionality in a structured logging action, then maybe make mozprocess (automatically?) log what we need. I could work on this once this is a bit clarified: I don't know mozlog yet - the structured thing, and I'm not sure how/if we should integrate this into mozprocess.

Some more discussion with :jgraham, I think we now have a way to (re)move printstatus. So basically, we need to add two log actions, "process_start" and "process_stop" (maybe "process_exit" makes more sense ?). Then in the formatters, we implement the strsig function (defined in automationutils.py and used by printstatus) and use that to at least TBPL and mach formatters. So far, pretty good, we then use that to log where it is required, replacing printstatus calls. Well, the only exception is that reftest do not use structured log. :( So two choices here. Make reftest structured-log aware now, or copy paste the printstatus function into reftest (waiting for reftest to use structured log). :jgraham proposed that we could just copy paste for now. Not really elegant, but this would allow us to move on, so I tend to agree to do that (with an important comment under the copy pasted function). I'm thinking about a creating a new bug to track what is described here (blocking this one), since this bug is about moving dumpScreen. :jgriffin, do you agree with that plan ?

Yes, I agree with this plan; chmanchester has looked into making reftest use structured logs and it looks non-trivial, and I don't think we need to block this on that. I'd be happy with copy-and-paste for now.

Bug 1091285 - move dumpScreen in a new mozscreenshot package. r=jgriffin This also completely remove build/automationutils.py.

Well, I would like some general feedback/review on this. :) I also pushed a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da3f37a22fae It may be useful to look at the patch in bug 1186551 to understand how that works. Thanks!

Comment on attachment 8640544 [details] MozReview Request: Bug 1091285 - move dumpScreen in a new mozscreenshot package. r=jgriffin https://reviewboard.mozilla.org/r/14329/#review12965 ::: testing/xpcshell/mach_commands.py (Diff revision 1) > - if not os.path.isfile(os.path.join(self.topsrcdir, 'build', 'automationutils.py')): This line actually is inteded to detect whether or not we're running against Thunderbird, and if so, append mozilla/build to sys.path. We should keep this line, but make it conditional on another file, I think.

(In reply to Jonathan Griffin (:jgriffin) from comment #9) > Comment on attachment 8640544 [details] > MozReview Request: Bug 1091285 - move dumpScreen in a new mozscreenshot > package. r=jgriffin > > https://reviewboard.mozilla.org/r/14329/#review12965 > > ::: testing/xpcshell/mach_commands.py > (Diff revision 1) > > - if not os.path.isfile(os.path.join(self.topsrcdir, 'build', 'automationutils.py')): > > This line actually is inteded to detect whether or not we're running against > Thunderbird, and if so, append mozilla/build to sys.path. We should keep > this line, but make it conditional on another file, I think. Let's just do this if mozilla/build actually exists, which it won't for mozilla-central, but will for comm-central.

Fixed and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be0f8d7d6c90 I wait for good results before asking another review.

Comment on attachment 8640544 [details] MozReview Request: Bug 1091285 - move dumpScreen in a new mozscreenshot package. r=jgriffin Bug 1091285 - move dumpScreen in a new mozscreenshot package. r=jgriffin This also completely remove build/automationutils.py.

(In reply to Julien Pagès from comment #12) > This also completely remove build/automationutils.py. Hooray! \o/

Comment on attachment 8640544 [details] MozReview Request: Bug 1091285 - move dumpScreen in a new mozscreenshot package. r=jgriffin https://reviewboard.mozilla.org/r/14329/#review13073 ::: testing/xpcshell/mach_commands.py:81 (Diff revision 2) > - if not os.path.isfile(os.path.join(self.topsrcdir, 'build', 'automationutils.py')): > + src_build_path = os.path.join(self.topsrcdir, 'build') Yes, except: src_build_path = os.path.join(self.topsrcdir, 'mozilla', 'build') since this block is here to distinguish comm-central from mozilla-central, and both have a top-level 'build' directory. Ah yes, it was the moz.build change that was breaking the earlier try run.

Comment on attachment 8640544 [details] MozReview Request: Bug 1091285 - move dumpScreen in a new mozscreenshot package. r=jgriffin Bug 1091285 - move dumpScreen in a new mozscreenshot package. r=jgriffin This also completely remove build/automationutils.py.

Pushed a more complete try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=526a90ef4542

Comment on attachment 8640544 [details] MozReview Request: Bug 1091285 - move dumpScreen in a new mozscreenshot package. r=jgriffin https://reviewboard.mozilla.org/r/14329/#review13107 Ship It!