Opened 2 years ago

Closed 5 weeks ago

#5730 defect closed fixed (fixed)

stdio endpoint tests leave a dirty reactor

Reported by: ashfall Owned by: glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/dirty-stdio-endpoint-5730
(diff, github, buildbot, log)
Author: glyph Launchpad Bug:

Description (last modified by glyph)

The problem with many of the tests in StandardIOEndpointsTestCase is that they're using a real reactor, but not actually waiting for the disconnection notification.

It would be nice to avoid using a real reactor (or, for that matter, real stdio streams) in most of these tests; if we want an integration test it should probably live in an isolated subprocess.

As per this comment on #4697, this needs to be fixed.

Change History (7)

comment:1 Changed 2 years ago by ashfall

#5892 looks like a duplicate of this.

comment:2 Changed 8 weeks ago by glyph

  • Description modified (diff)
  • Summary changed from Fix the dirty reactor warnings when tests for the stdio endpoints are run on Windows to stdio endpoint tests leave a dirty reactor

Previously, this ticket indicated that the issue was with Windows. However, when running test_endpoints under Emacs (which has a relatively slow pipe consumer on stdout) I have been seeing this somewhat regularly:

[ERROR]
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorAggregateError: Reactor was unclean.
Selectables:
<twisted.internet.process.ProcessWriter object at 0x104216e10>

twisted.internet.test.test_endpoints.StandardIOEndpointsTestCase.test_protocol

comment:3 Changed 8 weeks ago by glyph

  • Author set to glyph
  • Branch set to branches/dirty-stdio-endpoint-5730

(In [42774]) Branching to dirty-stdio-endpoint-5730.

comment:4 Changed 8 weeks ago by glyph

  • Keywords review added
  • Owner ashfall deleted

comment:5 follow-up: Changed 7 weeks ago by adiroiban

  • Keywords review removed
  • Owner set to glyph

Does this have a news file?


Is this supposed to work on Python 3, as for PY3 I see that it is set to None?


with then new setup method I find it hard to read tests...
I read the docstring of test_protocolCreation ... and then read
the code... but it looks like protocol is already created and the
test does not directly create the protocol
...same for test_passedReactor...

Maybe this can be merged into a single test


I think that the patch looks good and the removal of reactor singleton usage is welcomed.

Thanks

comment:6 in reply to: ↑ 5 Changed 5 weeks ago by glyph

Replying to adiroiban:

Does this have a news file?

It does now. Just a misc though, this is just a test issue.

Is this supposed to work on Python 3, as for PY3 I see that it is set to None?

There's no change to python 3 support in this branch, as it's just a fix for a test failure.

with then new setup method I find it hard to read tests...
I read the docstring of test_protocolCreation ... and then read
the code... but it looks like protocol is already created and the
test does not directly create the protocol
...same for test_passedReactor...

Maybe this can be merged into a single test

Out of scope for this change :-). Also, tests should be as fine-grained as possible, and I don't see that a lot would be gained by merging them together.

I think that the patch looks good and the removal of reactor singleton usage is welcomed.

Great.

Again, for future reference, please include the next step (please merge, please resubmit) in your reviews.

Thanks!

comment:7 Changed 5 weeks ago by glyph

  • Resolution set to fixed
  • Status changed from new to closed

(In [42819]) Merge dirty-stdio-endpoint-5730

Author: dreid, glyph
Reviewer: adiroiban
Fixes: #5730

Fix an intermittently failing (dirty reactor) test for StandardIOEndpoint.

Note: See TracTickets for help on using tickets.