Opened 4 years ago

Closed 4 years ago

#4467 defect closed fixed (fixed)

twisted.mail.test.test_mail.MaildirAppendStringTestCase.testAppend fails intermittently on some builders

Reported by: glyph Owned by:
Priority: normal Milestone:
Component: mail Keywords:
Cc: Branch: branches/reliable-appendMessage-4467
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

For example, this build failed because of it.

Possibly this is related to #3812?

Change History (9)

comment:1 Changed 4 years ago by glyph

  • Type changed from enhancement to defect

comment:2 Changed 4 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/reliable-appendMessage-4467

(In [29108]) Branching to 'reliable-appendMessage-4467'

comment:3 Changed 4 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

The problem is that message ordering is defined by when delivery completes, not when it starts. The test assumed that delivery would complete in the same order it was started, but on a machine with a clock of low enough precision this stopped being true.

Build results

comment:4 Changed 4 years ago by glyph

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

comment:5 Changed 4 years ago by glyph

  • Keywords review removed
  • Owner changed from glyph to exarkun
  • Status changed from assigned to new

OK, this looks good to me. Some minor stuff to fix, if you feel like it:

  1. It would be nice if appendMessage had a @return or something. It's really not clear to me when that deferred is supposed to fire, and it doesn't seem to be part of an interface.
  2. Pyflakes reports the imports from the top of the file as unused. (Actually, nevermind. Maybe just file a separate easy ticket for this one, since it seems to be over the file and unrelated to your changes.)
  3. Shouldn't _AppendTestMixin inherit from object, since it's a new class?

This looks like a big improvement though, so, merge when ready.

comment:6 Changed 4 years ago by exarkun

(In [29148]) Expand docstring for appendMessage

refs #4467

comment:7 Changed 4 years ago by exarkun

(In [29149]) Make _AppendTextMixin new-style

refs #4467

comment:8 Changed 4 years ago by exarkun

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

(In [29150]) Merge reliable-appendMessage-4467

Author: exarkun
Reviewer: glyph
Fixes: #4467

Change the tests for MaildirMailbox.appendMessage so that they only assert
a deterministic delivery-completion based delivery order. Previously they
attempted to assert delivery-initiation based order, which is not guaranteed.
Also remove the callLater(0, ...) in the implementation of appendMessage,
as it really is not necessary.

comment:9 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.