test_lazy_imports.py fails

maksverver commented 2 months ago

Note that the failing tests are marked slow so they are not run by default. Maybe the tests are just out of date? Or is there a real problem here?

$ PYTHONPATH=src/ pytest --runslow -vv tests/test_lazy_imports.py 
================================================================================== test session starts ==================================================================================
platform linux -- Python 3.12.5, pytest-8.3.2, pluggy-1.5.0 -- /usr/bin/python
cachedir: .pytest_cache
rootdir: /home/maks/Code/reader
configfile: pyproject.toml
plugins: typeguard-4.3.0, subtests-0.12.1, requests-mock-1.11.0
collected 5 items                                                                                                                                                                       

tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_NO_IMPORTS] PASSED                                                                                          [ 20%]
tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_ADD_HTTP] FAILED                                                                                            [ 40%]
tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_UPDATE_FEEDS] FAILED                                                                                        [ 60%]
tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_UPDATE_FEEDS_WORKERS] FAILED                                                                                [ 80%]
tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_UPDATE_SEARCH] FAILED                                                                                       [100%]

======================================================================================= FAILURES ========================================================================================
__________________________________________________________________ test_only_expected_modules_are_imported[S_ADD_HTTP] __________________________________________________________________

code = "reader.add_feed('http://example.com')", expected_modules = {'requests', 'urllib.request'}

    @parametrize_dict('code, expected_modules', SNIPPETS)
    def test_only_expected_modules_are_imported(code, expected_modules):
        modules = set(get_imported_modules(code))
        actual_modules = LAZY_MODULES & modules
        # sanity check
        assert 'reader' in modules
>       assert actual_modules == expected_modules, expected_modules
E       AssertionError: {'requests', 'urllib.request'}
E       assert frozenset({'requests', 'feedparser', 'urllib.request'}) == {'requests', 'urllib.request'}
E         
E         Extra items in the left set:
E         'feedparser'
E         
E         Full diff:
E         - {
E         + frozenset({
E         +     'feedparser',
E               'requests',
E               'urllib.request',
E         - }
E         + })

tests/test_lazy_imports.py:125: AssertionError
________________________________________________________________ test_only_expected_modules_are_imported[S_UPDATE_FEEDS] ________________________________________________________________

code = 'reader.update_feeds()', expected_modules = {'requests', 'urllib.request'}

    @parametrize_dict('code, expected_modules', SNIPPETS)
    def test_only_expected_modules_are_imported(code, expected_modules):
        modules = set(get_imported_modules(code))
        actual_modules = LAZY_MODULES & modules
        # sanity check
        assert 'reader' in modules
>       assert actual_modules == expected_modules, expected_modules
E       AssertionError: {'requests', 'urllib.request'}
E       assert frozenset({'requests', 'feedparser', 'urllib.request'}) == {'requests', 'urllib.request'}
E         
E         Extra items in the left set:
E         'feedparser'
E         
E         Full diff:
E         - {
E         + frozenset({
E         +     'feedparser',
E               'requests',
E               'urllib.request',
E         - }
E         + })

tests/test_lazy_imports.py:125: AssertionError
____________________________________________________________ test_only_expected_modules_are_imported[S_UPDATE_FEEDS_WORKERS] ____________________________________________________________

code = 'reader.update_feeds(workers=2)', expected_modules = {'concurrent.futures', 'requests', 'urllib.request'}

    @parametrize_dict('code, expected_modules', SNIPPETS)
    def test_only_expected_modules_are_imported(code, expected_modules):
        modules = set(get_imported_modules(code))
        actual_modules = LAZY_MODULES & modules
        # sanity check
        assert 'reader' in modules
>       assert actual_modules == expected_modules, expected_modules
E       AssertionError: {'concurrent.futures', 'requests', 'urllib.request'}
E       assert frozenset({'requests', 'feedparser', 'urllib.request', 'concurrent.futures'}) == {'requests', 'concurrent.futures', 'urllib.request'}
E         
E         Extra items in the left set:
E         'feedparser'
E         
E         Full diff:
E         - {
E         + frozenset({
E               'concurrent.futures',
E         +     'feedparser',
E               'requests',
E               'urllib.request',
E         - }
E         + })

tests/test_lazy_imports.py:125: AssertionError
_______________________________________________________________ test_only_expected_modules_are_imported[S_UPDATE_SEARCH] ________________________________________________________________

code = "from reader._types import EntryData, EntryUpdateIntent\nfrom datetime import datetime, timezone\nreader.add_feed('one...ry='summary')\nreader._storage.add_or_update_entry(EntryUpdateIntent(entry, dt, dt, dt, dt))\nreader.update_search()\n"
expected_modules = {'bs4', 'urllib.request'}

    @parametrize_dict('code, expected_modules', SNIPPETS)
    def test_only_expected_modules_are_imported(code, expected_modules):
        modules = set(get_imported_modules(code))
        actual_modules = LAZY_MODULES & modules
        # sanity check
        assert 'reader' in modules
>       assert actual_modules == expected_modules, expected_modules
E       AssertionError: {'bs4', 'urllib.request'}
E       assert frozenset({'bs4'}) == {'urllib.request', 'bs4'}
E         
E         Extra items in the right set:
E         'urllib.request'
E         
E         Full diff:
E         - {
E         + frozenset({
E               'bs4',
E         + })
E         -     'urllib.request',
E         - }

tests/test_lazy_imports.py:125: AssertionError
================================================================================ short test summary info ================================================================================
FAILED tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_ADD_HTTP] - AssertionError: {'requests', 'urllib.request'}
assert frozenset({'requests', 'feedparser', 'urllib.request'}) == {'requests', 'urllib.request'}

  Extra items in the left set:
  'feedparser'

  Full diff:
  - {
  + frozenset({
  +     'feedparser',
        'requests',
        'urllib.request',
  - }
  + })
FAILED tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_UPDATE_FEEDS] - AssertionError: {'requests', 'urllib.request'}
assert frozenset({'requests', 'feedparser', 'urllib.request'}) == {'requests', 'urllib.request'}

  Extra items in the left set:
  'feedparser'

  Full diff:
  - {
  + frozenset({
  +     'feedparser',
        'requests',
        'urllib.request',
  - }
  + })
FAILED tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_UPDATE_FEEDS_WORKERS] - AssertionError: {'concurrent.futures', 'requests', 'urllib.request'}
assert frozenset({'requests', 'feedparser', 'urllib.request', 'concurrent.futures'}) == {'requests', 'concurrent.futures', 'urllib.request'}

  Extra items in the left set:
  'feedparser'

  Full diff:
  - {
  + frozenset({
        'concurrent.futures',
  +     'feedparser',
        'requests',
        'urllib.request',
  - }
  + })
FAILED tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_UPDATE_SEARCH] - AssertionError: {'bs4', 'urllib.request'}
assert frozenset({'bs4'}) == {'urllib.request', 'bs4'}

  Extra items in the right set:
  'urllib.request'

  Full diff:
  - {
  + frozenset({
        'bs4',
  + })
  -     'urllib.request',
  - }
============================================================================== 4 failed, 1 passed in 0.96s ==============================================================================

lemon24 commented 2 months ago

I triggered a CI run, and all tests pass successfully, including the S_UPDATE_SEARCH one. I also can't repro locally using the same command.

Can you please provide a bit more info about the commit used and your environment?

git log --oneline -1
env | grep -e ^READER -e ^PY
pip freeze

maksverver commented 2 months ago

Happens both at head and with the 3.14 release (which is really recent anyway).

$ git log --oneline -1
f92e592 (HEAD -> master, origin/master, origin/HEAD) Sphinx: :noindex: -> :no-index:

$ env | grep -e ^READER -e ^PY
(outputs nothing)

$ pip freeze
pip: command not found

maksverver commented 2 months ago

I forgot to mention I removed the vendored feedparser implementation, because it broke the majority of test_parser.py (reader.exceptions.ParseError: unknown feed type: 'http://example.com/empty.rss'), which might be worth a bug report of its own.

That explains the extra feedparser imports, which are reader._vendor.feedparser in the unmodified source tree , but the latter isn't defined in LAZY_MODULES so the test ignores them. That's an actionable bugfix right there :)

With the vendored feedparser only S_UPDATE_SEARCH fails:

$ PYTHONPATH=src/ pytest --runslow -vv tests/test_lazy_imports.py 
================================================================================== test session starts ==================================================================================
platform linux -- Python 3.12.5, pytest-8.3.2, pluggy-1.5.0 -- /usr/bin/python
cachedir: .pytest_cache
rootdir: /home/maks/tmp/reader-clean
configfile: pyproject.toml
plugins: typeguard-4.3.0, subtests-0.12.1, requests-mock-1.11.0
collected 5 items                                                                                                                                                                       

tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_NO_IMPORTS] PASSED                                                                                          [ 20%]
tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_ADD_HTTP] PASSED                                                                                            [ 40%]
tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_UPDATE_FEEDS] PASSED                                                                                        [ 60%]
tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_UPDATE_FEEDS_WORKERS] PASSED                                                                                [ 80%]
tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_UPDATE_SEARCH] FAILED                                                                                       [100%]

======================================================================================= FAILURES ========================================================================================
_______________________________________________________________ test_only_expected_modules_are_imported[S_UPDATE_SEARCH] ________________________________________________________________

code = "from reader._types import EntryData, EntryUpdateIntent\nfrom datetime import datetime, timezone\nreader.add_feed('one...ry='summary')\nreader._storage.add_or_update_entry(EntryUpdateIntent(entry, dt, dt, dt, dt))\nreader.update_search()\n"
expected_modules = {'bs4', 'urllib.request'}

    @parametrize_dict('code, expected_modules', SNIPPETS)
    def test_only_expected_modules_are_imported(code, expected_modules):
        modules = set(get_imported_modules(code))
        actual_modules = LAZY_MODULES & modules
        # sanity check
        assert 'reader' in modules
>       assert actual_modules == expected_modules, expected_modules
E       AssertionError: {'bs4', 'urllib.request'}
E       assert frozenset({'bs4'}) == {'bs4', 'urllib.request'}
E         
E         Extra items in the right set:
E         'urllib.request'
E         
E         Full diff:
E         - {
E         + frozenset({
E               'bs4',
E         + })
E         -     'urllib.request',
E         - }

tests/test_lazy_imports.py:125: AssertionError
================================================================================ short test summary info ================================================================================
FAILED tests/test_lazy_imports.py::test_only_expected_modules_are_imported[S_UPDATE_SEARCH] - AssertionError: {'bs4', 'urllib.request'}
assert frozenset({'bs4'}) == {'bs4', 'urllib.request'}

  Extra items in the right set:
  'urllib.request'

  Full diff:
  - {
  + frozenset({
        'bs4',
  + })
  -     'urllib.request',
  - }
============================================================================== 1 failed, 4 passed in 1.06s ==============================================================================

If I understand correctly, the test complains that it expects urllib.request to be imported, but it isn't. That doesn't sound like a bad thing? Can you check where that import comes from in your version of the code?

lemon24 commented 2 months ago

I forgot to mention I removed the vendored feedparser implementation

I guess that will do it :)

because it broke the majority of test_parser.py ... which might be worth a bug report of its own.

That's strange, given CI succeeds (but then again, that's only Ubuntu). If you are ok with opening an issue for this (thank you!), I'm interested in:

the OS
whether you are using a virtual env or not
whether you are using system packages or not in the virtual env
what packages+versions are installed
- not sure why pip freeze doesn't work; if you are using pip, python -m pip freeze or python -m pip freeze may be worth trying
- if that doesn't work, this ought to do it (should be package manager agnostic)
```
python -c '\
import importlib.metadata as im
for d in im.distributions():
print(d.name, d.version)
' | sort > dists.txt
```

If there's no fix for the vendored feedparser issue, I should probably add a way to opt out of it.

lemon24 commented 2 months ago

but [reader._vendor.feedparser] isn't defined in LAZY_MODULES so the test ignores them

Indeed, will fix this.

assert frozenset({'bs4'}) == {'bs4', 'urllib.request'}

Not sure why this fails (may be due to bs4 importing urllib.request differently on your system, there's a comment in the tests about that being brittle :).

I think relaxing the == to <= should be an acceptable fix for this (we care more about slow stuff being imported by accident, if it's not imported it shouldn't matter).

maksverver commented 2 months ago

@lemon24 your latest commits fix the problem (thanks!) Should I close the issue? Or is there a reason you left it open?

lemon24 commented 2 months ago

I was waiting for confirmation, thank you!

lemon24 / reader

test_lazy_imports.py fails #349