Skip to content

Fix intermittent integration test failures#231

Merged
rwood-moz merged 1 commit into
mainfrom
testing/fix-intermittent-failures
Jul 8, 2026
Merged

Fix intermittent integration test failures#231
rwood-moz merged 1 commit into
mainfrom
testing/fix-intermittent-failures

Conversation

@rwood-moz

@rwood-moz rwood-moz commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

What changed?

Fix some mailstrom intermittent integration test failures.

Why?

The tests are mostly (98%) stable but would be nice to get rid of the intermittent failures.

Limitations and Notes

Changes made by Codex AI; we will see how these changes prove out after a couple weeks in CI. More details:

Changes made so far:

  • Fixed IMAP deletion test calls in test_messaging.py and test_messaging.py so single message ids are passed as lists to imap.delete_messages(). This prevents multi-digit ids like "25" from being iterated character-by-character.

  • Hardened IMAP.delete_messages() in IMAP.py so it verifies \Deleted by fetching message flags after STORE, rather than depending on the exact shape of the STORE response data.

  • Improved JMAP.wait_for_message_to_arrive() in JMAP.py to dynamically resolve the active account’s Inbox id, poll immediately, wait up to 7 minutes, accept one or more matching messages, and log the account/inbox being checked.

  • Updated JMAP.query_email() in JMAP.py to make collapse_threads configurable, preserving the existing default while allowing arrival polling to query with collapse_threads=False. This should help self-send CC/BCC tests where collapsed thread results may hide the inbox copy.

Applicable Issues

Fixes #230.

QA Log

  • Ran the integration tests on my machine against stage several times, and
  • Ran the nightly mailstrom integration tests GHA job against this branch in CI several times and didn't see any intermittent failures thus far.

@rwood-moz rwood-moz changed the title WIP: Fix intermittent integration test failures Fix intermittent integration test failures Jul 2, 2026
@rwood-moz rwood-moz marked this pull request as ready for review July 2, 2026 17:50
@aatchison

Copy link
Copy Markdown
Contributor

Review: Approve with nits ✅

Ran a multi-lens review of this change. TL;DR: this is a well-scoped de-flaking PR that fixes real root causes rather than loosening tolerances — the key property for a de-flaking change is that a genuinely-broken message/arrival still fails loudly, and that holds here. Nothing blocks merge.

What's good

  • delete_messages([test_msg]) — stops a bare string id (e.g. "25") from being iterated character-by-character, which silently flagged unrelated messages \Deleted while the test still passed by re-finding the intact subject. Genuine intermittent-corruption source and a likely mechanic behind A couple of mailstrom integration test intermittent failures #230.
  • Re-fetching FLAGS instead of asserting on data[0] (the optional STORE untagged echo, whose shape/timing varies by server) verifies authoritative server state on the same still-SELECTED connection — API-correct and de-flakes without weakening the assertion.
  • Resolved inbox_id replacing hardcoded inMailbox='a', guarded by asserts — turns a slow timeout-to-0 into an immediate, diagnosable failure.
  • Loop math is exact: (7*60//5)+1 = 85 checks with the if checks > 1 sleep guard → 84 × 5s = exactly 420s, no overshoot, and an already-delivered message returns ~5s faster.
  • query_email signature change is fully backward-compatiblecollapse_threads defaults to the old hardcoded True; all other callers unaffected.

Non-blocking suggestions

  1. (low) IMAP.delete_messages still has the footgun. Only the call sites were fixed; the helper still iterates its arg, so a future caller passing a scalar id re-opens the exact char-by-char wrong-deletion bug. Consider hardening the helper:
    def delete_messages(self, msg_ids):
        if isinstance(msg_ids, str):
            msg_ids = [msg_ids]
        ...
    and tightening the docstring to state it takes a list of ids.
  2. (nit) # = 85 comment. It restates a computed value and will lie if the timeout is tuned. The load-bearing detail is why the +1 exists — the first iteration skips its sleep, so +1 keeps total sleep == max_wait_seconds (420s). Worth documenting that coupling near the if checks > 1 guard instead of the literal 85.
  3. (nit) len(found_email_ids) >= 1 is just bool(...); if found_email_ids: reads clearer now that the intent is "any match found."

Out of scope (pre-existing, possible follow-up)

  • get_mailbox_by_name raises UnboundLocalError on its swallowed-exception path (results never assigned before the assert).
  • wait_for_message_to_arrive returns 0 on timeout despite the docstring saying -1.

Neither is introduced by this PR.

aatchison
aatchison previously approved these changes Jul 7, 2026

@aatchison aatchison left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved with nits!

@rwood-moz rwood-moz force-pushed the testing/fix-intermittent-failures branch from 794a509 to 6ca8400 Compare July 8, 2026 14:14
@rwood-moz

Copy link
Copy Markdown
Contributor Author

approved with nits!

Thanks @aatchison I made some of the suggested updates and now it needs a re-approval please, thanks

@rwood-moz rwood-moz merged commit 52f9274 into main Jul 8, 2026
2 checks passed
@rwood-moz rwood-moz deleted the testing/fix-intermittent-failures branch July 8, 2026 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A couple of mailstrom integration test intermittent failures

2 participants