Title: Idle: mock Text class and test thereof
Type: enhancement Stage: resolved
Components: IDLE Versions: Python 3.3, Python 3.4, Python 2.7
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: terry.reedy Nosy List: JayKrish, Todd.Rovito, ncoghlan, philwebster, python-dev, r.david.murray, terry.reedy
Priority: normal Keywords: patch

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

File name Uploaded Description Edit
mock_text.patch philwebster, 2013-07-08 04:55 review
mock_text2.diff terry.reedy, 2013-07-08 22:27 (tjr) review
mock_text5.diff terry.reedy, 2013-07-12 05:11 review
mock_text6.diff terry.reedy, 2013-07-12 06:38 review
Messages (21)
msg192327 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-05 06:49
test_rstrip2.patch contain a Text class to be added to The purpose of a mock class is to imitate another class with respect to certain behaviors. The way to verify that is does that is to run the same set of tests for those behaviors with both. This is similar in idea and will be the same in execution as running the same set of tests with a Python-coded module and a c-coded accelerator version of at least some functions of the module. The trick is to do this without code duplication is write a mixin class with the tests and derive two testcase classes using the mixin.
msg192328 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-05 06:56
The rstrip patch is with #18279. Design discussion occurred on #18226. Both issues and other future issues depend on this one.
msg192610 - (view) Author: Phil Webster (philwebster) * Date: 2013-07-08 04:55
Added Text class to and GUI/non-GUI tests in Running the IDLE tests produced no errors for me.
msg192700 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-08 22:27
Great start! Some revisions:
* Create root directly (and just once); add root.destroy, which added several warnings.
* Create Text directly; test should not require Editors. This removed warnings. I suspect that does other things that are not properly undone. In any case, tests should only create needed objects.
* Worked on docstrings; polished _decode.
* Revised .insert to handle '' and 'a....z\n' and added corresponding tests.
* Consistently used 'end' instead of 'end' here and 'END' there.
* Added test_no_delete and edited mock delete to pass.
* Finished compare methods and added tests.
* Deleted non-tk .setDate/.getData, which only slightly abbreviate the real .get and .insert methods. I notice that Phil already changed the FormatParagraph test to not use them.

Here is the result. I think it is about ready to commit after sleeping on it and a final review.
msg192912 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-11 21:34
Upon further review and tests, I notice that ._decode
* is a partial implementation of Text.index,
* so it should be renamed index and tested against Text.index,
* and it currently returns the wrong values for 'end' and high out-of-bounds indexes.

Tests with 'end' only pass now because the tested methods ignore the buggy decode for 'end' and special-case it instead. When I added tests with out of bounds indexes, they failed. I am working now on fixing all this.

Spaces are supposed to follow arg-separating commas in function calls. The initial patch lacked them and I have added them. I need them because they make code easier for *me* to read and review, which is an example of why PEP-8 requires them.
msg192915 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-11 22:27
Whoops, I got that partly wrong in that index returns a 'r.c' string after doing what _decode is supposed to do, which is to decode the input according to the current text. The mock index should just return '%s.%s' % _decode(position). There is still the point that index and _decode now give different coordinates for 'end' and that _decode should be tested by testing mock index, which wraps it, against tk index.

Since tk.Text args can be passed by name, the mock methods should use the same parameter names.
msg192916 - (view) Author: Phil Webster (philwebster) * Date: 2013-07-11 22:42
Thanks Terry, I will start PEP8-checking my code before I submit (as well as testing more thoroughly). I was thinking the same thing about the logic behind _decode and index functions needing to be combined.

How would you recommend adding functionality to the decode function to handle expressions like "1.2 +5 chars" or "1.0 lineend-5c"? (#18409) uses this kind of index, so it will be needed soon. I'm thinking that splitting off the base index then parsing the remaining modifier expressions is the way to go.

I was also thinking that a _checkIndex method would be good to have especially when using the +/-line and +/-char expressions.
msg192921 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-12 05:11
The real problem with 'end' and 'big.m' is that there are three proper decodings, depending on the method asking. I think this patch mostly solves the problems mentioned before. All tests, including many new ones, pass.
msg192926 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-12 06:38
A little more polishing and the tests pass again. I think I am done with the implemented methods. Before I commit, I want to review the set of 'pass' functions. It seems a bit haphazard. I think display changing methods like .see are fine.I am not sure about including internal operation methods like .mark_set (until actually needed).
msg192930 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-12 07:49
I want to commit this patch more or less as is so we can move forward with the tests that do not need anything more. We can then elaborate mock Text as needed and desired.

Looking at, for instance, I see that 'lineend' is an expression modifier in that same category as +/- chars/lines. So _decode should not really handle it until it handles such modifiers more generally.

As near as I could tell from experiment, 'm.n lineend' is the same as 'm.end', so the former is never needed and could be changed to the latter in Idle code. As far as you know, correct? The use for 'lineend' would seem to be after other modifiers, as in '1.0 +100c lineend', where the line whose end we go to is likely not line 1.

Remember that for advanced Text use, we have the option of using tk.Text for a test, at least at first. The gain would be not having to reproduce baroque behavior*. The cost would be making the test a gui test and running on fewer buildbots. I felt that the basic mock done so far would be worth the time because a) it would be useful for many tests and b) I would learn the Text widget and how to experiment with it to determine its under-documented behavior.

*For instance, having optional spaces within modifiers as well as using them to separate modifiers. Do you know if there is a coherent index grammar anywhere? Or is it only defined by the Text.index C code?
msg192934 - (view) Author: Phil Webster (philwebster) * Date: 2013-07-12 10:33
Using tk.Text for more involved tests sounds good (at least as a start). The pass functions were used to get the FormatParagraph test (#18226) working.

I've been using the same site as a reference and haven't found anything more detailed.
msg192982 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-07-13 00:19
New changeset 5ac2ec0a34a5 by Terry Jan Reedy in branch '2.7':
Issue #18365: Add mock Text class and test thereof versus tk.Text.

New changeset 8f13fb4c5826 by Terry Jan Reedy in branch '3.3':
Issue #18365: Add mock Text class and test thereof versus tk.Text.

New changeset 5611d3a954f8 by Terry Jan Reedy in branch '3.3':
Issue #18365: normalize whitespace

New changeset 86cc1983a94d by Terry Jan Reedy in branch '2.7':
Issue #18365: normalize whitespace
msg192983 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-13 00:20
I documented and left most of the 'pass' methods. The bind method is inherited from Misc class and is not specifically a Text method, but I will leave it here for now. These

    def undo_block_start(self, *args):

    def undo_block_stop(self, *args):

are not Text methods, so I removed them. We can discuss what you intended as part of the FormatParagraph issue.
msg192987 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-13 01:44
Looks like this is causing buildbot failures:
msg192995 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-07-13 04:02
New changeset 008d83c4efaf by Terry Jan Reedy in branch '2.7':
Issue #18365: 2.7 corrections so tests run
msg192999 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-13 05:04
Well, that is annoying. I expected 2.7 failures due to mistakes already corrected, but not with 3.x, and these look to be the latter.

ERROR: setUpClass (idlelib.idle_test.test_text.TkTextTest)
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/idlelib/idle_test/", line 219, in setUpClass
    cls.root = Tk()
  File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/tkinter/", line 1789, in __init__ = _tkinter.create(screenName, baseName, className, interactive, wantobjects, useTk, sync, use)
_tkinter.TclError: no display name and no $DISPLAY environment variable

I do not get this on my system, and I just retried with 
F:\Python\dev\py34\PCbuild>python_d -m test -v -ugui test_idle
and that works.

I did get a test_idle failure running all tests
and a crash with test_marshal in test_bad_reader (test.test_marshal.BugsTestCase)
 so I am rerunning with -v. This claims the problem is

_tkinter.TclError: Can't find a usable init.tcl in the following directories:

This is followed by a list of non-existent directories that omits the one what it actually is and where it is routinely found and it obviously was on the previous run with only test_idle run.

What puzzles me is that both buildbots skipped test_tk and test_ttk_guionly, which suggests that they do not have -ugui, (Nick, one of these is yours) which means they should not have ever called tkinter. On my machine,
works to stop the tkinter test case from running when I omit -ugui.
msg193007 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-07-13 08:07
New changeset bc3a34e47923 by Terry Jan Reedy in branch '3.3':
Issue #18365: convert buildbot errors to skips.

New changeset ba4c826848d5 by Terry Jan Reedy in branch '2.7':
Issue #18365: convert buildbot errors to skips.
msg193008 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-13 08:07
Looking through the 11 stable 3.x buildbots (1 still running), I see 3 outcomes.
1. Windows: test seemed to run, test_idle listed in 'altered environment'.
2. Some *nix: test gave no error, test_idle lit in 'tests skipped'. I presume this means that at least one thing was skipped, not the entire idle suite.
3. Other *nix: no test failed, but the one test case setup gave the tclerror. This also includes the 'murray' machine. Yours, David?

_tkinter.create just calls Tkapp_New with the same args and the message comes from tcl/tk. I do not know if 'display name' is the same as the 'screenName' parameter or not. The doc string just says "Return a new Toplevel widget on screen SCREENNAME." Is this something to do with X11? If so, why isn't $DISPLAY set? Passing screenName='fake' has no effect on my Win7 machine.

I think the following patch should quiet things for now, even if not the best patch.
msg193011 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-13 13:43
Yeah, DISPLAY is how unix finds the X11 display.  I don't remember how the tkguionly tests deal with that being missing, but since the tests are run "headless" there is indeed no DISPLAY for the tests to talk to.
msg193012 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-13 13:48
Looks like ttk_guionly gets skipped on my buildbot because of the following code at the top of test_ttk_guionly:

   except TclError as msg:
      # assuming ttk is not available
      raise unittest.SkipTest("ttk not available: %s" % msg)

So your skip fix may actually be the best solution.
msg193016 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-13 22:37
The change worked, so this issue can be closed. I opened a new issue, #18441, for dealing with the problem in test_idle by moving the TclError check there, so it does not arise in other idle_test/test_*.py files.
Date User Action Args
2013-07-13 22:37:23terry.reedysetstatus: open -> closed

messages: + msg193016
stage: needs patch -> resolved
2013-07-13 13:48:28r.david.murraysetmessages: + msg193012
2013-07-13 13:43:51r.david.murraysetmessages: + msg193011
2013-07-13 08:07:54terry.reedysetmessages: + msg193008
2013-07-13 08:07:04python-devsetmessages: + msg193007
2013-07-13 06:40:25terry.reedyunlinkissue18279 dependencies
2013-07-13 05:04:41terry.reedysetnosy: + ncoghlan
messages: + msg192999
2013-07-13 04:02:46python-devsetmessages: + msg192995
2013-07-13 01:44:28r.david.murraysetstatus: closed -> open

nosy: + r.david.murray
messages: + msg192987

stage: commit review -> needs patch
2013-07-13 00:20:21terry.reedysetstatus: open -> closed
resolution: fixed
messages: + msg192983
2013-07-13 00:19:12python-devsetnosy: + python-dev
messages: + msg192982
2013-07-12 10:33:36philwebstersetmessages: + msg192934
2013-07-12 07:49:14terry.reedysetmessages: + msg192930
2013-07-12 06:38:21terry.reedysetfiles: + mock_text6.diff

messages: + msg192926
2013-07-12 05:11:15terry.reedysetfiles: + mock_text5.diff

messages: + msg192921
2013-07-11 22:42:06philwebstersetmessages: + msg192916
2013-07-11 22:27:20terry.reedysetmessages: + msg192915
2013-07-11 21:34:23terry.reedysetmessages: + msg192912
2013-07-11 03:00:14JayKrishsetnosy: + JayKrish
2013-07-08 22:27:30terry.reedysetfiles: + mock_text2.diff

messages: + msg192700
stage: test needed -> commit review
2013-07-08 04:55:54philwebstersetfiles: + mock_text.patch
keywords: + patch
messages: + msg192610
2013-07-05 06:57:33terry.reedylinkissue18279 dependencies
2013-07-05 06:56:52terry.reedysetmessages: + msg192328
2013-07-05 06:55:20terry.reedylinkissue18226 dependencies
2013-07-05 06:49:06terry.reedycreate