bpo-36746: Create test for fcntl.lockf() #12999

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

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

https://bugs.python.org/issue36746

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

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

Lib/test/test_fcntl.py

+        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

Lib/test/test_fcntl.py

+
+        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

Lib/test/test_fcntl.py

+            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()

Lib/test/test_fcntl.py

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

Lib/test/test_fcntl.py

+
+        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

Lib/test/test_fcntl.py

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


os._exit() requires an argument.

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

Lib/test/test_fcntl.py

+            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

Lib/test/test_fcntl.py

+        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

Misc/NEWS.d/next/Tests/2019-04-28-18-36-54.bpo-36746.aqYNtS.rst Outdated Show resolved Hide resolved

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!

bpo-36746: Create test for fcntl.lockf() #12999

bpo-36746: Create test for fcntl.lockf() #12999

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!

Uh oh!

nanjekyejoannah commented Apr 28, 2019 •

edited by bedevere-bot

Loading