Opened 7 years ago

Closed 7 years ago

#7776 enhancement closed fixed (fixed)

Remove all todo tests and file dedicated tickets for them

Reported by: Adi Roiban Owned by: Adi Roiban
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/todo-removal-7776
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban


TODO tests are useless. We should remove them from code and file dedicated ticket for each todo test (or one ticket for related tests)

From mailing lists

The default state of a test should *not* be to skip.  It should be to run.
A skipped test is a useless test.

As a matter of general principle, the only reason a test should ever be
skipped within Twisted is if an optional dependency is not available.  And,
just to be clear, the presence of "TODO'd" tests is pretty much just a bug,
fix old ones and never add new ones :-).

I think that these todo are just another way of creating a ticket ...
with a promise to annoy you forever

And to annoy every contributor ever who works on something unrelated :).

Change History (6)

comment:1 Changed 7 years ago by Adi Roiban

Author: adiroiban
Branch: branches/todo-removal-7776

(In [43874]) Branching to todo-removal-7776.

comment:2 Changed 7 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Follow up tickets #7777 #7778 #1396 #7779 #7781 #7782 #7783 #7784 #7785

Does twisted still support linux 2.2 ? :) I have just removed a todo test for linux 2.2

I have converted conditionals todo into skips and made sure that there is a ticket for them.

I have searched the code for .todo pattern.

I have removed FIXME references to #6008 as twisted code should no longer have todo markers.

I triggered the buildbot and checked that there are no more expected failures.

As a follow up, maybe we should update the docs to mention that todo tests should not be merged in master...or at least add this to the reviewer's check list.

Please check that changes make sense. Thanks!

comment:3 Changed 7 years ago by Glyph

Twisted does not support Linux 2.2. Hopefully nobody is building new devices or installing new OSes that require it :-).

comment:4 Changed 7 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

This looks great, thanks for filing all those tickets. Land, please!

comment:5 Changed 7 years ago by Adi Roiban

Thanks for the review. I have created #7819 as a follow up of this so that in the future we don't merge todo tests.

comment:6 Changed 7 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [44066]) Merging todo-removal-7776: Remove all TODO tests.

Author: adiroiban Reviewer: glyph Fixes: #7776

Note: See TracTickets for help on using tickets.