Closed Bug 1517059 Opened 3 years ago Closed 3 years ago

Tidy up Mozmill/JSBridge resources

Categories

(Thunderbird :: Testing Infrastructure, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 66.0

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

Attachments

(1 file, 1 obsolete file)

In this bug I'm going to lint all the Python in mail/test/mozmill and mail/test/resources, and remove the docs, scripts, and test directories which are unused and seriously out-of-date.
Attached patch 1517059-mozmill-python-1.diff (obsolete) — Splinter Review
I know you love linting stuff aceman – how about some Python for a change?

Buried in this patch, I've moved the profile directory used for mozmill tests out of the objdir and into the temporary directory (whatever tempfile.gettempdir() returns). This has an advantage in that by setting $TEMP you can move the profile dir, to /dev/shm for example, which avoids writing to disk at all.
Attachment #9033827 - Flags: review?(acelists)
I'm not going to post a patch that only deletes 76 files, because that's ridiculous. So this ni? is really an r? on that operation. The files are:

mail/test/resources/mozmill/docs/*
mail/test/resources/mozmill/scripts/*
mail/test/resources/mozmill/test/*
Flags: needinfo?(philipp)
Luckly this didn't get buried in my ni queue :D I think it would be best for Jörg to decide on this, I believe he has been more involved with mozmill than me. From a brief look I didn't see anything of value.
Flags: needinfo?(philipp) → needinfo?(jorgk)
I guess it's OK as long as you don't delete mail/test/resources/mozmill/scripts/mozmill ;-) - Aceman is reviewing here, so we can confirm.
Flags: needinfo?(jorgk) → needinfo?(acelists)
That's not a real file.
Grrr, what happened there, cut&paste error :-( - I meant: mail/test/resources/mozmill/mozmill ;-)
(In reply to Geoff Lankow (:darktrojan) from comment #1)
> Created attachment 9033827 [details] [diff] [review]
> 1517059-mozmill-python-1.diff
> 
> I know you love linting stuff aceman – how about some Python for a change?

How did you create the patch? Is there a tool for python linting? Or is it manually decided cleanup?

> Buried in this patch, I've moved the profile directory used for mozmill
> tests out of the objdir and into the temporary directory (whatever
> tempfile.gettempdir() returns).

That sounds useful, I think xpcshell is already in temp dir so mozmill can follow.
What does tempfile.gettempdir() return? Is it e.g. /tmp on Linux, or does it append (create) some random subdirectory (thinking of mktemp() behaviour)?

> This has an advantage in that by setting
> $TEMP you can move the profile dir, to /dev/shm for example, which avoids
> writing to disk at all.

Yes, nice.
Flags: needinfo?(acelists)
Comment on attachment 9033827 [details] [diff] [review]
1517059-mozmill-python-1.diff

Review of attachment 9033827 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/test/mozmill/runtest.py
@@ +188,5 @@
>              pass
>          else:
>              self.set_preferences(self.menubar_preferences)
>  
> +        if (wrapper is not None and hasattr(wrapper, "NO_ACCOUNTS") and wrapper.NO_ACCOUNTS):

Why this change? Do we want long lines in python?

@@ +533,5 @@
>      if not parent_dir:
>          return
>  
>      from base64 import b64decode
>      from os import fdopen

'os' is also already imported above.
Attachment #9033827 - Flags: feedback+
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0883f1e35c11
Tidy up Mozmill/JSBridge resources: remove mail/test/resources/mozmill/scripts/. r=darktrojan
Sorry, no patch to land, so removed some of the stuff mentioned in comment #2. I was going to kill it all, but I asked myself whether docs had any value.
(In reply to :aceman from comment #7)
> How did you create the patch? Is there a tool for python linting? Or is it
> manually decided cleanup?

There's a standard (flake8) which m-c code is checked for automatically but not c-c code. I've used a plugin for my editor which might be slightly different but close enough.

> > Buried in this patch, I've moved the profile directory used for mozmill
> > tests out of the objdir and into the temporary directory (whatever
> > tempfile.gettempdir() returns).
> 
> That sounds useful, I think xpcshell is already in temp dir so mozmill can
> follow.
> What does tempfile.gettempdir() return? Is it e.g. /tmp on Linux, or does it
> append (create) some random subdirectory (thinking of mktemp() behaviour)?

It's just /tmp, so our profile directory becomes /tmp/mozmillprofile. xpcshell does this already (unless you run more than one test, annoyingly) and mochitest does this already.

(In reply to :aceman from comment #8)
> > +        if (wrapper is not None and hasattr(wrapper, "NO_ACCOUNTS") and wrapper.NO_ACCOUNTS):
> 
> Why this change? Do we want long lines in python?

I'll fix it and the other line longer than 100 chars, just for you. :)

> >      from base64 import b64decode
> >      from os import fdopen
> 
> 'os' is also already imported above.

Well spotted. These imports shouldn't be here but someone got lazy (me). Moving to the top.
Attachment #9034043 - Flags: review?(acelists)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/246f3ec47ccd
Tidy up Mozmill/JSBridge resources: remove mail/test/resources/mozmill/test/. r=darktrojan
(In reply to Geoff Lankow (:darktrojan) from comment #11)
> (In reply to :aceman from comment #7)
> > How did you create the patch? Is there a tool for python linting? Or is it
> > manually decided cleanup?
> 
> There's a standard (flake8) which m-c code is checked for automatically but
> not c-c code. I've used a plugin for my editor which might be slightly
> different but close enough.

I just got a flake8 error on a Try build. So apparently it IS checked … sometimes … ?
Attachment #9033827 - Attachment is obsolete: true
Attachment #9033827 - Flags: review?(acelists)
Comment on attachment 9034043 [details] [diff] [review]
1517059-mozmill-python-2.diff

Review of attachment 9034043 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.
Attachment #9034043 - Flags: review?(acelists) → review+
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/1ae2793b99f0
Tidy up Mozmill/JSBridge resources: remove mail/test/resources/mozmill/docs/. r=me
Keywords: leave-open
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/f2f995d7cf9a
Tidy up Mozmill/JSBridge resources; r=aceman
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 66.0
You need to log in before you can comment on or make changes to this bug.