Skip to content

gh-132950: Skip test_remote_pdb if remote exec is disabled #132951

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

Merged
merged 1 commit into from
Apr 25, 2025

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Apr 25, 2025

Skip test_keyboard_interrupt() if remote execution is disabled.

@gaogaotiantian
Copy link
Member

I don't think this is the correct way to fix it. If remote debugging is not supported, this whole test file should be skipped.

@gaogaotiantian
Copy link
Member

Do we have a flag somewhere about whether sys.remote_exec is supported? For now, we can do a quick test at the beginning of the module and skip the module if that's not supported.

@vstinner
Copy link
Member Author

I don't think this is the correct way to fix it. If remote debugging is not supported, this whole test file should be skipped.
Do we have a flag somewhere about whether sys.remote_exec is supported? For now, we can do a quick test at the beginning of the module and skip the module if that's not supported.

This change is a practical change for the current API. Currently, the only way to check if remote debugging is supported is to actually call sys.remote_exec().

Accepted https://peps.python.org/pep-0768/ only adds sys.remote_exec().

@vstinner
Copy link
Member Author

cc @pablogsal

@gaogaotiantian
Copy link
Member

Currently maybe the only test that really uses sys.remote_exec is this interrupt one, but the whole remote pdb feature is built on sys.remote_exec. Without it, it just won't work.

Having a specific check in _send_interrupt won't last once we add more tests (and we plan to do that very soon). We could run a quick foo test with sys.remote_exec() and test if this feature is supported, then skip the whole test if it's not. That could be a temporary solution before we have a flag to indicate whether it's supported.

@pablogsal
Copy link
Member

There is a macro to detect if remote_exec is supported. We can expose the macro somewhere (either the sys module or some other place). Ideas?

@pablogsal
Copy link
Member

Ah, I am forgetting I added this for this precise purpose:

>>> sys.is_remote_debug_enabled()
True

@pablogsal
Copy link
Member

It needs #132959 first

@vstinner
Copy link
Member Author

There is a macro to detect if remote_exec is supported. We can expose the macro somewhere (either the sys module or some other place). Ideas?

If you dislike my approach, I suggest adding sys._remote_exec_supported() function.

Checking if it's supported is non-trivial. Pseudo-code:

int
_PySysRemoteDebug_Supported(void)
{
#if !defined(Py_SUPPORTS_REMOTE_DEBUG)
    return 0;
#elif !defined(Py_REMOTE_DEBUG)
    return 0;
#else
    PyThreadState *tstate = _PyThreadState_GET();
    const PyConfig *config = _PyInterpreterState_GetConfig(tstate->interp);
    return (config->remote_debug == 1);
#endif
}

@pablogsal
Copy link
Member

There is a macro to detect if remote_exec is supported. We can expose the macro somewhere (either the sys module or some other place). Ideas?

If you dislike my approach, I suggest adding sys._remote_exec_supported() function.

Checking if it's supported is non-trivial. Pseudo-code:

int
_PySysRemoteDebug_Supported(void)
{
#if !defined(Py_SUPPORTS_REMOTE_DEBUG)
    return 0;
#elif !defined(Py_REMOTE_DEBUG)
    return 0;
#else
    PyThreadState *tstate = _PyThreadState_GET();
    const PyConfig *config = _PyInterpreterState_GetConfig(tstate->interp);
    return (config->remote_debug == 1);
#endif
}

We already have it:

cpython/Python/sysmodule.c

Lines 2433 to 2449 in 17718b0

/*[clinic input]
sys.is_remote_debug_enabled
Return True if remote debugging is enabled, False otherwise.
[clinic start generated code]*/
static PyObject *
sys_is_remote_debug_enabled_impl(PyObject *module)
/*[clinic end generated code: output=7ca3d38bdd5935eb input=7335c4a2fe8cf4f3]*/
{
#ifndef Py_REMOTE_DEBUG
Py_RETURN_FALSE;
#else
const PyConfig *config = _Py_GetConfig();
return PyBool_FromLong(config->remote_debug);
#endif
}

@vstinner
Copy link
Member Author

Ok, I rewrote my PR to use sys.is_remote_debug_enabled(). I chose to skip the only test which uses sys.remote_exec(): test_keyboard_interrupt().

@vstinner
Copy link
Member Author

Example:

$ PYTHON_DISABLE_REMOTE_DEBUG=1 ./python -m test -v test_remote_pdb -m test_keyboard_interrupt
(...)
test_keyboard_interrupt (test.test_remote_pdb.PdbConnectTestCase.test_keyboard_interrupt)
Test that sending keyboard interrupt breaks into pdb. ... skipped 'remote debugging is disabled'
(...)

@vstinner vstinner changed the title gh-132950: Skip test_remote_pdb on FreeBSD gh-132950: Skip test_remote_pdb if remote exec is disabled Apr 25, 2025
@vstinner
Copy link
Member Author

@gaogaotiantian: Please review the updated PR.

@gaogaotiantian
Copy link
Member

I still think we should skip the whole test file. Like I said, remote debugging does not exist if sys.remote_exec is not supported. It's pointless to test the mocking stuff. Also, we will have other tests that requires sys.remote_exec to execute - any integration test that actually attaches to the process (not just mock). I don't think we lose any coverage to simply skip this module when remote_exec() is not supported. Just raise a SkipTest at module level if it's not supported like test_pty.py.

@vstinner
Copy link
Member Author

I still think we should skip the whole test file.

Ok, I updated my PR to do that.

Sorry, I don't know this code, so I tried to write the smallest change :-)

@vstinner vstinner enabled auto-merge (squash) April 25, 2025 17:27
@vstinner vstinner merged commit 947c4f1 into python:main Apr 25, 2025
42 checks passed
@vstinner vstinner deleted the skip_remote_pdb branch April 25, 2025 17:28
@gaogaotiantian
Copy link
Member

Sorry, I don't know this code, so I tried to write the smallest change :-)

It makes sense to start with the smallest change :) In this specific case I think skipping the file is the way to go. This looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants