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 - Test hyperparser #65885

Closed
SaimadhavHeblikar mannequin opened this issue Jun 7, 2014 · 13 comments
Closed

IDLE - Test hyperparser #65885

SaimadhavHeblikar mannequin opened this issue Jun 7, 2014 · 13 comments

Comments

@SaimadhavHeblikar
Copy link
Mannequin

SaimadhavHeblikar mannequin commented Jun 7, 2014

BPO 21686
Nosy @terryjreedy, @taleinat
Files
  • test-hyperparser.diff
  • test-hyperparser-v1.diff
  • hyperparser_update.diff
  • test-hp-34-push.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 = None
    closed_at = <Date 2014-06-16.23:17:46.141>
    created_at = <Date 2014-06-07.15:01:54.405>
    labels = ['expert-IDLE']
    title = 'IDLE - Test hyperparser'
    updated_at = <Date 2014-06-17.20:04:40.406>
    user = 'https://bugs.python.org/SaimadhavHeblikar'

    bugs.python.org fields:

    activity = <Date 2014-06-17.20:04:40.406>
    actor = 'terry.reedy'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-06-16.23:17:46.141>
    closer = 'terry.reedy'
    components = ['IDLE']
    creation = <Date 2014-06-07.15:01:54.405>
    creator = 'Saimadhav.Heblikar'
    dependencies = []
    files = ['35512', '35630', '35640', '35661']
    hgrepos = []
    issue_num = 21686
    keywords = ['patch']
    message_count = 13.0
    messages = ['219942', '220510', '220515', '220548', '220549', '220595', '220700', '220755', '220780', '220781', '220798', '220801', '220870']
    nosy_count = 5.0
    nosy_names = ['terry.reedy', 'taleinat', 'jesstess', 'python-dev', 'Saimadhav.Heblikar']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue21686'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @SaimadhavHeblikar
    Copy link
    Mannequin Author

    SaimadhavHeblikar mannequin commented Jun 7, 2014

    Test for idlelib.HyperParser
    5 lines not tested. Any suggestion on how to hit those lines welcome.
    Will submit backport 2.7 once the patch for 3.4 is OK.

    @SaimadhavHeblikar SaimadhavHeblikar mannequin added the topic-IDLE label Jun 7, 2014
    @taleinat
    Copy link
    Contributor

    Which lines are not hit?

    @terryjreedy
    Copy link
    Member

    There are 7 scattered conditionals which only evaluate as one of True or False during tests and 5 lines not hit at all as a result. I am emailing instructions to install and use coverage package.

    @taleinat
    Copy link
    Contributor

    Here are details how to trigger all of the uncovered code lines and branches, listed by line.

    Uncovered lines:

    Line 159: This catches would-be identifiers (variable names) which are keywords or begin with a character that can't be first (e.g. a digit). We should have at least one test for each of these cases.

    Lines 196-198: These strip comments from inside an expression. We should have a test with sample code with a multi-line expression with a comment inside it.

    Line 225: Triggered when trying to find an expression including parentheses or brackets when they aren't closed, e.g. "[x for x in ".

    Uncovered branches (except those which just cause the above):

    Lines 32, 42: To trigger, have a block of code over 50 lines long and create a HyperParser at the last line (not inside any other block).

    Line 236: Have a string literal inside brackets or parens, and call get_expression().

    @taleinat
    Copy link
    Contributor

    Regarding lines 32 & 42, a test could also set editwin.num_context_lines to allow triggering with smaller code blocks. It's value needs to be a list of integers, the last of which is incredibly large, perhaps sys.maxint.

    @terryjreedy
    Copy link
    Member

    Since ParenMatch and fixes* to HyperParser depend on this issue, I would like to do this first. For HyperParser.py, I would like to first commit a clean-up only patch. The test_hyperparser patch should then only add the 'if __name__' clause at the end.

    I would like to apply the attached tomorrow, pending comments. It
    adds docstrings and properly formats them per PEP-8; reformats comment blocks, and properly wrap lines without '\', eliminating occurences already present.

    The v1 tests continue to pass once 'before the analyzed' is changed in the test file to 'precedes'. 'Index' or 'statement' might be better targets.

    *After test_hyperparser.py is added, additional tests for bpo-21756 (finding closers) and new bpo-21765 -- making HyperParser work with non-ascii identifiers, will be added as part of any code fix patches.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 16, 2014

    New changeset 5063df721985 by Terry Jan Reedy in branch '2.7':
    Issue bpo-21686: idlelib/HyperParser.py - Update docstrings and comments and
    http://hg.python.org/cpython/rev/5063df721985

    New changeset ddf15cf0db72 by Terry Jan Reedy in branch '3.4':
    Issue bpo-21686: idlelib/HyperParser.py - Update docstrings and comments and
    http://hg.python.org/cpython/rev/ddf15cf0db72

    @terryjreedy
    Copy link
    Member

    Coverage is now '99%'. Unless I see problems that are more than trivial, I am going to polish the patch and commit.

    I believe partial coverage of "for context in editwin.num_context_lines:" is because the iterable is never empty. But it never will be in practice and I believe the code is not intended to work if it is. So I ignore this.

    I believe partial coverage of deeply nested
    if rawtext[pos] in "'\"":
    is because this is reached with rawtext[pop] *not* in "'\"". Reading up, to reach this point, rawtext[pos] must also not be in "([":,
    bracketing[brck_index][0] == brck_limit
    must never become true inside the while look (because it breaks), and pos == brck_limit must be true. I don't know if this is all possible. Tal, if you find a triggering test case after I commit, it can be added later.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 16, 2014

    New changeset 59730aecce9b by Terry Jan Reedy in branch '2.7':
    Issue bpo-21686: add unittest for idlelib.HyperParser. Original patch by Saimadhav
    http://hg.python.org/cpython/rev/59730aecce9b

    New changeset 2fc89d2230a2 by Terry Jan Reedy in branch '3.4':
    Issue bpo-21686: add unittest for idlelib.HyperParser. Original patch by Saimadhav
    http://hg.python.org/cpython/rev/2fc89d2230a2

    @terryjreedy
    Copy link
    Member

    Attached is the 3.4 patch as pushed. The main 'invisible' change, other than the review comment, was to create the code string just once.

    I discovered that the earlier version had a bug in saying 'ok' to the 3.x HyperParser bug of no longer recognizing 'False', etc, as identifiers due to being made keywords. The buggy test properly failed on 2.7. I did the easy fix and split the bad test into two.

    Since the 2.7 and 3.x versions of HyperParser were identical before the fix, the 3.x version must also not recognize the new ... Ellipsis literal. Tal, do you think doing so would be sensibly possible? (IE, would it be worth a new issue?)
    ...
    x = ...
    print(...)
    are all legal 3.x expressions.

    @taleinat
    Copy link
    Contributor

    Ouch. I hadn't thought about the Ellipsis literal!

    That would be sensibly possible, yes, but not simple. I think opening a separate issue for this would be prudent.

    @terryjreedy
    Copy link
    Member

    bpo-21787 Idle: make 3.x Hyperparser.get_expression recognize ...

    @terryjreedy
    Copy link
    Member

    I forgot to mention that with this particular shutdown order
    @classmethod
    def tearDownClass(cls):
    del cls.text, cls.editwin
    cls.root.destroy()
    del cls.root
    the shutdown warning message we have seen about not handling an event went away. Even though editwin is a dummy, it had to be deleted -- I suspect to delete its reference to the gui text widget.

    @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
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants