fix: Tombstone MDN before sending it (#8252)#8257
Conversation
WofWca
left a comment
There was a problem hiding this comment.
Sounds like this should work but I'm probably not the right person to ask.
IDK about introducing different behavior for tests though, it doesn't seem like something we often do at least. At least some comments about could be useful? But maybe it's obvious to someone who knows what they're reading.
The |
This is already discussed in #8008, we indeed don't want to have conditional compilation or hooks just for tests. Just checking that MDN is in |
44261e2 to
842d1f4
Compare
842d1f4 to
48b7697
Compare
|
For the record, i've removed conditional compilation, but just in case if changes still look complex, i'm going to replace the changes to |
|
I've tried to add a Python test, but it fails because #7758 is still unmerged and that affects testing because the message state is "synced back" from IMAP, doing anything with MDNs is not helpful at the moment. This PR should be merged after #7758 So this is flaky, sometimes def test_mark_fresh_vs_self_mdn(acfactory) -> None:
alice, bob = acfactory.get_online_accounts(2)
bob.set_config("bcc_self", "1")
alice_contact_bob = alice.create_contact(bob)
alice_chat = alice_contact_bob.create_chat()
alice_chat.send_text("Hello!")
event = bob.wait_for_incoming_msg_event()
bob.wait_for_event(EventType.IMAP_INBOX_IDLE)
chat_id = event.chat_id
msg_id = event.msg_id
bob_chat = bob.get_chat_by_id(chat_id)
message = bob.get_message_by_id(msg_id)
bob.clear_all_events()
bob.mark_seen_messages([message])
bob_chat.mark_fresh()
assert bob_chat.get_fresh_message_count() == 1
bob.wait_for_event(EventType.IMAP_INBOX_IDLE)
assert bob_chat.get_fresh_message_count() == 1 |
48b7697 to
8aeaa76
Compare
| bob.mark_seen_messages([message]) | ||
| bob_chat.mark_fresh() | ||
| assert bob_chat.get_fresh_message_count() == 1 | ||
| for _ in range(3): |
There was a problem hiding this comment.
Can improve this in a follow-up PR if you want, or create an issue. We don't need to interrupt the inbox loop that often, e.g. in imap::prefetch_should_download(). markseen_on_imap_table() even interrupts it unconditionally: it does INSERT OR IGNORE, but it doesn't check if a row was inserted.
There was a problem hiding this comment.
Currently the test looks like it will get stuck if we decrease the number of interruptions or even if the network is slow and two interruptions collapse into one. I'd at least put a TODO here.
There was a problem hiding this comment.
There is probably a way to make it non-flaky by not relying on inbox IDLE already without modifications to how often we interrupt IDLE.
E.g. set up a second Bob and wait for MDN to arrive there (the chat marked as seen on second Bob's device). Then send another message from second Bob to self messages and wait for it to arrive on first Bob's device. Then check the status of the chat with Alice on Bob's first device.
There was a problem hiding this comment.
For Rust tests dedicated MDN rendering function as described in #8008 (comment) is likely also fine if it is just a factored out code that is also used by the code running outside the tests.
There was a problem hiding this comment.
There is probably a way to make it non-flaky by not relying on inbox IDLE already without modifications to how often we interrupt IDLE.
I've modified the test by waiting for the MDN on alice's side and sending a new message to bob, to not complicate the test too much. Have checked that it fails w/o the fix.
Still, should i create an issue for reduction of number of inbox loop interruptions?
|
https://github.com/chatmail/core/actions/runs/27877188488/job/82498958716?pr=8257 -- the same issue as in #8222 (comment). Not related to the PR. |
| bob.mark_seen_messages([message]) | ||
| bob_chat.mark_fresh() | ||
| assert bob_chat.get_fresh_message_count() == 1 | ||
| for _ in range(3): |
There was a problem hiding this comment.
Currently the test looks like it will get stuck if we decrease the number of interruptions or even if the network is slow and two interruptions collapse into one. I'd at least put a TODO here.
WofWca
left a comment
There was a problem hiding this comment.
Still looks OK to my untrained eye.
Otherwise, when it appears on IMAP, it will mark chat messages as seen/noticed even if markfresh_chat() is called meanwhile.
8aeaa76 to
84afb82
Compare
|
There was an unrelated failure of Not clear why it failed after the successful reconnection, we have a 300s timeout in the CFFI tests. |
Otherwise, when it appears on IMAP, it will mark chat messages as seen/noticed even if
markfresh_chat() is called meanwhile.
Fix #8252