IDLE Unit test for FormatParagrah.py

[rovitotv]

BPO	18226
Nosy	@terryjreedy, @serwy, @rovitotv
Dependencies	bpo-18365: Idle: mock Text class and test thereof
Superseder	bpo-18583: Idle: enhance FormatParagraph
Files	18226IDLEUnitTestFormatParagraph.patch mock_tk.py 18226IDLEUnitTestFormatParagraph2.patch 18226IDLEUnitTestFormatParagraph3.patch 18226IDLEUnitTestFormatParagraph4.patch 18226IDLEUnitTestFormatParagraph5.patch 18226IDLEUnitTestFormatParagraph6.patch 18226FormatPara7.diff 18226FormatPara8.diff 18226FormatPara9.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/terryjreedy'
closed_at = <Date 2013-08-10.21:24:34.133>
created_at = <Date 2013-06-15.22:33:56.957>
labels = ['expert-IDLE', 'type-feature']
title = 'IDLE Unit test for FormatParagrah.py'
updated_at = <Date 2013-08-15.19:15:19.440>
user = 'https://github.com/rovitotv'

bugs.python.org fields:

activity = <Date 2013-08-15.19:15:19.440>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = True
closed_date = <Date 2013-08-10.21:24:34.133>
closer = 'terry.reedy'
components = ['IDLE']
creation = <Date 2013-06-15.22:33:56.957>
creator = 'Todd.Rovito'
dependencies = ['18365']
files = ['30743', '30746', '30853', '30947', '30960', '30982', '31032', '31070', '31192', '31204']
hgrepos = []
issue_num = 18226
keywords = ['patch']
message_count = 22.0
messages = ['191242', '192136', '192155', '192162', '192164', '192173', '192175', '192612', '193220', '193252', '193267', '193268', '193371', '193687', '193844', '193849', '194644', '194718', '194833', '194836', '195275', '195276']
nosy_count = 6.0
nosy_names = ['terry.reedy', 'roger.serwy', 'Todd.Rovito', 'python-dev', 'JayKrish', 'philwebster']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = '18583'
type = 'enhancement'
url = 'https://bugs.python.org/issue18226'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

[rovitotv]

Continuing the IDLE unittest framework initiated in http://bugs.python.org/issue15392.

A small unit test for IDLE FormatParagraph.py. This patch introduces a test module named test_format_paragraph.py inside Lib/idlelib/idle_test, considering the guidance in README file from idle_test. I should have a patch uploaded by Monday 6/17/2013 night.

[rovitotv]

Here is a uncompleted patch but works for the most part. I thought I would post just in case somebody wanted to provide me comments on the general direction of the patch. The naming might have to change but this follows Terry Reedy's model of monkey patching.

[terryjreedy]

The test passes as written, so I am not sure what 'mostly works' means.

I said elsewhere, and revised README to match, that I changed my mind about test case names starting with 'Test'. Ending with 'Test' is much more common in /test and I decided from experience that it works better in verbose listings.

A complete test of FormatParagraph might have these test cases:
Is_Get_Test (with methods for is_ and get_ functions)
FindTest
ReformatFunctionTest
ReformatClassTest

I am assuming that the latter 3 will each need multiple tests. You can add blank subclasses, possibly with comments on what to do, as I did in test_grep.

(While reviewing reformat_paragraph, you might consider whether you think the 'xxx Should' comments are valid.)

Guido just reminded pydev readers that a proper docstring is a single summary line, with period, optionally followed by a blank line and more explanation. For multiple lines, the closing ''' should be on its own line.

Idle is deficient in this regard, either omitting docstrings where needed or writing docstrings as comments or mis-formatting multiple lines. Writing tests is a good time to add or revise them because one has to understand the function to test it. See comments on review.

I think putting a mock text widget and mock editor class together is one file. They should eventually be filled out, but starting with pass is ok. But see next message.

'Monkey-patching' is importing a module and changing a binding within the module. For Idle testing, a reason to do this is to avoid using tkinter. For example, test_config_name imports configSectionNameDialog and rebinds tkMessageBox to a mock_tk.Mbox*. This is necessary because even though methods are rebound as attributes of a dummy class in the test module, their read-only .__globals__ attribute still points to the module where they are defined.

Monkey patching is only needed for global names used within the method tested. All self.xyx attribute references are resolved with the dummy instance and class. Hence mock Var is used in the dummy class but not monkey-patched into the imported module.

This test does not monkey patch and does not need too. The only imported glogals are re and idleConf, and the latter does not (I presume) involve tkinter. Neither do any of the objects defined in the module. It is really handy for testing that FormatParagraph is initialized with a passed-in editwin argument, so we can simply pass in a non-gui substitute for testing.

• This is done is a setup function so it can and will be undone in a matching teardown function.

[terryjreedy]

The patch models Text.data as a single string. But a tkinter Text contains a sequence of strings. While the single sequence works for the one test of the ...event method, a list of strings is needed for .get and many other methods with position parameters to actually work. And .get is needed for testing find_paragraph and, I am sure, other methods of other classes.

I am not sure of the correct initialization, but it mostly will not matter.

More troublesome is that tkinter lines start at 1, not 0. Possible responses are 1) add a dummy first line to self.data, 2) added all line number by -1 before indexing, or 3) ignore the difference as it will not matter for most tests. I like 3). Any test that cares can prepend an extra \n to the beginning of the text is loads into the editor.

Choosing 3, the methods would be

def __init__(self):
    self.data = ['']  # I think

def setData(self, text)
    self.data = text.split('\n)

def getData(self):
    return '\n'.join(self.data)

# easy so far ;-)

def _decode(self, position):  # private helper
    line, col = position.split('.')
    line = int(line)
    col = len(self.data[line]) if col == '0 lineend' else int(col)
    return line, col

Doc string for Text.get(self, index1, index2=None) is
"Return the text from INDEX1 to INDEX2 (not included)."
Interpreting this gives

def get(self, start, end=None):
    line, col = self._decode(start)
    if end is None:
        return self.data[line][col]
    else:
        endline, endcol = self._decode(end)
        if line == endline:
            return self.data[line][col:endcol]
        else:
            lines = [self.data[line][col:]]
            for i in range(line+1, endline):
                lines.append(self.data[i])
            lines.append(self.data[endline][:endcol])
            return '\n'.join(lines)

This .get code can be used or adapted for .count, .dump, .delete, .replace, and even .insert. At this point, we need a test for the mock Text class. Maybe we can extract something from tkinter.Text tests.

I am not sure how far to go with this; at some point (use of marks or tags?), we say "Use tkinter.Text and make it a gui test.". But handling as least basic indexing and pairs of indexes seems essential to me.

For gui methods like .see and .scan*, the docstring should be something short like "Gui method, do nothing."

[philwebster]

I'm not sure if this is worth pursuing, but I made a mock Text Widget that behaves more like an actual Text Widget. I've attached my modified mock_tk.py which I used to create a mock editor window. This works for the test I made in bpo-18279, but I am also working on a version similar to Todd's for that issue.

[rovitotv]

Terry,
Thank you for the feedback this helps me alot! I will work with Phil Webster and will use his Text Widget and EditorWindow classes. Hopefully this will help us converge on a strong unit test for FormatParagraph.py. Thanks for the reminder about triple quoted strings I think your idea to make sure those strings are in place now is excellent.

[terryjreedy]

Phil (and Todd): I do not know what you mean by "behaves like more like an actual Text Widget" and whether you are talking about internal or external behavior. But I know that tk does not use an immutable string to hold the text. There is no such thing in C. Tk might use one array of 2-byte words with a gap at the cursor and an auxiliary line pointer array, or it might use an array of line structure similar to what I did (but again with arrays rather than immutable strings for each line), or possibly some other structure, However we could only imitate the internals of tk.Text by using (deprecated) PyUNICODE arrays (see the array doc) and doing much more programming effort.

Re-splitting a single line over and over is too inefficient to consider for a repository patch. I believe your particular indexToInt with getLine makes the conversion O(n*n) instead of 'just' O(n), whereas keeping the line in split form makes it O(1).

Side note, as far as I know, "re.split('\n', text)" (the r prefix is not needed here) does the same thing as text.split('\n'). But it is slower and harder to type. We don't use re when a string method does the same thing.

[philwebster]

I made slight modifications to Todd's initial patch to test the mock Text widget from bpo-18365.

Thank you for the helpful feedback Terry!

[philwebster]

Added tests for FormatParagraph using single/multiline comment blocks and single/multiline strings in the mock editor window.

Here is a summary of the changes:

FormatParagraph.py

• description for format_paragraph_event
• modified so that selecting a long comment line will comment the new wrapped line
• if __name__ == __main__"...

mock_tk.py

• FormatParagraph uses the 'insert' tag, so I implemented tag_add using a dict (not sure if this is the best option...)
• index returns '' if the index refers to selection that doesn't exist (this replicates the real Text widget). It does this by excepting a TypeError and I'm guessing there is a better way to do it than this.
• tags (i.e. 'sel.first') can be set to 'end' or other indexes which would not work with a regular Text widget, but it may be useful here.

mock_idle.py

• added undo start/stop for the mock text widget
• used 'sel.first' and 'sel.last' for get_selection_indices()

test_rstrip.py

• This test used the 'insert' tag, so I set 'insert' to an index to keep the tests passing.

test_text.py

• Same issue with 'insert' as test_rstrip

Should I be submitting multiple patches for these changes or is it ok to combine them? I was also wondering if it would be a good idea to split up the FormatParagraph tests into separate tests.

[philwebster]

After reading Terry's comments on the initial patch I turned FormatParagraph's initial comments into a docstring and made sure that all lines were < 80 characters.

[terryjreedy]

What I think still needs to be done.

• FormatParagraph.py:

As near as I can tell from the patch, the comments are correct except that one is needed for reformat_paragraph.

I gather that the substantive code change allows reformatting of a comment block when it is properly selected (with the beginning at the beginning of the line). The proof will be in the testing.

The middle of format_paragraph_event currently looks like

    if comment_header:
        <bunch of no-gui code>
    else:
        newdata = reformat_paragraph(data, maxformatwidth)

where reformat_paragraph is no-gui code. The comment formatting code should be pulled out into a no-gui reformat_comment function (with docstring) and replaced with in its current location with
newdata = reformat_comment(data, maxformatwidth, comment_header).

• test_formatparagraph.py:

As I hinted before, the test file should begin with the simplest functions and those that do not require widgets (or mocks thereof). In the initial idle test issue bpo-15392, Nick recommended "As much as possible, push logic testing down into the non-GUI tests." This is easiet when non-gui logic and widget manipulation are isolated in different function. When logic has already been pushed into non-gui functions, they should normally be tested first.

Is_Get_Test (with methods for is_ and get_ functions)
ReformatCommentTest
ReformatParagraphTest
would be straightforward text test cases.

FindTest
FormatEventTest (split into multiple test cases)
require widgets.

If the helper functions do not work, the functions that use them will not work. So FormatEventTest does not really test the method unless all the other tests pass.

For this file, there is the additional factor that the latter test cases (or groups of test cases) require Text methods that do not currently exist and will not be added immediately. See my message on bpo-18425, as well as below. So they should first be implemented using the tk Text widget and requires('gui').

An advantage of starting with real widgets is that it separates problems with the tested and test files from problems with the mocks. Remember that mocks are only needed for the buildbots, not our desktops with graphics screens.

I have not yet looked at the individual tests, but please do a patch for only these two files.

[terryjreedy]

I am dubious that the current code will work with tkinter. Marks name slice positions. Tags name 0 to many pairs of slice positions, with 'sel' predefined as corresponding to a mouse selection and limited to one pair. According to my tests, tag_add('sel.first', None) adds a *new* tag that has nothing to do with the 'sel' selection tag. Tag.first and tag.last, if not used themselves as tag names, refer to the first and second slice positions of the first pair. Idle only uses 'sel.first' and 'sel.last', which simplifies what we need in Text._decode.

Experiments indicate that the two sel marks can be set with mark_set, in the sense that the internal structures are set even though the editor screen is not affected:

>>> t.mark_set('sel.first', 1.0)
>>> t.mark_set('sel.last', 1.3)
>>> e. get_selection_indices()
('1.0', '1.3')

'Find in files' indicates that Idle never does this. Except for two instances, the only three marks set are 'insert', 'iomark', and 'my_anchor', which again simplifies mock Text.

Once tests are running with Tk, we can comment out the tk use code (see test_rstrip.py) and add to the mocks what is either needed, or appears reusable for other tests, to make these tests pass again.

For instance, we could subclass mock_tk.Test to add test-specific methods with either 'pass' or minimal specialized code needed.

*mock_idle.Editor:

To use the subclass of Test, one option would be a subclass of mock_idle.Editor that uses the subclass of Text.

Editor.__init__.

Or we could add a parameter text_class to init that defaults to mock_tk.Text, so we can inject a subclass thereof without subclassing Editor.

The two undo_block lines are ok but irrelevant until we can use mock test. You might have mentioned that they are copied from EditorWindow. (The reason they work is that EditorWindow().text is not tkinter.Text() but a Python wrapper thereof.)

The purpose of get_selection_indices is to report the endpoints of the user-selected slice of text. Normal users do so with the mouse. We will have to do so by directly setting the return values. If I did not want to be able to run the rests with tk, the following would be sufficient.

    _selection = ('', '')
    def get_selection_indices(self):
        return self._selection
# with test code setting editor._selection = ('u.v', 'x.y')

However, since that will not work with tk, which as near as I can tell, requires text.mark_set.

mock_tk.Text:

Any (non-trivial) changes to this and its tests should be a separate issue or at least a separate patch.

In any case, the patch is wrong as it is. So are the ones for test_rstrip.py and test_text.py.

[philwebster]

I added and rewrote tests to use the Text widget (when necessary). The only changes made to FormatParagraph are documentation/comment formatting and the if __main__ function. I will work on the comment header behavior separately and can post the patch in this issue. Thanks for your guidance Terry!