classification
Title: IDLE Unit test for RstripExtension.py
Type: enhancement Stage: resolved
Components: IDLE Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: terry.reedy Nosy List: JayKrish, Todd.Rovito, philwebster, python-dev, terry.reedy
Priority: normal Keywords: patch

Created on 2013-06-21 22:37 by philwebster, last changed 2013-07-13 06:42 by terry.reedy. This issue is now closed.

Files
File name Uploaded Description Edit
test_rstripext.patch philwebster, 2013-06-21 22:38 review
test_rstrip.patch philwebster, 2013-06-24 23:15 Revised patch with no mock EditorWindow and new name review
test_rstrip2.patch philwebster, 2013-07-04 19:58 New mock IDLE classes and rstrip extension test review
test_rstrip3.patch philwebster, 2013-07-08 04:56 review
Messages (15)
msg191618 - (view) Author: Phil Webster (philwebster) * Date: 2013-06-21 22:37
This is a single test for RstripExtension.py, following from #15392. I also added a mock EditorWindow module with a Text widget for testing. test_rstripextension.py seems to run fine inside IDLE and produces the following output:

>>> 
test_do_rstrip (__main__.Test_rstripextension) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.100s

OK

However, when I run via the command line, I get the following output:

philwebster@ubuntu:~/Dev/cpython$ ./python -m test -ugui test_idle
[1/1] test_idle
Warning -- warnings.showwarning was modified by test_idle
1 test altered the execution environment:
    test_idle

I attempted to replicate the results from #18189, but saw the same message. Any thoughts?
msg191627 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-22 01:35
There is a separate issue about killing that warning.

If you leave off '-ugui', you will see the traceback and why I said that requires('gui') should be wrapped for unittest discovery. As the code is, the call is executed when the file is imported, possibly during discovery, before the  unittest or regrtest runner starts running tests and counting success, fail, skip.

Two other thoughts before I look as rstrip and the content of the new test file. 

I would like to just call it test_rstrip. 

I have thought of having just one mock_idle.py for mock Idle classes (as opposed to mock_tk with mock tk classes. I am not sure how many will will need. Since your mock_ewin uses a real tk.Text widget, hence requiring gui, what is its purpose? The point of the mock_tk classes is to avoid gui and make the test text only. I am not saying that this is the only purpose, but other purposes should be stated and documented.
msg191629 - (view) Author: Phil Webster (philwebster) * Date: 2013-06-22 02:38
Thank you for the feedback Terry. I'm not seeing the traceback without '-ugui' either, so I'm going to look into that. I get the same results with requires('gui') moved inside of setUp, is that what you mean by wrapping?

For mock_ewin I used a real Text widget because RstripExtension uses index(), get(), and delete() and I was not able to figure out how the widget implemented these (is there a single string with the contents?). I can work on a non-gui test though if that's what needs to be done.

Thanks!
msg191821 - (view) Author: Phil Webster (philwebster) * Date: 2013-06-24 23:15
Modified the first patch to get rid of mock EditorWindow in favor of the real thing. Also renamed the test to 'test_rstrip'.
msg192314 - (view) Author: Phil Webster (philwebster) * Date: 2013-07-04 19:58
Added to Terry's Text Widget code (in #18226) and created mock_idle.py for the mock EditorWindow. Todd's FormatParagraph test in the aforementioned issue also passes with the mock EditorWindow.
msg192325 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-05 02:28
I want to make two separate commits. First add mock Text into mock_tk.py, and add a new test_text.py. I suggested looking at the tkinter Text test. I turns out that it only tests a few special search cases. I am guessing that they have something to do with the tkinter interface. If neither you nor Todd feel like writing the Text test, I will. Let me know either way.

Second, add the new mock_idle and test_rstrip files. For the latter, at least one line should have no whitespace, and one should have an indent. With an indent, the test would fail if rstrip in the tested file were changed to strip.

In the tearDown method, the apparent purpose of
        self.rstripextension = None
is to actually do
        del self.rstripextension
and that happens automatically when self disappears.
With a mock editor window, there is no need for "root.destroy" and hence for the close call, and hence for the tearDown method.

With only one test method, the setUp lines can be part of the test. For the attribute names, I strongly prefer 'rstrip' to 'rstripExtension' and 'editor' to 'mockEditorWindow'.

I am curious about this comment:
# Note: Tkinter always adds a newline at the end of the text widget,
# hence the newline in the expected_text string

In live Idle, I tried 'strip trailing whitespace' with text that did not end with \n and there was none visible after.

An annoyance is that after stripping  the filename is prefixed with * to indicate that it has been changed and needs to be changed, even when it has not (or should not have been). Closing brings up the unnecessary 'Changed, save?' box. Is this related to the comment?
msg192329 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-05 06:57
I decided that the first commit should be a separate issue that this issue depends on.
msg192611 - (view) Author: Phil Webster (philwebster) * Date: 2013-07-08 04:56
This patch contains mock_idle.py and the rstrip test using the mock text widget from #18365.

Terry- For some reason, the Text widget always contains a '\n' as the last character even when there is nothing visible. Doing a text.get('1.0','end') always has a '\n' at the end from what I can tell. I'm not sure about the filename changing, is it worth creating a new issue for?
msg192993 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-13 03:30
Phil (and everyone else): PLEASE submit patches with 4 space indents and no tabs and no trailing spaces. Even if the code below runs in the CPython interpreter,

        self.undo = mockUndoDelegator() <8 spaces>
    <4 spaces>
    def get_selection_indices(self): <4 spaces>
    	first = self.text.index('1.0') <4 spaces, 1 tab >

class mockUndoDelegator:
	def undo_block_start(*args): <1 tab>
		pass  <2 tabs>

the CPython repository whitespace pre-commit will say this:

remote:  - file Lib/idlelib/idle_test/test_text.py is not whitespace-normalized in 979905090779
remote: * Run Tools/scripts/reindent.py on .py files or Tools/scripts /reindent-rst.py on .rst files listed above
remote: * and commit that change before pushing to this repo.
remote: transaction abort!
remote: rollback completed

as happened with the mock_tk/test_text patch. I already fixed this file, but next time...

About no trailing whitespace: that is not an option when committing, so I will change the string literal to use explicit \n and implicit catenation, as in 'ab  \n' 'cd\n' == 'ab  \ncd\n'. This will make the trailing ws visible anyway.

The way to not get the tk=added \n is to use 'insert' rather than 'end', as inself.text.get('1.0','insert'). 'Insert' is the end of user input, before the guard.

Will commit patch soon.
msg192998 - (view) Author: Todd Rovito (Todd.Rovito) * Date: 2013-07-13 04:52
As a suggestion I always use the command "make patchcheck" (before making the patch) which catches the white space and tab problem plus it fixes other things.  Here is more information on patch check in the developer's guide.

http://docs.python.org/devguide/committing.html#patch-checklist
msg193000 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-13 05:28
I am aware of patchcheck, but the problem for me is that 'make patchcheck' does not work on Windows; the doc is wrong on the awkward to type Windows alternative; and it is usually useless anyway. But I agree that anyone who does not use a editor configured to automatically converts tabs to 4 spaces, as are both Idle and Notepad++ here, should run reindent.py.

When I learn how to write extensions, maybe I will work on one to run or imitate patchcheck, and offer to open Acks and News for editing if not in the changeset.
msg193002 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-13 06:13
Trying it out, I rediscovered that patchcheck has a Windows bug. This time I reported it ;-) #18439.
msg193003 - (view) Author: Roundup Robot (python-dev) Date: 2013-07-13 06:35
New changeset ec71fcdcfeac by Terry Jan Reedy in branch '2.7':
Issue #18279: Add tests for idlelib/RstripExtension.py. Original patch by
http://hg.python.org/cpython/rev/ec71fcdcfeac

New changeset 22ce68d98345 by Terry Jan Reedy in branch '3.3':
Issue #18279: Add tests for idlelib/RstripExtension.py. Original patch by
http://hg.python.org/cpython/rev/22ce68d98345
msg193005 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-13 06:40
A simple change to RstripExtension.py fixed the marking of unchanged files as changed. I also removed a useless extra iteration. Having a test makes it possible to do things like this without breaking what already worked.
msg193006 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-13 06:42
I had to remove the dependency to close this, since the test_text issue #18365 was reopened.
History
Date User Action Args
2013-07-13 06:42:29terry.reedysetstatus: open -> closed

messages: + msg193006
stage: patch review -> resolved
2013-07-13 06:40:25terry.reedysetresolution: fixed
dependencies: - Idle: mock Text class and test thereof
messages: + msg193005
2013-07-13 06:35:33python-devsetnosy: + python-dev
messages: + msg193003
2013-07-13 06:13:05terry.reedysetmessages: + msg193002
2013-07-13 05:28:07terry.reedysetmessages: + msg193000
2013-07-13 04:52:37Todd.Rovitosetmessages: + msg192998
2013-07-13 03:30:54terry.reedysetmessages: + msg192993
2013-07-08 04:56:03philwebstersetfiles: + test_rstrip3.patch

messages: + msg192611
2013-07-05 06:57:33terry.reedysetassignee: terry.reedy
dependencies: + Idle: mock Text class and test thereof
messages: + msg192329
2013-07-05 02:28:16terry.reedysetmessages: + msg192325
stage: patch review
2013-07-04 19:58:42philwebstersetfiles: + test_rstrip2.patch

messages: + msg192314
2013-06-24 23:15:17philwebstersetfiles: + test_rstrip.patch

messages: + msg191821
2013-06-22 02:38:31philwebstersetmessages: + msg191629
2013-06-22 01:35:29terry.reedysetmessages: + msg191627
versions: + Python 3.3
2013-06-21 22:38:46philwebstersetfiles: + test_rstripext.patch
keywords: + patch
2013-06-21 22:38:33philwebstersetnosy: + terry.reedy
2013-06-21 22:38:14philwebstersetnosy: + Todd.Rovito
2013-06-21 22:37:53philwebstercreate