Opened 3 years ago

Closed 2 years ago

#5521 defect closed fixed (fixed)

Many ReactorBuilder-style tests in twisted.internet.test do not declare their reactor requirements via requiredInterfaces

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords: tests
Cc: micahel@… Branch: branches/better-test-declarations-5521
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

ReactorBuilder.requiredInterfaces exists to allow tests which require a particular reactor interface to be skipped when an attempt is made to run those tests against a reactor which does not support that interface.

However, many of our tests (for TCP, for UNIX, for UDP, for time) do not use this feature and will still try to run against a reactor that does not provided the necessary interface.

Attachments (1)

required-interfaces.patch (5.6 KB) - added by exarkun 3 years ago.

Download all attachments as: .zip

Change History (13)

Changed 3 years ago by exarkun

comment:1 Changed 3 years ago by exarkun

Attached is a patch that fixes the instances of this that I found. I didn't pay particularly close attention when constructing it, I just added requirements as quick as I could to get the tests to skip against my reactor that is missing most interfaces.

comment:2 Changed 2 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:3 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/better-test-declarations-5521

(In [34734]) Branching to 'better-test-declarations-5521'

comment:4 Changed 2 years ago by exarkun

(In [34735]) Declare that IReactorTCP is required by these test cases. Also, re-wrap docstrings to 80 columns and unify the IPv6 skip definitions.

refs #5521

comment:5 Changed 2 years ago by exarkun

(In [34736]) Make all the necessary declarations for these

refs #5521

comment:6 Changed 2 years ago by exarkun

(In [34737]) The same thing for IReactorTime

refs #5521

comment:7 Changed 2 years ago by exarkun

(In [34739]) I feel as though I've been here before.

refs #5521

comment:8 Changed 2 years ago by exarkun

(In [34742]) This is redundant now, I think

refs #5521

comment:9 Changed 2 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted
  • Status changed from assigned to new

Just what you'd expect, but overlooked before because every existing reactor already implemented all of these interfaces and no one developed new reactors very TDDly. Build results.

comment:10 Changed 2 years ago by mwh

  • Cc micahel@… added
  • Keywords review removed
  • Owner set to exarkun

This seems fine to me, although I have no way of verifying that it works. It certainly looks like it should :-) I don't understand the

if ipv6Skip:
    skip = ipv6Skip

construction, isn't that equivalent to plain skip = ipv6Skip. If you have a good reason for that, and you're sure the few failures in buildbot are unrelated (they certainly look unrelated to me), please merge.

comment:11 Changed 2 years ago by exarkun

isn't that equivalent to plain skip = ipv6Skip

Sadly it is not. A class-level skip = None is different than not having a class-level skip attribute at all. It's terrible, and probably represents a bug or misfeature in trial to be fixed.

comment:12 Changed 2 years ago by exarkun

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

(In [34751]) Merge better-test-declarations-5521

Author: exarkun
Reviewer: mwhudson
Fixes: #5521

Make sure to declare which reactor interfaces are required by various ReactorBuilder-style
tests.

Note: See TracTickets for help on using tickets.