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

Use proper command line parsing in _testembed #69120

Closed
ncoghlan opened this issue Aug 25, 2015 · 9 comments
Closed

Use proper command line parsing in _testembed #69120

ncoghlan opened this issue Aug 25, 2015 · 9 comments
Assignees
Labels
3.7 (EOL) end of life tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

BPO 24932
Nosy @brettcannon, @ncoghlan, @vstinner, @encukou, @ericsnowcurrently, @zooba, @stratakis
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • Files
  • 24932_1.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/zooba'
    closed_at = <Date 2017-01-02.06:12:48.498>
    created_at = <Date 2015-08-25.05:51:28.296>
    labels = ['3.7', 'type-feature', 'tests']
    title = 'Use proper command line parsing in _testembed'
    updated_at = <Date 2019-05-18.17:11:29.999>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2019-05-18.17:11:29.999>
    actor = 'eric.snow'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2017-01-02.06:12:48.498>
    closer = 'steve.dower'
    components = ['Tests']
    creation = <Date 2015-08-25.05:51:28.296>
    creator = 'ncoghlan'
    dependencies = []
    files = ['46093']
    hgrepos = []
    issue_num = 24932
    keywords = ['patch']
    message_count = 9.0
    messages = ['249106', '249134', '284338', '284360', '284456', '284457', '284468', '293110', '293281']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'vstinner', 'petr.viktorin', 'python-dev', 'eric.snow', 'steve.dower', 'cstratak']
    pr_nums = ['552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue24932'
    versions = ['Python 3.7']

    @ncoghlan
    Copy link
    Contributor Author

    Programs/_testembed (invoked by test_capi to test CPython's embedding support) is currently a very simple application with only two different embedding tests: https://hg.python.org/cpython/file/tip/Programs/_testembed.c

    In light of proposals like PEP-432 to change the interpreter startup sequence and make it more configurable, it seems desirable to be better able to test more configuration options directly, without relying on the abstraction layer provided by the main CPython executable.

    The specific unit testing library that prompted this idea was cmocka, which is used by libssh, sssd and cwrap: https://cmocka.org/

    cwrap in turn is used by the Samba team to mock out network interfaces and other operations using LD_PRELOAD: https://cwrap.org/

    We don't necessarily have to use those particular libraries, I'm just filing this issue to capture the idea of improving our C level unit testing capabilities, and then updating regrtest to better capture and report those results (currently there are just a couple of tests in test_capi that call the _testembed executable)

    @ncoghlan ncoghlan added tests Tests in the Lib/test dir type-feature A feature request or enhancement labels Aug 25, 2015
    @brettcannon
    Copy link
    Member

    Someone is going to think of [googletest] (https://github.com/google/googletest) and then realize that it is a C++ test suite and thus won't work unless you explicitly compile Python for C++.

    @zooba
    Copy link
    Member

    zooba commented Dec 30, 2016

    The only real advantage of adding a native unit testing framework here is to avoid having to start/destroy _testembed[.exe] multiple times during the test run. But given the nature of these tests is highly environmental, I don't think we can reasonably avoid it. I'm also highly doubtful that any framework is going to actually reduce the work we'd need to do to mock out initialization steps.

    I've attached a patch that takes the first step towards making _testembed more usable, by changing main() to look up a test from a list and execute it, then return the exit code. The support in test_capi.EmbeddingTests is neat, so adding more tests here will be fairly straightforward.

    @zooba zooba added the 3.7 (EOL) end of life label Dec 30, 2016
    @ncoghlan ncoghlan changed the title Migrate _testembed to a C unit testing library Use proper command line parsing in _testembed Dec 31, 2016
    @ncoghlan
    Copy link
    Contributor Author

    Steve's patch LGTM, and I now agree our needs are simple enough that we can rely on subprocess+unittest for the result reporting.

    I'd just missed Steve's simple solution of using the return code to indicate success or failure, as well as Eric's approach in bpo-29102 of teaching the related Python test case to read the text output from the _testembed test case.

    If we eventually want to go beyond the single "run this test case" argument, then adding getopt() support to _testembed should be sufficient.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 2, 2017

    New changeset 69b2a6284f3d by Steve Dower in branch 'default':
    Issue bpo-24932: Use proper command line parsing in _testembed
    https://hg.python.org/cpython/rev/69b2a6284f3d

    @zooba
    Copy link
    Member

    zooba commented Jan 2, 2017

    Patch committed without modification. I'll keep an eye on the buildbots just in case, though I may end up going offline before they get to this changeset.

    @zooba zooba self-assigned this Jan 2, 2017
    @zooba
    Copy link
    Member

    zooba commented Jan 2, 2017

    Buildbots are happy, so I'm calling this done. Jumping straight to getopt is overengineering right now in my opinion, but if there's a need for it with a future test we can consider it then.

    @zooba zooba closed this as completed Jan 2, 2017
    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented May 5, 2017

    Downstream backporting of PEP-538 to the 3.6 branch also depends on this fix.

    Would it be beneficial in any way to cherry-pick it for 3.6?

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented May 9, 2017

    Based on the latest round of PEP-538 review, the related test case that currently relies on _testembed is going to be able to be simplified to just setting LC_ALL=C when calling the subprocess (the amount of code it's letting me delete from the draft patch is further persuading me that changing the env var updates from "LC_ALL & LANG" to "LC_CTYPE & LANG" is the right call).

    That means that change will also eliminate the dependence on this test suite enhancement for backports.

    @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
    3.7 (EOL) end of life tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants