From 2eaa12d084cb1c8756a4987f4b05fe1ab81168a9 Mon Sep 17 00:00:00 2001 From: Jeremy Cline Date: Sep 29 2016 12:49:53 +0000 Subject: Use context managers to ensure files are closed There were several places where context managers were not being used and files were potentially not being closed. I started investigating this since the unit tests are not passing if the code directory is mounted with SSHFS. The test setUp failed to remove directories with ``shutil.rmtree`` with "directory not empty". Looking at the directories, there were .fuse_hidden* files so I assumed the file handles were being leaked. The tests are still failing so I've either missed something in my search or that was not the problem, but I thought this would be a good change in any case. Signed-off-by: Jeremy Cline --- diff --git a/pagure/hooks/files/git_multimail.py b/pagure/hooks/files/git_multimail.py index 88930c3..03b10bb 100755 --- a/pagure/hooks/files/git_multimail.py +++ b/pagure/hooks/files/git_multimail.py @@ -2091,8 +2091,8 @@ class ProjectdescEnvironmentMixin(Environment): git_dir = get_git_dir() try: - projectdesc = open( - os.path.join(git_dir, 'description')).readline().strip() + with open(os.path.join(git_dir, 'description')) as f: + projectdesc = f.readline().strip() if projectdesc and not projectdesc.startswith( 'Unnamed repository'): return projectdesc diff --git a/setup.py b/setup.py index efcf7ab..e11d55a 100644 --- a/setup.py +++ b/setup.py @@ -33,12 +33,12 @@ def get_requirements(requirements_file='requirements.txt'): :return type: list """ - lines = open(requirements_file).readlines() - return [ - line.rstrip().split('#')[0] - for line in lines - if not line.startswith('#') - ] + with open(requirements_file) as f: + return [ + line.rstrip().split('#')[0] + for line in f.readlines() + if not line.startswith('#') + ] setup( diff --git a/tests/test_pagure_flask_ui_issues.py b/tests/test_pagure_flask_ui_issues.py index a261241..e68de08 100644 --- a/tests/test_pagure_flask_ui_issues.py +++ b/tests/test_pagure_flask_ui_issues.py @@ -179,20 +179,19 @@ class PagureFlaskIssuestests(tests.Modeltests): csrf_token = output.data.split( 'name="csrf_token" type="hidden" value="')[1].split('">')[0] - stream = open(os.path.join(tests.HERE, 'placebo.png'), 'r') - data = { - 'title': 'Test issue', - 'issue_content': 'We really should improve on this issue\n' - '', - 'status': 'Open', - 'filestream': stream, - 'enctype': 'multipart/form-data', - 'csrf_token': csrf_token, - } - - output = self.app.post( - '/test/new_issue', data=data, follow_redirects=True) - stream.close() + with open(os.path.join(tests.HERE, 'placebo.png'), 'r') as stream: + data = { + 'title': 'Test issue', + 'issue_content': 'We really should improve on this issue\n' + '', + 'status': 'Open', + 'filestream': stream, + 'enctype': 'multipart/form-data', + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/test/new_issue', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( @@ -211,18 +210,18 @@ class PagureFlaskIssuestests(tests.Modeltests): user.username = 'pingou' with tests.user_set(pagure.APP, user): - stream = open(os.path.join(tests.HERE, 'placebo.png'), 'r') - data = { - 'title': 'Test issue', - 'issue_content': 'We really should improve on this issue', - 'status': 'Open', - 'filestream': stream, - 'enctype': 'multipart/form-data', - 'csrf_token': csrf_token, - } - - output = self.app.post( - '/test/new_issue', data=data, follow_redirects=True) + with open(os.path.join(tests.HERE, 'placebo.png'), 'r') as stream: + data = { + 'title': 'Test issue', + 'issue_content': 'We really should improve on this issue', + 'status': 'Open', + 'filestream': stream, + 'enctype': 'multipart/form-data', + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/test/new_issue', data=data, follow_redirects=True) self.assertEqual(output.status_code, 404) @patch('pagure.lib.git.update_git') @@ -1130,16 +1129,15 @@ class PagureFlaskIssuestests(tests.Modeltests): with tempfile.NamedTemporaryFile() as eicarfile: eicarfile.write(pyclamd.ClamdUnixSocket().EICAR()) eicarfile.flush() - stream = open(eicarfile.name, 'rb') - data = { - 'csrf_token': csrf_token, - 'filestream': stream, - 'enctype': 'multipart/form-data', - } - output = self.app.post( - '/test/issue/1/upload', data=data, follow_redirects=True) + with open(eicarfile.name, 'rb') as stream: + data = { + 'csrf_token': csrf_token, + 'filestream': stream, + 'enctype': 'multipart/form-data', + } + output = self.app.post( + '/test/issue/1/upload', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) - stream.close() json_data = json.loads(output.data) exp = { 'output': 'notok', @@ -1147,15 +1145,14 @@ class PagureFlaskIssuestests(tests.Modeltests): self.assertDictEqual(json_data, exp) # Attach a file to a ticket - stream = open(os.path.join(tests.HERE, 'placebo.png'), 'rb') - data = { - 'csrf_token': csrf_token, - 'filestream': stream, - 'enctype': 'multipart/form-data', - } - output = self.app.post( - '/test/issue/1/upload', data=data, follow_redirects=True) - stream.close() + with open(os.path.join(tests.HERE, 'placebo.png'), 'rb') as stream: + data = { + 'csrf_token': csrf_token, + 'filestream': stream, + 'enctype': 'multipart/form-data', + } + output = self.app.post( + '/test/issue/1/upload', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) json_data = json.loads(output.data) diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py index 7beab23..a442f6c 100644 --- a/tests/test_pagure_flask_ui_repo.py +++ b/tests/test_pagure_flask_ui_repo.py @@ -2886,8 +2886,9 @@ index 0000000..fb7093d img = os.path.join(tests.HERE, 'placebo.png') # Missing CSRF Token - data = {'filestream': open(img)} - output = self.app.post('/test/upload/', data=data) + with open(img, mode='rb') as stream: + data = {'filestream': stream} + output = self.app.post('/test/upload/', data=data) self.assertEqual(output.status_code, 200) self.assertIn('

Upload a new release

', output.data) @@ -2895,9 +2896,10 @@ index 0000000..fb7093d 'name="csrf_token" type="hidden" value="')[1].split('">')[0] # Upload successful - data = {'filestream': open(img), 'csrf_token': csrf_token} - output = self.app.post( - '/test/upload/', data=data, follow_redirects=True) + with open(img, mode='rb') as stream: + data = {'filestream': stream, 'csrf_token': csrf_token} + output = self.app.post( + '/test/upload/', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( '\n File', output.data)