Skip to content

Resolve TODO comments in test_concurrent_futures.py #100522

Closed
@JosephSBoyle

Description

@JosephSBoyle

Feature or enhancement

There are 3 TODO comments that should be resolved in the test_concurrent_futures.py file[0]

# TODO(brian@sweetapp.com): Should have a test with a non-zero timeout.

2.
# TODO(brian@sweetapp.com): This test is timing dependent.

3.
# TODO(brian@sweetapp.com): This test is timing dependent.

Pitch

  • Resolving 1), i.e adding a test to cover the non-zero timeout case would simply make the tests more robust. resolved in: gh-100522 Add a test for 'futures.as_completed' timing out with a non-zero timeout value #100523
  • Resolving 2) would have two benefits. Firstly, it would make the tests run faster by ~1s (there is a 1s sleep). Secondly, sleeping in the tests can occasionally cause flakiness[1] and so I would consider it good practice to remove them where practical.
  • Resolving 3) Same reasons as for 2).

Is there a preferred way to tackle this?


[0]. https://github.com/python/cpython/blob/e16d4ed59072839b49bda4b447f260201aae7e39/Lib/test/test_concurrent_futures.py
[1]. This seems to be acknowledged in the related timeouts that are used:

# If a test using SHORT_TIMEOUT starts to fail randomly on slow buildbots, use

CC: @brianquinlan

Linked PRs

Metadata

Metadata

Assignees

No one assigned

    Labels

    testsTests in the Lib/test dirtype-featureA feature request or enhancement

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions