bpo-30303: IDLE: Add _utest argument to textview by louisom · Pull Request #1499 · python/cpython
bpo-30303: Add _utest argument to textview
bpo-30303: IDLE: Add _utest argument to textview
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first reading, the textview.py changes are just right. The test needs some minor changes.
| def _command(): | ||
| self.called = True | ||
| self.view = tv.view_text(self.root, 'TITLE_TEXT', 'COMMAND', _utest=self._utest) | ||
| button = Button(self.root, text='BUTTON', command=_command) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use global 'root' here and below.
|
|
||
| class ButtonClickTextViewTest(unittest.TestCase): | ||
| @classmethod | ||
| def setUpClass(cls): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this and tearDownClass are needed. The module root should work. Just pass '_utest=True' in both calls.
| ''' | ||
| from idlelib import textview as tv | ||
| from test.support import requires | ||
| from test.support import requires, findfile |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment below about findfile.
| @@ -8,12 +8,12 @@ | |||
| Coverage: 94%. | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After I adapt my coverage.bat to work with the git repository, I will get new value.
|
|
||
| self.assertEqual(self.called, True) | ||
| self.assertEqual(self.view.title(), 'TITLE_TEXT') | ||
| self.assertEqual(self.view.textView.get('1.0', '1.end'), 'COMMAND') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow with 'button.destroy()'.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using addCleanup instead of directly using button.destroy
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
|
|
||
| def test_view_file_bind_with_button(self): | ||
| def _command(): | ||
| fn = findfile('CREDITS.txt', 'idlelib') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
osp = os.path
fn =osp.abspath(osp.join(osp.dirname('file'), '..', 'CREDITS.txt'))
would be equivalent. But to make the test simpler, just pass 'file' instead of fn and adjust assert.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The boldface __file__is the dunder name interpreted as markup. Unlike StackOverflow, there is no preview of the rendering. Let's try it with backticks __file__.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the interpretation is only in quotes? 'file', "file".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't reproduce the equivalent between osp and findfile. On my situation, I'm running unittest at python/cpython, so the osp.dirname('file') will return ``, not Lib/idlelib/idle_test.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now with quotes and backticks: backtick inner: "__file__", outer '__file__'.
PS I checked the markdown link below but it was insufficient.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see, is __file__, not 'file'
| self.assertEqual(self.view.title(), 'TITLE_FILE') | ||
| self.assertEqual(self.view.textView.get('1.0', '1.end'), | ||
| 'Guido van Rossum, as well as being the creator of the ' | ||
| 'Python language, is the') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using file makes this file self-contained.
self.assertIn(self.view.textView.get('1.0', '1.end'), 'idlelib.textview'
If this file is edited to make this fail, then this file fails immediately.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am about to push my changes back and will merge once CI allows.
| self.assertEqual(self.view.textView.get('1.0', '1.end'), | ||
| f.readline().strip()) | ||
| self.assertEqual(self.view.textView.get('2.0', '2.end'), | ||
| f.readline().strip()) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like making this future proof. However, since the 2nd line is blank and Texts have a default extra \n at the end, I believe this would pass even if the textview only displayed one line. So I added an intermediate f.readline() and changed 2s to 3s.
Mariatta pushed a commit to Mariatta/cpython that referenced this pull request
(cherry picked from commit ba365da)
Can I ask if a CLA was eventually verified as being signed before merging this?
@louisom is my previous account, does b.p.o. support comma split (louisom, mlouielu) at the detail page?
terryjreedy pushed a commit that referenced this pull request
(cherry picked from commit ba365da)
🐍🍒⛏🤖 Thanks @louisom for the PR, and @terryjreedy for merging it 🌮🎉.I'm working now to backport this PR to: 3.6.
The backport error is because this was backported already in #1916. (And the file has been refactored since.) I removed the label.