python/mozbuild/mozbuild/test/test_preprocessor.py doesn't clean up after itself

RESOLVED FIXED in mozilla29

Status

Firefox Build System
General
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla29

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
python/mozbuild/mozbuild/test/test_preprocessor.py is creating a lot of temporary files and isn't cleaning up after itself.

It appears to deposit things in cwd, which is a problem when you are running tests via |mach python-tests| or similar mechanism.
(Assignee)

Comment 1

5 years ago
Created attachment 8350837 [details] [diff] [review]
Don't leave temporary files when executing test_preprocessor.py

Between the reindentation and making the tests use modern techniques and
cleaning up the formatting, I pretty much changed every line. Sorry for
the impossible review.
Attachment #8350837 - Flags: review?(mshal)
(Assignee)

Updated

5 years ago
Assignee: nobody → gps
Status: NEW → ASSIGNED
Comment on attachment 8350837 [details] [diff] [review]
Don't leave temporary files when executing test_preprocessor.py

>diff --git a/python/mozbuild/mozbuild/test/test_preprocessor.py b/python/mozbuild/mozbuild/test/test_preprocessor.py
>--- a/python/mozbuild/mozbuild/test/test_preprocessor.py
>+++ b/python/mozbuild/mozbuild/test/test_preprocessor.py
>@@ -1,577 +1,551 @@
> import unittest
> 
> from StringIO import StringIO
> import os
>-import sys
>-import os.path
>+import shutil
>+
>+from tempfile import mkdtemp
>+
> from mozunit import main, MockedOpen
> 
> from mozbuild.preprocessor import Preprocessor

For my own python-y education, is there a reason why some imports are grouped and why others are spaced apart?

>+    def do_include_compare(self, content_lines, expected_lines):
>+        content = '%s\n' % '\n'.join(content_lines)
>+        expected = '%s\n'.rstrip() % '\n'.join(expected_lines)

I think you can change '%s\n' to just '%s' in both the 'content' & 'expected' lines since they get rstrip()'d anyway.

>+        with MockedOpen({'dummy': content}):
>+            self.pp.do_include('dummy')
>+            self.assertEqual(self.pp.out.getvalue().rstrip(), expected)

Maybe rstrip() both self.pp.out.getvalue() and 'expected' here, or rstrip() them both in the 'content = ' and 'expected = ' lines. It's a bit odd to follow when they are stripped in different places.

The tests all look good!
Attachment #8350837 - Flags: review?(mshal) → review+
(Assignee)

Comment 3

5 years ago
(In reply to Michael Shal [:mshal] from comment #2)
> >diff --git a/python/mozbuild/mozbuild/test/test_preprocessor.py b/python/mozbuild/mozbuild/test/test_preprocessor.py
> >--- a/python/mozbuild/mozbuild/test/test_preprocessor.py
> >+++ b/python/mozbuild/mozbuild/test/test_preprocessor.py
> >@@ -1,577 +1,551 @@
> > import unittest
> > 
> > from StringIO import StringIO
> > import os
> >-import sys
> >-import os.path
> >+import shutil
> >+
> >+from tempfile import mkdtemp
> >+
> > from mozunit import main, MockedOpen
> > 
> > from mozbuild.preprocessor import Preprocessor
> 
> For my own python-y education, is there a reason why some imports are
> grouped and why others are spaced apart?

There is little method to this madness. I try to put things in alphabetical order with newline-delimited groupings of system, 3rd party, local packages. I often fail to follow my own convention.
 
> >+    def do_include_compare(self, content_lines, expected_lines):
> >+        content = '%s\n' % '\n'.join(content_lines)
> >+        expected = '%s\n'.rstrip() % '\n'.join(expected_lines)
> 
> I think you can change '%s\n' to just '%s' in both the 'content' &
> 'expected' lines since they get rstrip()'d anyway.

You are correct.

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/3b169432bbf7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 1313259

Updated

4 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.