1517059 - Tidy up Mozmill/JSBridge resources

Status: RESOLVED FIXED

Description

[Geoff Lankow (:darktrojan)]

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.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1517059-mozmill-python-1.diff

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.

Comment 2

[Geoff Lankow (:darktrojan)]

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/*

Comment 3

[Philipp Kewisch [:Fallen] [:📆]]

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.

Comment 4

[Jorg K (GMT+2)]

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.

Comment 5

[Geoff Lankow (:darktrojan)]

That's not a real file.

Comment 6

[Jorg K (GMT+2)]

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

Comment 7

[:aceman]

(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.

Comment 8

[:aceman]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0883f1e35c11
Tidy up Mozmill/JSBridge resources: remove mail/test/resources/mozmill/scripts/. r=darktrojan

Comment 10

[Jorg K (GMT+2)]

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.

Comment 11

[Geoff Lankow (:darktrojan)]

(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.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1517059-mozmill-python-2.diff

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/246f3ec47ccd
Tidy up Mozmill/JSBridge resources: remove mail/test/resources/mozmill/test/. r=darktrojan

Comment 14

[Geoff Lankow (: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 … ?

Comment 15

[:aceman]

Comment on attachment 9034043 [details] [diff] [review]
1517059-mozmill-python-2.diff

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

Thanks.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1ae2793b99f0
Tidy up Mozmill/JSBridge resources: remove mail/test/resources/mozmill/docs/. r=me

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f2f995d7cf9a
Tidy up Mozmill/JSBridge resources; r=aceman