bpo-36746: Create test for fcntl.lockf() by nanjekyejoannah · Pull Request #12999 · python/cpython

nanjekyejoannah · 2019-04-28T18:34:44Z

I have created a test for fcntl.lockf().

https://bugs.python.org/issue36746

nanjekyejoannah · 2019-05-04T12:55:07Z

I have made the requested changes; please review again.

bedevere-bot · 2019-05-04T12:55:10Z

Thanks for making the requested changes!

: please review the changes made to this pull request.

tirkarthi · 2019-05-05T12:33:26Z

+        if pid == 0:
+            rval = 2
+            try:
+                fcntl.lockf(open(self.f.name, self.f.mode), fcntl.LOCK_EX | fcntl.LOCK_NB)


This line causes a ResourceWarning since the open file handle is not closed. Perhaps assign it to a variable and make sure it's closed in the test.

➜ cpython git:(master) ✗ ./python.exe -m unittest -v test.test_fcntl.TestFcntl.test_lockf struct.pack: b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00' test_lockf (test.test_fcntl.TestFcntl) ... /Users/karthikeyansingaravelan/stuff/python/cpython/Lib/test/test_fcntl.py:154: ResourceWarning: unclosed file <_io.BufferedRandom name='@test_27262_tmp'> fcntl.lockf(open(self.f.name, self.f.mode), fcntl.LOCK_EX | fcntl.LOCK_NB) ResourceWarning: Enable tracemalloc to get the object allocation traceback ok ---------------------------------------------------------------------- Ran 1 test in 0.008s OK

tirkarthi · 2019-05-05T12:39:20Z

+
+        fcntl.lockf(self.f, fcntl.LOCK_UN)
+
+        self.f.close()


I feel this is redundant since tearDown takes care of closing this. I am not sure some of the other tests close it too. Is it intentional?

tirkarthi · 2019-05-05T12:43:48Z

+            finally:
+                os._exit(rval)
+
+        assert pid > 0


self.assertGreater(pid, 0) could be useful in error messages where pid value is shown for debugging assertion failure.

berkerpeksag

I think your approach looks pretty good.

Have you considered using subprocess.run() instead of os.fork()? I think the former might be a better alternative especially if you are planning to increase test coverage of fcntl since it would make code reusing easier.

Also, while you are editing this file, could you please replace the following snippet

def test_main():
    run_unittest(TestFcntl)

if __name__ == '__main__':
    test_main()

with

if __name__ == '__main__':
    unittest.main()

bedevere-bot · 2019-05-05T12:53:41Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

nanjekyejoannah · 2019-06-06T18:43:47Z

@berkerpeksag I have made some changes to use subprocess.run(). I have not used subprocess.run() before so your thoughts on this change are highly appreciated.

nanjekyejoannah · 2019-06-06T21:01:48Z

I have made the requested changes; please review again

bedevere-bot · 2019-06-06T21:01:51Z

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

vstinner · 2019-08-20T15:22:21Z

+
+        fcntl.lockf(self.f, fcntl.LOCK_EX | fcntl.LOCK_NB)
+
+        code = textwrap.dedent('''


fcntl module should be imported, no? Moreover, you should pass self.f.name into the string. For example, use an f-string and replace self.f.name with {TESTFN!r}. Replace self.f.mode with 'wb+'.

vstinner · 2019-08-20T15:22:53Z

+            else:
+                os.EX_OK
+            finally:
+                os._exit()


os._exit() requires an argument.

vstinner · 2019-08-20T15:23:13Z

+            except OSError as e:
+                if e.errno not in (errno.EACCES, errno.EAGAIN):
+                    raise
+                os.EX_OSERR


what's the purpose of this statement?

vstinner · 2019-08-20T15:24:27Z

+        fcntl.lockf(self.f, fcntl.LOCK_UN)
+
+    def test_lockf_errors(self):
+


remove this empty line

bedevere-bot · 2019-08-20T15:25:53Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2020-01-15T21:37:09Z

PR #17010 added tests for fcntl.lockf(). I close this PR. Thanks @nanjekyejoannah anyway!

create test for fcntl.lockf

c3b786f

the-knights-who-say-ni added the CLA signed label Apr 28, 2019

bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Apr 28, 2019

nanjekyejoannah changed the title ~~bpo-36746: Create test for fcntl.lockf~~ bpo-36746: Create test for fcntl.lockf() Apr 28, 2019

📜🤖 Added by blurb_it.

b9b199b

berkerpeksag added the skip news label Apr 28, 2019

berkerpeksag reviewed Apr 28, 2019

View reviewed changes

Comment thread Misc/NEWS.d/next/Tests/2019-04-28-18-36-54.bpo-36746.aqYNtS.rst Outdated

remove news entry for trivial fix.

2280d1c

bedevere-bot added awaiting change review and removed awaiting review labels May 4, 2019

tirkarthi reviewed May 5, 2019

View reviewed changes

berkerpeksag requested changes May 5, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels May 5, 2019

Fix space

bb26eb8

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 6, 2019

vstinner requested changes Aug 20, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Aug 20, 2019

vstinner closed this Jan 15, 2020


		fcntl.lockf(self.f, fcntl.LOCK_EX \| fcntl.LOCK_NB)

		code = textwrap.dedent('''

		fcntl.lockf(self.f, fcntl.LOCK_UN)

		def test_lockf_errors(self):

Uh oh!

Uh oh!

Conversation

nanjekyejoannah commented Apr 28, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nanjekyejoannah commented May 4, 2019

Uh oh!

bedevere-bot commented May 4, 2019

Uh oh!

tirkarthi May 5, 2019

Choose a reason for hiding this comment

Uh oh!

tirkarthi May 5, 2019

Choose a reason for hiding this comment

Uh oh!

tirkarthi May 5, 2019

Choose a reason for hiding this comment

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented May 5, 2019

Uh oh!

nanjekyejoannah commented Jun 6, 2019

Uh oh!

nanjekyejoannah commented Jun 6, 2019

Uh oh!

bedevere-bot commented Jun 6, 2019

Uh oh!

vstinner Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 20, 2019

Uh oh!

vstinner commented Jan 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nanjekyejoannah commented Apr 28, 2019 •

edited by bedevere-bot

Loading