Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
bpo-25872: Add unit tests for linecache and threading#25913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
the-knights-who-say-ni commentedMay 5, 2021
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 thePSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find abugs.python.org (b.p.o) account corresponding to the following GitHub usernames: 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 followthe steps outlined in the CPython devguide to rectify this issue. You cancheck yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for your first CPython PR! I hope this will be followed by many more.
See a couple of comments below.
Lib/test/test_threading.py Outdated
| thread.exc=None | ||
| deftest_multithread_modify_file_noerror(self): | ||
| importtraceback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I believePEP-8 asks for imports to be at module scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 0Lib/test/test_threading.py:47:5: E301 expected 1 blank line, found 0Lib/test/test_threading.py:49:5: E301 expected 1 blank line, found 0Lib/test/test_threading.py:52:1: E302 expected 2 blank lines, found 1Lib/test/test_threading.py:145:41: E228 missing whitespace around modulo operatorLib/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 0Lib/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 commentLib/test/test_threading.py:285:24: E261 at least two spaces before inline commentLib/test/test_threading.py:307:36: E261 at least two spaces before inline commentLib/test/test_threading.py:408:13: E128 continuation line under-indented for visual indentLib/test/test_threading.py:424:21: E128 continuation line under-indented for visual indentLib/test/test_threading.py:436:69: E231 missing whitespace after ':'Lib/test/test_threading.py:450:26: E128 continuation line under-indented for visual indentLib/test/test_threading.py:458:26: E128 continuation line under-indented for visual indentLib/test/test_threading.py:716:9: E301 expected 1 blank line, found 0Lib/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 0Lib/test/test_threading.py:769:44: E261 at least two spaces before inline commentLib/test/test_threading.py:838:40: E231 missing whitespace after ','Lib/test/test_threading.py:900:13: E301 expected 1 blank line, found 0Lib/test/test_threading.py:902:13: E301 expected 1 blank line, found 0Lib/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 semicolonLib/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 indentLib/test/test_threading.py:1450:14: E127 continuation line over-indented for visual indentLib/test/test_threading.py:1509:1: E302 expected 2 blank lines, found 1Lib/test/test_threading.py:1512:1: E302 expected 2 blank lines, found 1Lib/test/test_threading.py:1515:1: E302 expected 2 blank lines, found 1Lib/test/test_threading.py:1519:1: E302 expected 2 blank lines, found 1Lib/test/test_threading.py:1522:1: E302 expected 2 blank lines, found 1Lib/test/test_threading.py:1526:1: E302 expected 2 blank lines, found 1Lib/test/test_threading.py:1529:1: E302 expected 2 blank lines, found 1Lib/test/test_threading.py:1532:1: E302 expected 2 blank lines, found 1Lib/test/test_threading.py:1535:1: E302 expected 2 blank lines, found 1Lib/test/test_threading.py:1651:17: E128 continuation line under-indented for visual indent> pep8 Lib/test/test_linecache.pyLib/test/test_linecache.py:229:9: E301 expected 1 blank line, found 0Lib/test/test_linecache.py Outdated
| self.assertTrue(_) | ||
| self.assertEqual(1,len(linecache.cache.keys())) | ||
| withsupport.swap_attr(os,'stat',raise_oserror): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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()))Lib/test/test_linecache.py Outdated
| _=linecache.getlines(source_name) | ||
| self.assertEqual(1,len(linecache.cache.keys())) | ||
| withsupport.swap_attr(os,'stat',raise_oserror): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As above - delete/modify the file instead of mocking os.stat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As shown above, I write delete/modify test.
Lib/test/test_linecache.py Outdated
| self.assertEqual(lines3, []) | ||
| self.assertEqual(linecache.getlines(FILENAME),lines) | ||
| deftest_oserror(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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).
- seehttps://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 , sayclass 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.
Lib/test/test_threading.py Outdated
| fortinthreads: | ||
| t.start() | ||
| fortinthreads: | ||
| t.join() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a reason why you don't do
for t in threads: t.start() t.join()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nothing, sorry.
So I fixed it to your feedback.
Lib/test/test_threading.py Outdated
| threading.Thread(target=modify_file) | ||
| foriinrange(100) | ||
| ] | ||
| try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Which exceptions are you trying to ignore here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nothing, sorry. I removedtry.
Lib/test/test_linecache.py Outdated
| deftest_checkcache_with_no_parameters(self): | ||
| self.assertEqual(3,len(linecache.cache.keys())) | ||
| linecache.checkcache() | ||
| self.assertTrue([self.unchange_file]==list(linecache.cache.keys())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
assertEqual
Lib/test/test_linecache.py Outdated
| self.assertEqual(3,len(linecache.cache.keys())) | ||
| linecache.checkcache(self.deleted_file) | ||
| self.assertTrue(2==len(linecache.cache.keys())and | ||
| self.deleted_filenotinlinecache.cache.keys()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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).
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>
Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>
Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>
Uh oh!
There was an error while loading.Please reload this page.
The rebase doesn't look right - the PR should not be showing so many changed files. |
bedevere-bot commentedMay 17, 2021
🤖 New build scheduled with the buildbot fleet by@iritkatriel for commitac550ef 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Thanks@uniocto for the PR, and@iritkatriel for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9. |
Sorry,@uniocto and@iritkatriel, I could not cleanly backport this to |
bedevere-bot commentedMay 18, 2021
GH-26208 is a backport of this pull request to the3.10 branch. |
(cherry picked from commit115dea9)Co-authored-by: uniocto <serit142sa33go@gmail.com>
(cherry picked from commit115dea9)
bedevere-bot commentedMay 18, 2021
GH-26211 is a backport of this pull request to the3.9 branch. |
Uh oh!
There was an error while loading.Please reload this page.
https://bugs.python.org/issue25872
https://bugs.python.org/issue25872