bpo-30303: IDLE: Add _utest argument to textview by louisom · Pull Request #1499 · python/cpython

louisom · 2017-05-08T08:23:57Z

No description provided.

terryjreedy

At first reading, the textview.py changes are just right. The test needs some minor changes.

terryjreedy · 2017-05-17T06:15:11Z

Lib/idlelib/idle_test/test_textview.py

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


Use global 'root' here and below.

terryjreedy · 2017-05-17T06:15:14Z

Lib/idlelib/idle_test/test_textview.py


+class ButtonClickTextViewTest(unittest.TestCase):
+    @classmethod
+    def setUpClass(cls):


I don't think this and tearDownClass are needed. The module root should work. Just pass '_utest=True' in both calls.

terryjreedy · 2017-05-17T06:15:20Z

Lib/idlelib/idle_test/test_textview.py

 '''
 from idlelib import textview as tv
-from test.support import requires
+from test.support import requires, findfile


See comment below about findfile.

terryjreedy · 2017-05-17T06:15:21Z

Lib/idlelib/idle_test/test_textview.py

@@ -8,12 +8,12 @@
 Coverage: 94%.


After I adapt my coverage.bat to work with the git repository, I will get new value.

terryjreedy · 2017-05-17T06:16:11Z

Lib/idlelib/idle_test/test_textview.py

+
+        self.assertEqual(self.called, True)
+        self.assertEqual(self.view.title(), 'TITLE_TEXT')
+        self.assertEqual(self.view.textView.get('1.0', '1.end'), 'COMMAND')


Follow with 'button.destroy()'.

I'm using addCleanup instead of directly using button.destroy

terryjreedy · 2017-05-17T06:30:36Z

Lib/idlelib/idle_test/test_textview.py

+
+    def test_view_file_bind_with_button(self):
+        def _command():
+            fn = findfile('CREDITS.txt', 'idlelib')


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.

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__.

Maybe the interpretation is only in quotes? 'file', "file".

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.

Now with quotes and backticks: backtick inner: "__file__", outer '__file__'.
PS I checked the markdown link below but it was insufficient.

ah, I see, is __file__, not 'file'

terryjreedy · 2017-05-17T06:32:29Z

Lib/idlelib/idle_test/test_textview.py

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


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.

codecov · 2017-05-17T06:53:20Z

Codecov Report

Merging #1499 into master will decrease coverage by 0.75%.
The diff coverage is 10.52%.

@@            Coverage Diff             @@
##           master    #1499      +/-   ##
==========================================
- Coverage   83.44%   82.68%   -0.76%     
==========================================
  Files        1368     1432      +64     
  Lines      346621   353062    +6441     
==========================================
+ Hits       289241   291934    +2693     
- Misses      57380    61128    +3748

Impacted Files	Coverage Δ
Lib/idlelib/idle_test/test_textview.py	`3.12% <0%> (-1.43%)`	⬇️
Lib/idlelib/textview.py	`25.45% <57.14%> (-0.48%)`	⬇️
Lib/__future__.py	`6.45% <0%> (-90.33%)`	⬇️
Lib/test/test_select.py	`87.69% <0%> (-1.54%)`	⬇️
Lib/wsgiref/handlers.py	`88.41% <0%> (-0.86%)`	⬇️
Lib/test/libregrtest/main.py	`59.18% <0%> (-0.35%)`	⬇️
Lib/_strptime.py	`94.68% <0%> (-0.34%)`	⬇️
Lib/bdb.py	`26.84% <0%> (-0.31%)`	⬇️
Lib/test/test_strptime.py	`94.76% <0%> (-0.27%)`	⬇️
Lib/test/test_ftplib.py	`95.72% <0%> (-0.26%)`	⬇️
... and 103 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55ace65...0c33759. Read the comment docs.

terryjreedy

I am about to push my changes back and will merge once CI allows.

terryjreedy · 2017-05-17T20:09:43Z

Lib/idlelib/idle_test/test_textview.py

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


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.

(cherry picked from commit ba365da)

brettcannon · 2017-06-05T18:52:47Z

Can I ask if a CLA was eventually verified as being signed before merging this?

Mariatta · 2017-06-06T03:47:49Z

@brettcannon I asked the PR author in bpo-30303 about this.
It seems that @louisom is the same person as @mlouielu, and @mlouielu has signed the CLA.

mlouielu · 2017-06-06T03:54:57Z

@louisom is my previous account, does b.p.o. support comma split (louisom, mlouielu) at the detail page?

mlouielu · 2017-06-06T04:00:50Z

@Mariatta I've add another CLA to @louisom, please help to verify this, thanks!

(cherry picked from commit ba365da)

brettcannon · 2017-06-07T21:40:33Z

@Mariatta @mlouielu thanks for the clarification!

miss-islington · 2017-09-07T00:24:19Z

🐍🍒⛏🤖 Thanks @louisom for the PR, and @terryjreedy for merging it 🌮🎉.I'm working now to backport this PR to: 3.6.

miss-islington · 2017-09-07T00:24:22Z

Sorry, @louisom and @terryjreedy, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.

terryjreedy · 2017-09-07T05:42:32Z

The backport error is because this was backported already in #1916. (And the file has been refactored since.) I removed the label.

bpo-30303: Add _utest argument to textview

2dcc46b

louisom changed the title ~~bpo-30303: Add _utest argument to textview~~ bpo-30303: IDLE: Add _utest argument to textview May 10, 2017

terryjreedy requested changes May 17, 2017

View reviewed changes

louisom added 2 commits May 17, 2017 14:50

Remove setUpClass and using global root for test

c099342

Add cleanup for test button

fccde83

louisom and others added 4 commits May 17, 2017 14:55

Make view_file test self-contained

d30ae3f

Remove unused import

62e88f8

Make viewfile test self-compare

0c33759

Merge remote-tracking branch 'origin/master' into pr_1499

2d85827

terryjreedy approved these changes May 17, 2017

View reviewed changes

Tweak new test. Edit and add docstrings to textview.

766ddaa

terryjreedy merged commit ba365da into python:master May 17, 2017

terryjreedy added the needs backport to 3.6 label May 18, 2017

Mariatta added CLA not signed and removed CLA not signed labels May 31, 2017

the-knights-who-say-ni added the CLA not signed label May 31, 2017

Mariatta removed the CLA not signed label May 31, 2017

the-knights-who-say-ni added the CLA not signed label May 31, 2017

Mariatta removed the CLA not signed label May 31, 2017

the-knights-who-say-ni added the CLA not signed label May 31, 2017

Mariatta pushed a commit to Mariatta/cpython that referenced this pull request Jun 2, 2017

[3.6] bpo-30303: IDLE: Add _utest argument to textview (pythonGH-1499)

ba5f530

(cherry picked from commit ba365da)

brettcannon mentioned this pull request Jun 5, 2017

[3.6] bpo-30303: IDLE: Add _utest argument to textview (GH-1499) #1916

Merged

terryjreedy pushed a commit that referenced this pull request Jun 6, 2017

[3.6] bpo-30303: IDLE: Add _utest argument to textview (GH-1499) (#1916)

e1a60d9

(cherry picked from commit ba365da)

Mariatta removed the CLA not signed label Jun 6, 2017

the-knights-who-say-ni added the CLA signed label Jun 6, 2017

terryjreedy mentioned this pull request Jun 10, 2017

Questions about cherry-picker from Windows never-venv user python/core-workflow#113

Closed

Mariatta added needs backport to 3.6 and removed needs backport to 3.6 labels Sep 7, 2017

terryjreedy removed the needs backport to 3.6 label Sep 7, 2017

Uh oh!

Conversation

louisom commented May 8, 2017

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettcannon commented Jun 5, 2017

Uh oh!

Mariatta commented Jun 6, 2017

Uh oh!

mlouielu commented Jun 6, 2017

Uh oh!

mlouielu commented Jun 6, 2017

Uh oh!

brettcannon commented Jun 7, 2017

Uh oh!

miss-islington commented Sep 7, 2017

Uh oh!

miss-islington commented Sep 7, 2017

Uh oh!

terryjreedy commented Sep 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented May 17, 2017 •

edited

Loading