Opened 7 years ago

Closed 7 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
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

For example, this build failed because of it.

Possibly this is related to #3812?

Change History (9)

comment:1 Changed 7 years ago by Glyph

Type: enhancementdefect

comment:2 Changed 7 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/reliable-appendMessage-4467

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

comment:3 Changed 7 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone 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 7 years ago by Glyph

Owner: set to Glyph
Status: newassigned

comment:5 Changed 7 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to Jean-Paul Calderone
Status: assignednew

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 7 years ago by Jean-Paul Calderone

(In [29148]) Expand docstring for appendMessage

refs #4467

comment:7 Changed 7 years ago by Jean-Paul Calderone

(In [29149]) Make _AppendTextMixin new-style

refs #4467

comment:8 Changed 7 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(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 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.