Tidy up Mozmill/JSBridge resources

RESOLVED FIXED in Thunderbird 66.0

Status

enhancement
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
Thunderbird 66.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

5 months ago
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.
Assignee

Comment 1

5 months ago
Posted 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)
Assignee

Comment 2

5 months ago
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)

Comment 4

5 months ago
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)
Assignee

Comment 5

5 months ago
That's not a real file.

Comment 6

5 months ago
Grrr, what happened there, cut&paste error :-( - I meant: mail/test/resources/mozmill/mozmill ;-)

Comment 7

5 months ago
(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 8

5 months ago
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+

Updated

5 months ago
Keywords: leave-open

Comment 9

5 months ago
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

Comment 10

5 months ago
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.
Assignee

Comment 11

5 months ago
(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.
Assignee

Comment 12

5 months ago
Attachment #9034043 - Flags: review?(acelists)

Comment 13

5 months ago
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
Assignee

Comment 14

5 months ago
(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 … ?
Assignee

Updated

5 months ago
Attachment #9033827 - Attachment is obsolete: true
Attachment #9033827 - Flags: review?(acelists)

Comment 15

5 months ago
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+

Comment 16

5 months ago
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
Assignee

Updated

4 months ago
Keywords: leave-open

Comment 17

4 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/f2f995d7cf9a
Tidy up Mozmill/JSBridge resources; r=aceman
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Assignee

Updated

4 months ago
Target Milestone: --- → Thunderbird 66.0
You need to log in before you can comment on or make changes to this bug.