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

Sprinkle @slow on slow tests #3215

Merged
merged 2 commits into from
Feb 26, 2025
Merged

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Feb 25, 2025

Fixes #3206.

The main thing is that it turns out that waiting for a socket to fail to connect takes up to 2 seconds on Windows -- so tests that rely on that should be marked @slow.


This approximately halves local test time on Windows.

================================================ slowest 10 durations =================================================
0.75s call     src/trio/_tests/test_exports.py::test_static_tool_sees_all_symbols[jedi-trio]
0.72s call     src/trio/_tests/tools/test_gen_exports.py::test_lint_failure
0.70s call     src/trio/_tests/test_repl.py::test_main_entrypoint
0.52s call     src/trio/_tests/test_util.py::test_coroutine_or_error
0.39s call     src/trio/_tests/test_exports.py::test_static_tool_sees_all_symbols[pylint-trio]
0.33s call     src/trio/_tests/test_subprocess.py::test_run
0.32s call     src/trio/_tests/test_subprocess.py::test_interactive[run_process in nursery]
0.32s call     src/trio/_tests/test_socket.py::test_many_sockets
0.26s call     src/trio/_tests/test_subprocess.py::test_interactive[open_process]
0.26s call     src/trio/_core/_tests/test_asyncgen.py::test_fallback_when_no_hook_claims_it
===================================== 762 passed, 83 skipped, 2 xfailed in 17.93s =====================================

@jakkdl
Copy link
Member

jakkdl commented Feb 25, 2025

2 seconds sounds unnecessarily slow, even if it's only CI. It'd be nice if we could lower the timeout somehow, or cancel it if it hasn't connected in a reasonable time. Though the former sounds difficult if there's no official API, and the latter would change what the test is actually testing for.

@A5rocks
Copy link
Contributor Author

A5rocks commented Feb 25, 2025

Based on the link in https://stackoverflow.com/a/54599972;

Upon receiving the ACK/RST client from the target host, the client determines that there is indeed no service listening there. In the Microsoft Winsock implementation of TCP, a pending connection will keep attempting to issue SYN packets until a maximum retry value is reached (set in the registry, this value defaults to 3 extra times)

It seems it only actually takes 1s, so we must be trying two ports (ipv4 and ipv6?).

Setting registry values doesn't seem like something a test suite should do locally.

Copy link
Member

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

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

Looks pretty good, agree with jakkdl's comment as well though

Copy link

codecov bot commented Feb 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00000%. Comparing base (a9f8abd) to head (144ff95).
Report is 56 commits behind head on main.

Additional details and impacted files
@@                Coverage Diff                @@
##                 main        #3215     +/-   ##
=================================================
  Coverage   100.00000%   100.00000%             
=================================================
  Files             124          124             
  Lines           18792        20902   +2110     
  Branches         1268         1641    +373     
=================================================
+ Hits            18792        20902   +2110     
Files with missing lines Coverage Δ
...c/trio/_tests/test_highlevel_open_tcp_listeners.py 100.00000% <100.00000%> (ø)
src/trio/_tests/test_socket.py 100.00000% <100.00000%> (ø)
src/trio/_tests/tools/test_gen_exports.py 100.00000% <100.00000%> (ø)

... and 20 files with indirect coverage changes

@jakkdl jakkdl added this pull request to the merge queue Feb 26, 2025
Merged via the queue into python-trio:main with commit 4df83b4 Feb 26, 2025
43 checks passed
@A5rocks A5rocks deleted the faster-local-pytest branch February 26, 2025 11:00
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.

Mark test_gen_exports's test_process as run_slow
3 participants