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

test_dis should test the dis module, not everything else #90916

Open
markshannon opened this issue Feb 15, 2022 · 7 comments
Open

test_dis should test the dis module, not everything else #90916

markshannon opened this issue Feb 15, 2022 · 7 comments
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@markshannon
Copy link
Member

BPO 46760
Nosy @terryjreedy, @markshannon, @JelleZijlstra, @brandtbucher, @iritkatriel
PRs
  • bpo-46760: Remove unnecessary offsets from expected out in test.test_dis #31369
  • 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 = None
    created_at = <Date 2022-02-15.14:17:08.858>
    labels = ['type-feature', 'tests']
    title = 'test_dis should test the dis module, not everything else'
    updated_at = <Date 2022-02-18.21:38:39.147>
    user = 'https://github.com/markshannon'

    bugs.python.org fields:

    activity = <Date 2022-02-18.21:38:39.147>
    actor = 'terry.reedy'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Tests']
    creation = <Date 2022-02-15.14:17:08.858>
    creator = 'Mark.Shannon'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 46760
    keywords = ['patch']
    message_count = 2.0
    messages = ['413291', '413514']
    nosy_count = 5.0
    nosy_names = ['terry.reedy', 'Mark.Shannon', 'JelleZijlstra', 'brandtbucher', 'iritkatriel']
    pr_nums = ['31369']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue46760'
    versions = []

    @markshannon
    Copy link
    Member Author

    markshannon commented Feb 15, 2022

    <rant>

    This is getting really annoying.
    It takes longer to fix all the heavily coupled and poorly written tests in test_dis than to make the real changes.

    Tiny changes in the calling sequence, or reordering CFGs, cause huge diffs in the test_dis module.
    No one ever checks these changes, they are just noise.

    I've put this under "enhancement" as there is no "wastes a huge amount of time" category.

    </rant>

    The test_dis should not:

    • Contain offsets; they turn one line diffs into 100 line diffs.
    • Contain tests for the compiler; they belong elsewhere.
    • Contain big strings; write proper tests not just regex matches.
    • Tests for Instruction should should not depend on the compiler output; create the bytecode directly.

    This is not a new problem, but it does seem to be getting progressively worse.

    A lot of the irritation stems from
    b39fd0c
    although the tests from before that still hardcode offsets.

    @markshannon markshannon added tests Tests in the Lib/test dir type-feature A feature request or enhancement labels Feb 15, 2022
    @terryjreedy
    Copy link
    Member

    With IDLE, I have issues with trying to test IDLE code without retesting tkinter, as well as deciding on the proper units of behavior to test.

    Some suggestions:

    1. Add a docstring to the module with guidelines, after review from a couple of others.
      For instance,I believe you are saying that no test should explicitly call compile. Rather the test writer should call compile and extract the bytecode to copy into a test.

    2. Make yourself a module code owner so you get informed and can review. Reviewing would be easier with guidelines to refer to, instead of repeating them.

    3. An example issue is that some callables take many types of arguments. If not already, tests that the function can extract the bytecode from various objects should be separate from tests that extracted bytecode is then handled properly. Would any internal refactoring of dis and addition of _test functions make it easier to make test more independent of each other?

    4. I would rather read the multiple long lists like

    expected_opinfo_outer = [
      Instruction(opname='LOAD_CONST', opcode=100, arg=1, argval=3, argrepr='3', offset=0, starts_line=2, is_jump_target=False),
      Instruction(opname='LOAD_CONST', opcode=100, arg=2, argval=4, argrepr='4', offset=3, starts_line=None, is_jump_target=False),
      ...
    ]

    condensed to

    expected_opinfo_outer = [Instruction(opname, opcode, arg, argval, argrepr,
                                         offset, starts_line, is_jumps_target)
        for opname, opcode, arg, argval, argrepr, offset, starts_line, is_jumps_target in
    (('LOAD_CONST', 100,    1,   3,     '3',      0,      2,           False),
     ('LOAD_CONST', 100,    2,   4,     '4',      3,      None,        False),
     ...
    )]

    @martindemello
    Copy link
    Contributor

    i'm interested in working on this (and refactoring dis to support better testability if needed). i like @terryjreedy 's ideas in general so i can start by implementing some of them.

    @iritkatriel
    Copy link
    Member

    I think the most complex part of this issue is to make sure that the things that are covered by test_dis (and which do not belong there) are covered by other tests. Only then we can reduce test_dis to the right scope.

    martindemello added a commit to martindemello/cpython that referenced this issue Sep 1, 2022
    …test_dis
    
    * Use a table with aligned elements rather than a list of
      `Instruction(key=value, ...)` constructors
    * Don't specify `positions=None` since it never changes.
    * Shorten overly-long string literals in test code.
    * Fix the one test that depends on its own line number.
    @terryjreedy
    Copy link
    Member

    terryjreedy commented Sep 1, 2022

    I added checkboxes to Mark's todo list in the initial post. The first item is already fixed.

    @martindemello
    Copy link
    Contributor

    there are still offsets in the Instruction(...) lists; I'll get rid of them in the next PR, so that this one can remain a pure refactoring.

    @martindemello
    Copy link
    Contributor

    by this:

    Contain big strings; write proper tests not just regex matches.

    do you mean that we should always be matching the structured output of _get_instruction_bytes and not the formatted string output of _disassemble_bytes?

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants