bpo-25872: Add unit tests for linecache and threading #25913

uniocto · 2021-05-05T09:39:12Z

https://bugs.python.org/issue25872

the-knights-who-say-ni · 2021-05-05T09:39:16Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@uniocto

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

iritkatriel

Thank you for your first CPython PR! I hope this will be followed by many more.

See a couple of comments below.

iritkatriel · 2021-05-07T09:05:17Z

Lib/test/test_threading.py

@@ -1338,6 +1338,25 @@ def run(self):
        # explicitly break the reference cycle to not leak a dangling thread
        thread.exc = None

+    def test_multithread_modify_file_noerror(self):
+        import traceback


I believe PEP-8 asks for imports to be at module scope

Hi @iritkatriel
Thank you for your feedback and I'll rewrite it to support pep8.
Incidentally, pep-8 point out where I have not changed this time.
Should I also refactor the following?

> pep8 Lib/test/test_threading.py Lib/test/test_threading.py:45:5: E301 expected 1 blank line, found 0 Lib/test/test_threading.py:47:5: E301 expected 1 blank line, found 0 Lib/test/test_threading.py:49:5: E301 expected 1 blank line, found 0 Lib/test/test_threading.py:52:1: E302 expected 2 blank lines, found 1 Lib/test/test_threading.py:145:41: E228 missing whitespace around modulo operator Lib/test/test_threading.py:152:80: E501 line too long (88 > 79 characters) Lib/test/test_threading.py:171:9: E301 expected 1 blank line, found 0 Lib/test/test_threading.py:223:9: E265 block comment should start with '# ' Lib/test/test_threading.py:248:80: E501 line too long (80 > 79 characters) Lib/test/test_threading.py:259:40: E261 at least two spaces before inline comment Lib/test/test_threading.py:285:24: E261 at least two spaces before inline comment Lib/test/test_threading.py:307:36: E261 at least two spaces before inline comment Lib/test/test_threading.py:408:13: E128 continuation line under-indented for visual indent Lib/test/test_threading.py:424:21: E128 continuation line under-indented for visual indent Lib/test/test_threading.py:436:69: E231 missing whitespace after ':' Lib/test/test_threading.py:450:26: E128 continuation line under-indented for visual indent Lib/test/test_threading.py:458:26: E128 continuation line under-indented for visual indent Lib/test/test_threading.py:716:9: E301 expected 1 blank line, found 0 Lib/test/test_threading.py:733:80: E501 line too long (82 > 79 characters) Lib/test/test_threading.py:751:9: E301 expected 1 blank line, found 0 Lib/test/test_threading.py:769:44: E261 at least two spaces before inline comment Lib/test/test_threading.py:838:40: E231 missing whitespace after ',' Lib/test/test_threading.py:900:13: E301 expected 1 blank line, found 0 Lib/test/test_threading.py:902:13: E301 expected 1 blank line, found 0 Lib/test/test_threading.py:913:1: E303 too many blank lines (3) Lib/test/test_threading.py:969:80: E501 line too long (82 > 79 characters) Lib/test/test_threading.py:1009:80: E501 line too long (87 > 79 characters) Lib/test/test_threading.py:1066:47: E203 whitespace before ':' Lib/test/test_threading.py:1193:61: E703 statement ends with a semicolon Lib/test/test_threading.py:1237:80: E501 line too long (81 > 79 characters) Lib/test/test_threading.py:1399:80: E501 line too long (80 > 79 characters) Lib/test/test_threading.py:1449:14: E127 continuation line over-indented for visual indent Lib/test/test_threading.py:1450:14: E127 continuation line over-indented for visual indent Lib/test/test_threading.py:1509:1: E302 expected 2 blank lines, found 1 Lib/test/test_threading.py:1512:1: E302 expected 2 blank lines, found 1 Lib/test/test_threading.py:1515:1: E302 expected 2 blank lines, found 1 Lib/test/test_threading.py:1519:1: E302 expected 2 blank lines, found 1 Lib/test/test_threading.py:1522:1: E302 expected 2 blank lines, found 1 Lib/test/test_threading.py:1526:1: E302 expected 2 blank lines, found 1 Lib/test/test_threading.py:1529:1: E302 expected 2 blank lines, found 1 Lib/test/test_threading.py:1532:1: E302 expected 2 blank lines, found 1 Lib/test/test_threading.py:1535:1: E302 expected 2 blank lines, found 1 Lib/test/test_threading.py:1651:17: E128 continuation line under-indented for visual indent > pep8 Lib/test/test_linecache.py Lib/test/test_linecache.py:229:9: E301 expected 1 blank line, found 0

iritkatriel · 2021-05-07T17:17:32Z

Lib/test/test_linecache.py

+        self.assertTrue(_)
+        self.assertEqual(1, len(linecache.cache.keys()))
+
+        with support.swap_attr(os, 'stat', raise_oserror):


The fact that os.stat is used in checkcache is an implementation detail, not a part of that's function's external API, so I don't think the unit test should depend on that.

I would write this as a black-box test. And also test that the cache clearing is selective. Note that there are two cases in checkcache() where the cache is popped: OSError from stat, or size/timestamp don't match.

So, create three files f1, f2, f3 and load them into the cache. Then delete f1, modify f2 and call checkcache. Ensure that the cache entries for f1 and f2 were removed from the cache but the entries for f3 are still there.

Thank you for your feedback.
As you said, other tests are written in such a way that they do not depend on the internal implementation.
I will try to write them in a black box as you suggested, so I would appreciate it if you could take look again !!

I write a black box test, and would be happy if you could check it out.

def test_oserror(self): def _oserror_helper(): linecache.clearcache() be_deleted_file = os_helper.TESTFN + '.1' be_modified_file = os_helper.TESTFN + '.2' unchange_file = os_helper.TESTFN + '.3' self.addCleanup(os_helper.unlink, be_deleted_file) self.addCleanup(os_helper.unlink, be_modified_file) self.addCleanup(os_helper.unlink, unchange_file) with open(be_deleted_file, 'w', encoding='utf-8') as source: source.write('print("will be deleted")') with open(be_modified_file, 'w', encoding='utf-8') as source: source.write('print("will be modified")') with open(unchange_file, 'w', encoding='utf-8') as source: source.write('print("unchange")') _ = linecache.getlines(be_deleted_file) _ = linecache.getlines(be_modified_file) _ = linecache.getlines(unchange_file) self.assertEqual(3, len(linecache.cache.keys())) os.remove(be_deleted_file) with open(be_modified_file, 'w', encoding='utf-8') as source: source.write('print("was modified")') return (be_deleted_file, be_modified_file, unchange_file) deleted_file, modified_file, unchange_file = _oserror_helper() _ = linecache.checkcache(deleted_file) self.assertEqual(2, len(linecache.cache.keys())) _ = linecache.checkcache(modified_file) self.assertEqual(1, len(linecache.cache.keys())) _ = linecache.checkcache(unchange_file) self.assertEqual(1, len(linecache.cache.keys())) deleted_file, modified_file, unchange_file = _oserror_helper() _ = linecache.updatecache(deleted_file) self.assertEqual(2, len(linecache.cache.keys())) _ = linecache.updatecache(modified_file) self.assertEqual(2, len(linecache.cache.keys())) _ = linecache.updatecache(unchange_file) self.assertEqual(2, len(linecache.cache.keys()))

iritkatriel · 2021-05-07T17:19:34Z

Lib/test/test_linecache.py

+        _ = linecache.getlines(source_name)
+        self.assertEqual(1, len(linecache.cache.keys()))
+
+        with support.swap_attr(os, 'stat', raise_oserror):


As above - delete/modify the file instead of mocking os.stat.

As shown above, I write delete/modify test.

iritkatriel · 2021-05-08T11:57:29Z

Lib/test/test_linecache.py

@@ -238,6 +238,48 @@ def raise_memoryerror(*args, **kwargs):
        self.assertEqual(lines3, [])
        self.assertEqual(linecache.getlines(FILENAME), lines)

+    def test_oserror(self):


This is better. Now let's see how it should be organized. I have several comments:

test_oserror is a a good name for a test that tests oserror, but this test is testing something else.

It's better to split tests so that each test function is testing one thing (and its name says what).

see https://docs.python.org/3/library/linecache.html -- updatecache is not part of the documented API, so I'm not sure it needs to be tested directly.

There is already a test_checkcache function higher in this file, which covers some of what this test does but not all of it. It should be refactored into the new test rather than just adding a new one, so that all related tests are together.

The checkcache() with no parameters API needs to be tested as well.

You could create a new test class , say class LineCacheInvalidationTests(unittest.TestCase):
Put the initialization code (what you called _oserror_helper) in this class's setUp.
Then add test functions for each test case.

iritkatriel · 2021-05-08T13:10:32Z

Lib/test/test_threading.py

+            for t in threads:
+                t.start()
+            for t in threads:
+                t.join()


Is there a reason why you don't do

for t in threads: t.start() t.join()

?

Nothing, sorry.
So I fixed it to your feedback.

iritkatriel · 2021-05-08T13:15:07Z

Lib/test/test_threading.py

+            threading.Thread(target=modify_file)
+            for i in range(100)
+        ]
+        try:


Which exceptions are you trying to ignore here?

Nothing, sorry. I removed try.

iritkatriel · 2021-05-09T09:48:59Z

Lib/test/test_linecache.py

+    def test_checkcache_with_no_parameters(self):
+        self.assertEqual(3, len(linecache.cache.keys()))
+        linecache.checkcache()
+        self.assertTrue([self.unchange_file] == list(linecache.cache.keys()))


assertEqual

iritkatriel · 2021-05-09T09:52:22Z

Lib/test/test_linecache.py

+        self.assertEqual(3, len(linecache.cache.keys()))
+        linecache.checkcache(self.deleted_file)
+        self.assertTrue(2 == len(linecache.cache.keys()) and
+                        self.deleted_file not in linecache.cache.keys())


Break this up into assertEqual and assertNotIn. See the assert methods here: https://docs.python.org/3/library/unittest.html

That way we get better error messages in the output when the assertion fails.

iritkatriel

This is a new thing I wanted to show you - I made a couple of suggested edits. You can click to accept them (and then you don't need to make a new commit). They will be committed, and then you need to pull the changes into your local repo so that you are in sync ("git pull" should do it).

Lib/test/test_linecache.py

Lib/test/test_threading.py

Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>

Lib/test/test_threading.py

iritkatriel · 2021-05-17T14:19:33Z

The rebase doesn't look right - the PR should not be showing so many changed files.

bedevere-bot · 2021-05-17T15:54:32Z

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit ac550ef 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

miss-islington · 2021-05-18T08:56:55Z

Thanks @uniocto for the PR, and @iritkatriel for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

miss-islington · 2021-05-18T08:57:10Z

Sorry, @uniocto and @iritkatriel, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 115dea9e2602b96b63390f00cc880e90c433efa2 3.9

bedevere-bot · 2021-05-18T08:57:13Z

GH-26208 is a backport of this pull request to the 3.10 branch.

(cherry picked from commit 115dea9) Co-authored-by: uniocto <serit142sa33go@gmail.com>

(cherry picked from commit 115dea9)

bedevere-bot · 2021-05-18T10:39:29Z

GH-26211 is a backport of this pull request to the 3.9 branch.

…H-25913) (cherry picked from commit 115dea9) Co-authored-by: uniocto <serit142sa33go@gmail.com>

… (GH-26212) (cherry picked from commit 115dea9) Co-authored-by: uniocto <serit142sa33go@gmail.com>

…26211) (cherry picked from commit 115dea9) Co-authored-by: uniocto <serit142sa33go@gmail.com>

uniocto added 6 commits May 5, 2021 15:33

bpo-25872: add test_oserror

5143e74

refactor linecache

fc1f92d

add test_multithread_modify_file()

fc4df5d

refactor test_threading

a977922

refactor test_multithread_modify_file_noerror()

759de2b

refactor raise_oserror()

5666a1d

bedevere-bot added the awaiting review label May 5, 2021

the-knights-who-say-ni added the CLA not signed label May 5, 2021

bedevere-bot added the tests Tests in the Lib/test dir label May 5, 2021

the-knights-who-say-ni added CLA signed and removed CLA not signed labels May 6, 2021

iritkatriel reviewed May 7, 2021

View reviewed changes

Change black box tests and refactoring

4513859

uniocto force-pushed the fix-issue-25872 branch from 213db0c to 5666a1d Compare May 8, 2021 06:33

iritkatriel reviewed May 8, 2021

View reviewed changes

uniocto added 2 commits May 9, 2021 11:16

refactor LineCacheInvalidationTests

8376d21

refactor test_multithread_modify_file_noerror

708e348

iritkatriel reviewed May 9, 2021

View reviewed changes

refactor assertions

f021de2

iritkatriel reviewed May 9, 2021

View reviewed changes

Lib/test/test_linecache.py Outdated Show resolved Hide resolved

Lib/test/test_linecache.py Outdated Show resolved Hide resolved

iritkatriel reviewed May 9, 2021

View reviewed changes

Lib/test/test_threading.py Show resolved Hide resolved

uniocto and others added 3 commits May 9, 2021 19:42

Update Lib/test/test_linecache.py

a396fad

Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>

Update Lib/test/test_linecache.py

833c9b9

Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>

Update Lib/test/test_threading.py

bcc20ab

Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>

iritkatriel approved these changes May 9, 2021

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels May 9, 2021

iritkatriel added the skip news label May 9, 2021

iritkatriel requested changes May 12, 2021

View reviewed changes

Lib/test/test_threading.py Outdated Show resolved Hide resolved

uniocto requested review from lysnikolaou, markshannon and pablogsal as code owners May 17, 2021 13:18

iritkatriel removed request for markshannon, pablogsal, lysnikolaou and isidentical May 17, 2021 14:18

uniocto force-pushed the fix-issue-25872 branch from 17dfe1c to 78897e7 Compare May 17, 2021 14:39

Merge remote-tracking branch 'upstream/main' into fix-issue-25872

ac550ef

iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 17, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 17, 2021

iritkatriel added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels May 17, 2021

iritkatriel merged commit 115dea9 into python:main May 18, 2021

bedevere-bot removed the awaiting merge label May 18, 2021

miss-islington assigned iritkatriel May 18, 2021

bedevere-bot removed the needs backport to 3.10 only security fixes label May 18, 2021

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 18, 2021

bpo-25872: Add unit tests for linecache and threading (pythonGH-25913)

bc221b1

(cherry picked from commit 115dea9) Co-authored-by: uniocto <serit142sa33go@gmail.com>

iritkatriel pushed a commit to iritkatriel/cpython that referenced this pull request May 18, 2021

bpo-25872: Add unit tests for linecache and threading (pythonGH-25913)

c051788

(cherry picked from commit 115dea9)

bedevere-bot removed the needs backport to 3.9 only security fixes label May 18, 2021

iritkatriel pushed a commit to iritkatriel/cpython that referenced this pull request May 18, 2021

[3.10] bpo-25872: Add unit tests for linecache and threading (pythonG…

4f32791

…H-25913) (cherry picked from commit 115dea9) Co-authored-by: uniocto <serit142sa33go@gmail.com>

iritkatriel added a commit that referenced this pull request May 18, 2021

[3.10] bpo-25872: Add unit tests for linecache and threading (GH-25913)…

373741a

… (GH-26212) (cherry picked from commit 115dea9) Co-authored-by: uniocto <serit142sa33go@gmail.com>

iritkatriel added a commit that referenced this pull request May 18, 2021

bpo-25872: Add unit tests for linecache and threading (GH-25913) (GH-…

c05d8a6

…26211) (cherry picked from commit 115dea9) Co-authored-by: uniocto <serit142sa33go@gmail.com>

pablogsal mentioned this pull request Jul 18, 2021

[3.10] Correct the order of regen-abidump #27228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-25872: Add unit tests for linecache and threading #25913

bpo-25872: Add unit tests for linecache and threading #25913

uniocto commented May 5, 2021 •

edited by bedevere-bot

Loading

the-knights-who-say-ni commented May 5, 2021

iritkatriel left a comment

iritkatriel May 7, 2021

uniocto May 8, 2021 •

edited

Loading

iritkatriel May 7, 2021

uniocto May 8, 2021

uniocto May 8, 2021

iritkatriel May 7, 2021

uniocto May 8, 2021

iritkatriel May 8, 2021

iritkatriel May 8, 2021

uniocto May 9, 2021 •

edited

Loading

iritkatriel May 8, 2021

uniocto May 9, 2021

iritkatriel May 9, 2021

iritkatriel May 9, 2021

iritkatriel left a comment

iritkatriel commented May 17, 2021

bedevere-bot commented May 17, 2021

miss-islington commented May 18, 2021

miss-islington commented May 18, 2021

bedevere-bot commented May 18, 2021

bedevere-bot commented May 18, 2021

bpo-25872: Add unit tests for linecache and threading #25913

bpo-25872: Add unit tests for linecache and threading #25913

Conversation

uniocto commented May 5, 2021 • edited by bedevere-bot Loading

the-knights-who-say-ni commented May 5, 2021

Recognized GitHub username

iritkatriel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uniocto May 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uniocto May 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iritkatriel left a comment

Choose a reason for hiding this comment

iritkatriel commented May 17, 2021

bedevere-bot commented May 17, 2021

miss-islington commented May 18, 2021

miss-islington commented May 18, 2021

bedevere-bot commented May 18, 2021

bedevere-bot commented May 18, 2021

uniocto commented May 5, 2021 •

edited by bedevere-bot

Loading

uniocto May 8, 2021 •

edited

Loading

uniocto May 9, 2021 •

edited

Loading