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

gh-91265: Make old ctypes.macholib tests runned by python -m test #32094

Merged
merged 5 commits into from
Apr 18, 2022

Conversation

arhadthedev
Copy link
Member

@arhadthedev arhadthedev commented Mar 24, 2022

Some tests for ctypes.macholib.dyld and all tests for ctypes.macholib.dylib and ctypes.macholib.framework are located outside of expected ctypes.test module and were not ported from assert to unittest facilities. This causes the following problems:

  • the tests aren't run by python -m test so they are effectively dead
  • an end user can run them by python -m ctypes.macholib.dyld, python -m ctypes.macholib.dylib, and python -m ctypes.macholib.framework
  • they are available even if a user chose to install CPython without tests

This PR moves these tests into ctypes.test.test_macholib suit and wraps them as unittest.TestCase.

I believe this PR should be backported to not export unintended features in unintended places.

Fixes GH-91265.

@arhadthedev arhadthedev changed the title Move tests from ctypes.macholib.dy* to ctypes.test Make old ctypes.macholib tests runned by python -m test Mar 24, 2022
@arhadthedev arhadthedev changed the title Make old ctypes.macholib tests runned by python -m test bpo-47109: Make old ctypes.macholib tests runned by python -m test Mar 24, 2022
@arhadthedev arhadthedev marked this pull request as ready for review March 24, 2022 12:04
@MaxwellDupre
Copy link
Contributor

After rebuild against this PR: tests:
cpython on  pr/32094 via 🐍 v3.11.0a6+
✦ ❯ ./python -m unittest -vvv Lib/ctypes/test/test_macholib.py
Ran 3 tests in 0.000s
OK (skipped=3)

Or
./python -m test -vvv test_macholib
0:00:00 load avg: 1.07 Run tests sequentially
0:00:00 load avg: 1.07 [1/1] test_macholib
test test_macholib crashed -- Traceback (most recent call last):
File "/home/me/Documents/cpython/Lib/test/libregrtest/runtest.py", line 352, in _runtest_inner
refleak = _runtest_inner2(ns, test_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/me/Documents/cpython/Lib/test/libregrtest/runtest.py", line 292, in _runtest_inner2
the_module = importlib.import_module(abstest)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/me/Documents/cpython/Lib/importlib/init.py", line 126, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "", line 1206, in _gcd_import
File "", line 1178, in _find_and_load
File "", line 1142, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'test.test_macholib'

@arhadthedev arhadthedev changed the title bpo-47109: Make old ctypes.macholib tests runned by python -m test gh-91265: Make old ctypes.macholib tests runned by python -m test Apr 10, 2022
@arhadthedev
Copy link
Member Author

Ping.

This PR doesn't add extra logic, it just collects stuff into Lib/ctypes/test/test_macholib.py and replaces assert with self.assertEqual/self.assertIsNone.

Copy link
Contributor

@ronaldoussoren ronaldoussoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally had some time to seriously look at this. The PR looks good to me, as the description says this moves some tests to explicit test files to ensure they are picked up by the regular test runs.

@ronaldoussoren
Copy link
Contributor

@arhadthedev : Sorry about the delay in reviewing, I had to find some time to seriously look at this and be sure that tests are correctly moved.

Did you sign the CLA? The CLA check is blocking the merge of your PR.

@ronaldoussoren ronaldoussoren self-assigned this Apr 17, 2022
@arhadthedev
Copy link
Member Author

Sorry about the delay in reviewing

No worries! The issue needed the chance to collect feedback anyway.

Did you sign the CLA? The CLA check is blocking the merge of your PR.

Yes, both on the bpo (https://bugs.python.org/user40922) and after the migration (#91453 (comment), I'm one of authors there). So it's just a matter of waiting for the bot to be sorted out.


P. S.: @ronaldoussoren (because I know that the "Mentioned" queue is an order of magnitude less that the "Participating" one).

@arhadthedev arhadthedev reopened this Apr 17, 2022
@arhadthedev
Copy link
Member Author

@ronaldoussoren I've reopened the PR to retrigger the bot; it worked.

@ronaldoussoren ronaldoussoren merged commit 804ea2d into python:main Apr 18, 2022
@arhadthedev arhadthedev deleted the macholib-move-tests branch April 18, 2022 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Old ctypes.macholib tests are ignored by python -m test
5 participants