Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDLE Unit test for RstripExtension.py #62479

Closed
philwebster mannequin opened this issue Jun 21, 2013 · 15 comments
Closed

IDLE Unit test for RstripExtension.py #62479

philwebster mannequin opened this issue Jun 21, 2013 · 15 comments
Assignees
Labels
topic-IDLE type-feature A feature request or enhancement

Comments

@philwebster
Copy link
Mannequin

philwebster mannequin commented Jun 21, 2013

BPO 18279
Nosy @terryjreedy, @rovitotv
Files
  • test_rstripext.patch
  • test_rstrip.patch: Revised patch with no mock EditorWindow and new name
  • test_rstrip2.patch: New mock IDLE classes and rstrip extension test
  • test_rstrip3.patch
  • 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-07-13.06:42:29.448>
    created_at = <Date 2013-06-21.22:37:53.663>
    labels = ['expert-IDLE', 'type-feature']
    title = 'IDLE Unit test for RstripExtension.py'
    updated_at = <Date 2013-07-13.06:42:29.447>
    user = 'https://bugs.python.org/philwebster'

    bugs.python.org fields:

    activity = <Date 2013-07-13.06:42:29.447>
    actor = 'terry.reedy'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2013-07-13.06:42:29.448>
    closer = 'terry.reedy'
    components = ['IDLE']
    creation = <Date 2013-06-21.22:37:53.663>
    creator = 'philwebster'
    dependencies = []
    files = ['30664', '30694', '30777', '30852']
    hgrepos = []
    issue_num = 18279
    keywords = ['patch']
    message_count = 15.0
    messages = ['191618', '191627', '191629', '191821', '192314', '192325', '192329', '192611', '192993', '192998', '193000', '193002', '193003', '193005', '193006']
    nosy_count = 5.0
    nosy_names = ['terry.reedy', 'Todd.Rovito', 'python-dev', 'JayKrish', 'philwebster']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue18279'
    versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

    @philwebster
    Copy link
    Mannequin Author

    philwebster mannequin commented Jun 21, 2013

    This is a single test for RstripExtension.py, following from bpo-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 bpo-18189, but saw the same message. Any thoughts?

    @philwebster philwebster mannequin added topic-IDLE type-feature A feature request or enhancement labels Jun 21, 2013
    @terryjreedy
    Copy link
    Member

    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.

    @philwebster
    Copy link
    Mannequin Author

    philwebster mannequin commented Jun 22, 2013

    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!

    @philwebster
    Copy link
    Mannequin Author

    philwebster mannequin commented Jun 24, 2013

    Modified the first patch to get rid of mock EditorWindow in favor of the real thing. Also renamed the test to 'test_rstrip'.

    @philwebster
    Copy link
    Mannequin Author

    philwebster mannequin commented Jul 4, 2013

    Added to Terry's Text Widget code (in bpo-18226) and created mock_idle.py for the mock EditorWindow. Todd's FormatParagraph test in the aforementioned issue also passes with the mock EditorWindow.

    @terryjreedy
    Copy link
    Member

    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?

    @terryjreedy
    Copy link
    Member

    I decided that the first commit should be a separate issue that this issue depends on.

    @terryjreedy terryjreedy self-assigned this Jul 5, 2013
    @philwebster
    Copy link
    Mannequin Author

    philwebster mannequin commented Jul 8, 2013

    This patch contains mock_idle.py and the rstrip test using the mock text widget from bpo-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?

    @terryjreedy
    Copy link
    Member

    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.

    @rovitotv
    Copy link
    Mannequin

    rovitotv mannequin commented Jul 13, 2013

    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

    @terryjreedy
    Copy link
    Member

    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.

    @terryjreedy
    Copy link
    Member

    Trying it out, I rediscovered that patchcheck has a Windows bug. This time I reported it ;-) bpo-18439.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 13, 2013

    New changeset ec71fcdcfeac by Terry Jan Reedy in branch '2.7':
    Issue bpo-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 bpo-18279: Add tests for idlelib/RstripExtension.py. Original patch by
    http://hg.python.org/cpython/rev/22ce68d98345

    @terryjreedy
    Copy link
    Member

    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.

    @terryjreedy
    Copy link
    Member

    I had to remove the dependency to close this, since the test_text issue bpo-18365 was reopened.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-IDLE type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant