Closed Bug 1091285 Opened 5 years ago Closed 4 years ago

Move dumpScreen out of automationutils

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jgriffin, Assigned: parkouss)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
Depends on: 1087474
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 ?
Flags: needinfo?(jgriffin)
Depends on: 1186551
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.
Flags: needinfo?(jgriffin)
Bug 1091285 - move dumpScreen in a new mozscreenshot package. r=jgriffin

This also completely remove build/automationutils.py.
Attachment #8640544 - Flags: review?(jgriffin)
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.
Attachment #8640544 - Flags: review?(jgriffin)
(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.
Attachment #8640544 - Flags: review?(jgriffin)
(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.
Attachment #8640544 - Flags: review?(jgriffin)
Attachment #8640544 - Flags: review?(jgriffin)
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.
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!
Attachment #8640544 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/mozilla-central/rev/c12f87f25000
Assignee: nobody → j.parkouss
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.