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

Bump pytest to 8.0.0 #3335

Closed
wants to merge 4 commits into from
Closed

Bump pytest to 8.0.0 #3335

wants to merge 4 commits into from

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Feb 1, 2024

@pquentin pquentin added the Skip Changelog Pull requests that don't require a changelog entry label Feb 1, 2024
@ecerulm
Copy link
Contributor

ecerulm commented Feb 1, 2024

Umm,

In current main I tested with pytest 7.4.4 and

nox -rs test-3.12 -- test/contrib/test_pyopenssl.py::TestHTTPS_TLSv1_3

passed.

With pytest 8.0.0 it fails:

FAILED test/contrib/test_pyopenssl.py::TestHTTPS_TLSv1_3::test_ssl_context_ssl_version_uses_ssl_min_max_versions - Failed: DID NOT WARN. No warnings of type (<class 'DeprecationWarning'>,) were emitted.

The changelog mentions some changes regarding pytest.warns() but the test should still work ... but it doesn't

@ecerulm
Copy link
Contributor

ecerulm commented Feb 1, 2024

With pytest 8.0.0

  • test/contrib/test_pyopenssl.py::TestHTTPS_TLSv1_3::test_ssl_context_ssl_version_uses_ssl_min_max_versions FAILS
  • test/contrib/test_pyopenssl.py::TestHTTPS_TLSv1_2::test_ssl_context_ssl_version_uses_ssl_min_max_versions WORKS

So I guess it's more related to pytest collect the test, since TestHTTPS_TLSv1_3 was meant to be SKIPPED with ssl.PROTOCOL_TLSv1_3 isn't available.

@ecerulm ecerulm mentioned this pull request Feb 1, 2024
@illia-v
Copy link
Member

illia-v commented Feb 27, 2024

I think we can bump the memory limit in test_get_all_memory_usage_single_chunk from 10.01 MB to 10.1 MB or even a bit higher if needed to fix the consistent macOS 3.11 failures, this shouldn't change the meaning of the test

@pquentin
Copy link
Member Author

This sounds good to me! But can we report it to pytest-memray first?

@ecerulm
Copy link
Contributor

ecerulm commented Feb 27, 2024

Since it's relevant to this discussion

#3337 (comment)

This appears to break test_get_all_memory_usage_single_chunk on macOS 3.10/11/12 somehow:
...
The 528B in response.py at line 246 are allocated by the deque. The 64KiB are allocated in SSLSocket.read() but this makes no sense in this context as the test does not call SSL at all:
...
This sounds like an incompatibility between pytest 8 and pytest-memray on macOS that we should report to pytest-memray.

Copying my comment from #3337 (comment)

I wonder , should we increase the memory limit for this test to "10.064 MB"? After all we already have "10.01 MB" which I assume the "0.01" was to give some room as indicated in pytest.mark.limit_memory

test/test_response.py::TestBytesQueueBuffer::test_get_all_memory_usage_single_chunk fails not only on macOS

  • macOS 3.10
  • macOS 3.11
  • macOS 3.12
  • Ubuntu 22.04 3.12
  • Ubuntu 3.x test_brotlipy

I investigated a bit

  • if we run test/test_response.py::TestBytesQueueBuffer test alone they will pass

  • if we run test/contrib/test_pyopenssl.py::TestSocketClosing:test_socket_close_socket_then_file or test/contrib/test_pyopenssl.py::TestSocketClosing::test_socket_close_stays_open_with_makefile_open before test/test_response.py::TestBytesQueueBuffer::test_get_all_memory_usage_single_chunk then it will fail.

    • I can reproduce 100% on my mac laptop.
    • failure seem to be a consequence of running other tests prior to this one.

...

@illia-v
Copy link
Member

illia-v commented Feb 27, 2024

This sounds good to me! But can we report it to pytest-memray first?

I wonder if the warning is relevant to our case

@pquentin
Copy link
Member Author

My understanding is that objects created during one test may not be deleted immediately, so if you allocate two strings of 1MB each, if Python does not release the memory of the first string immediately, your test will use 2MB of memory. But objects created by one should not be leaking into another test, I think?

@ecerulm
Copy link
Contributor

ecerulm commented Feb 27, 2024

I created bloomberg/pytest-memray#109

But objects created by one should not be leaking into another test, I think?

Just to clarify here you refer to the suspicion that the allocations of 64.0KiB bllow on ssl.py:1107 does not really belong to the test_get_all_memory_usage_single_chunk.

When we run this test in isolation the test passes, but if we run test_socket_close_socket_then_file before test_get_all_memory_usage_single_chunk it fails. That suggest that the allocation of 64KiB really happens on test_socket_close_socket_then_file but "leaks " into the test_get_all_memory_usage_single_chunk .

 _________ TestBytesQueueBuffer.test_get_all_memory_usage_single_chunk __________
Test was limited to 10.0MiB but allocated 10.1MiB
------------------------------ memray-max-memory -------------------------------
List of allocations:
    - 64.0KiB allocated here:
        read:/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/ssl.py:1107
        ...
    - 528.0B allocated here:
        __init__:/Users/runner/work/urllib3/urllib3/.nox/test-3-12/lib/python3.12/site-packages/urllib3/response.py:246
        ...
    - 10.0MiB allocated here:
        test_get_all_memory_usage_single_chunk:/Users/runner/work/urllib3/urllib3/test/test_response.py:115
        ...

@pablogsal
Copy link

See bloomberg/pytest-memray#109 (comment)

@ecerulm
Copy link
Contributor

ecerulm commented Mar 4, 2024

thanks to @pablogsal, for actually finding the culprit.

There are several test cases that leave lingering server_threads, that in itself is not desirable, but in particular it confuses memray since allocations on those threads will show as part of later tests.

I've created #3358, to ensure that the server_thread is always closes/finished/terminated after each test (teardown_method).

@pquentin
Copy link
Member Author

pquentin commented Mar 5, 2024

Closing in favor of #3358. Thanks @pablogsal!

@pquentin pquentin closed this Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Pull requests that don't require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants