From f0fd79f86fee5b9e67a8fc8e0f220187fb61709d Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 8 Sep 2023 11:09:15 +0100 Subject: [PATCH 1/8] Tests for redacting messages not increasing unreads --- cypress/e2e/read-receipts/high-level.spec.ts | 44 ++++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/cypress/e2e/read-receipts/high-level.spec.ts b/cypress/e2e/read-receipts/high-level.spec.ts index 0685c3af09c..7e1727726e3 100644 --- a/cypress/e2e/read-receipts/high-level.spec.ts +++ b/cypress/e2e/read-receipts/high-level.spec.ts @@ -137,6 +137,16 @@ describe("Read receipts", () => { cy.get(".mx_ThreadView_timelinePanelWrapper", { log: false }).should("have.length", 1); } + /** + * Close the threads panel. (Actually, close any right panel, but for these + * tests we only open the threads panel.) + */ + function closeThreadsPanel() { + cy.log("Close threads panel"); + cy.get(".mx_RightPanel").findByTitle("Close").click(); + cy.get(".mx_RightPanel").should("not.exist"); + } + function sendMessageAsClient(cli: MatrixClient, room: string, messages: Message[]) { findRoomByName(room).then(async ({ roomId }) => { const room = cli.getRoom(roomId); @@ -1618,9 +1628,37 @@ describe("Read receipts", () => { }); describe("in threads", () => { - // One of the following two must be right: - it.skip("Redacting the threaded message pointed to by my receipt leaves the room read", () => {}); - it.skip("Redacting a threaded message after it was read makes the room unread", () => {}); + it("Redacting the threaded message pointed to by my receipt leaves the room read", () => { + // Given I have some threads + goTo(room1); + receiveMessages(room2, [ + "Root", + threadedOff("Root", "ThreadMsg1"), + threadedOff("Root", "ThreadMsg2"), + "Root2", + threadedOff("Root2", "Root2->A"), + ]); + assertUnread(room2, 5); + + // And I have read them + goTo(room2); + assertUnreadThread("Root"); + openThread("Root"); + assertUnreadLessThan(room2, 4); + openThread("Root2"); + assertRead(room2); + closeThreadsPanel(); + goTo(room1); + assertRead(room2); + + // When the latest message in a thread is redacted + receiveMessages(room2, [redactionOf("ThreadMsg2")]); + + // Then the room and thread are still read + assertStillRead(room2); + goTo(room2); + assertReadThread("Root"); + }); it.skip("Reading an unread thread after a redaction of the latest message makes it read", () => {}); it.skip("Reading an unread thread after a redaction of an older message makes it read", () => {}); From 7f1d69416f3330d037f015c48460269abcdeff25 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 8 Sep 2023 14:04:21 +0100 Subject: [PATCH 2/8] Comment explaining tips for writing high level rr tests --- cypress/e2e/read-receipts/high-level.spec.ts | 23 ++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/cypress/e2e/read-receipts/high-level.spec.ts b/cypress/e2e/read-receipts/high-level.spec.ts index 7e1727726e3..4b3a9b6e057 100644 --- a/cypress/e2e/read-receipts/high-level.spec.ts +++ b/cypress/e2e/read-receipts/high-level.spec.ts @@ -14,6 +14,29 @@ See the License for the specific language governing permissions and limitations under the License. */ +/* + * # High Level Read Receipt Tests + * + * Tips for writing these tests: + * + * * Break up your tests into the smallest test case possible. The purpose of + * these tests is to understand hard-to-find bugs, so small tests are necessary. + * We know that Cypress recommends combining tests together for performance, but + * that will frustrate our goals here. (We will need to find a different way to + * reduce CI time.) + * + * * Try to assert something after every action, to make sure it has completed. + * E.g.: + * markAsRead(room2); + * assertRead(room2); + * You should especially follow this rule if you are jumping to a different + * room or similar straight afterwards. + * + * * Use assertStillRead() if you are asserting something is read when it was + * also read before. This waits a little while to make sure you're not getting a + * false positive. + */ + /// import type { MatrixClient, MatrixEvent, Room, IndexedDBStore } from "matrix-js-sdk/src/matrix"; From 70ea7eb6b51b428dbee77da5084c31dd9314eece Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 8 Sep 2023 14:04:48 +0100 Subject: [PATCH 3/8] Test for restarting with a receipt pointing at a redacted thread root --- cypress/e2e/read-receipts/high-level.spec.ts | 32 +++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/cypress/e2e/read-receipts/high-level.spec.ts b/cypress/e2e/read-receipts/high-level.spec.ts index 4b3a9b6e057..e076944add4 100644 --- a/cypress/e2e/read-receipts/high-level.spec.ts +++ b/cypress/e2e/read-receipts/high-level.spec.ts @@ -1696,7 +1696,37 @@ describe("Read receipts", () => { it.skip("Reading a reply to a redacted message marks the thread as read", () => {}); it.skip("A thread with an unread redaction is still unread after restart", () => {}); - it.skip("A thread with a read redaction is still read after restart", () => {}); + it("A thread with a read redaction is still read after restart", () => { + // Given my receipt points at a redacted thread message + goTo(room1); + receiveMessages(room2, [ + "Root", + threadedOff("Root", "ThreadMsg1"), + threadedOff("Root", "ThreadMsg2"), + "Root2", + threadedOff("Root2", "Root2->A"), + ]); + assertUnread(room2, 5); + goTo(room2); + assertUnreadThread("Root"); + openThread("Root"); + assertUnreadLessThan(room2, 4); + openThread("Root2"); + assertRead(room2); + closeThreadsPanel(); + goTo(room1); + assertRead(room2); + receiveMessages(room2, [redactionOf("ThreadMsg2")]); + assertStillRead(room2); + goTo(room2); + assertReadThread("Root"); + + // When I restart + saveAndReload(); + + // Then the room is still read + assertRead(room2); + }); it.skip("A thread with an unread reply to a redacted message is still unread after restart", () => {}); it.skip("A thread with a read replt to a redacted message is still read after restart", () => {}); }); From 07078a694e3f7b2ea6df1a1103524dc54401a230 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 8 Sep 2023 14:18:31 +0100 Subject: [PATCH 4/8] Two failing tests for counting correctly when a thread message was redacted --- cypress/e2e/read-receipts/high-level.spec.ts | 47 +++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/cypress/e2e/read-receipts/high-level.spec.ts b/cypress/e2e/read-receipts/high-level.spec.ts index e076944add4..5904f378e51 100644 --- a/cypress/e2e/read-receipts/high-level.spec.ts +++ b/cypress/e2e/read-receipts/high-level.spec.ts @@ -1683,7 +1683,52 @@ describe("Read receipts", () => { assertReadThread("Root"); }); - it.skip("Reading an unread thread after a redaction of the latest message makes it read", () => {}); + // XXX: fails because the unread count is still 1 when it should be 0 + it.skip("Reading an unread thread after a redaction of the latest message makes it read", () => { + // Given an unread thread where the latest message was redacted + goTo(room1); + receiveMessages(room2, ["Root", threadedOff("Root", "ThreadMsg1"), threadedOff("Root", "ThreadMsg2")]); + assertUnread(room2, 3); + receiveMessages(room2, [redactionOf("ThreadMsg2")]); + assertUnread(room2, 2); + goTo(room2); + assertUnreadThread("Root"); + + // When I read the thread + openThread("Root"); + assertRead(room2); + closeThreadsPanel(); + goTo(room1); + + // Then the thread is read + assertRead(room2); + goTo(room2); + assertReadThread("Root"); + }); + // XXX: fails because the unread count is still 1 when it should be 0 + it.skip("Reading an unread thread after a redaction of the latest message makes it read after restart", () => { + // Given a redacted message is not counted in the unread count + goTo(room1); + receiveMessages(room2, ["Root", threadedOff("Root", "ThreadMsg1"), threadedOff("Root", "ThreadMsg2")]); + assertUnread(room2, 3); + receiveMessages(room2, [redactionOf("ThreadMsg2")]); + assertUnread(room2, 2); + goTo(room2); + assertUnreadThread("Root"); + openThread("Root"); + assertRead(room2); + closeThreadsPanel(); + goTo(room1); + assertRead(room2); + goTo(room2); + assertReadThread("Root"); + + // When I restart + saveAndReload(); + + // Then the room is still read + assertRead(room2); + }); it.skip("Reading an unread thread after a redaction of an older message makes it read", () => {}); it.skip("Marking an unread thread as read after a redaction makes it read", () => {}); it.skip("Sending and redacting a message after marking the thread as read makes it unread", () => {}); From bdfdf3c65a517cf0e7fe9da30c3e37e157b347a9 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 8 Sep 2023 14:22:35 +0100 Subject: [PATCH 5/8] Test for reading a thread containing an earlier redaction --- cypress/e2e/read-receipts/high-level.spec.ts | 25 ++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/cypress/e2e/read-receipts/high-level.spec.ts b/cypress/e2e/read-receipts/high-level.spec.ts index 5904f378e51..16ac8fd97c5 100644 --- a/cypress/e2e/read-receipts/high-level.spec.ts +++ b/cypress/e2e/read-receipts/high-level.spec.ts @@ -1683,7 +1683,7 @@ describe("Read receipts", () => { assertReadThread("Root"); }); - // XXX: fails because the unread count is still 1 when it should be 0 + // XXX: fails because the unread count is still 1 when it should be 0 (this is a genuine stuck unread case) it.skip("Reading an unread thread after a redaction of the latest message makes it read", () => { // Given an unread thread where the latest message was redacted goTo(room1); @@ -1729,7 +1729,28 @@ describe("Read receipts", () => { // Then the room is still read assertRead(room2); }); - it.skip("Reading an unread thread after a redaction of an older message makes it read", () => {}); + // XXX: fails because the unread count is still 1 when it should be 0 + it.skip("Reading an unread thread after a redaction of an older message makes it read", () => { + // Given an unread thread where an older message was redacted + goTo(room1); + receiveMessages(room2, ["Root", threadedOff("Root", "ThreadMsg1"), threadedOff("Root", "ThreadMsg2")]); + assertUnread(room2, 3); + receiveMessages(room2, [redactionOf("ThreadMsg1")]); + assertUnread(room2, 2); + goTo(room2); + assertUnreadThread("Root"); + + // When I read the thread + openThread("Root"); + assertRead(room2); + closeThreadsPanel(); + goTo(room1); + + // Then the thread is read + assertRead(room2); + goTo(room2); + assertReadThread("Root"); + }); it.skip("Marking an unread thread as read after a redaction makes it read", () => {}); it.skip("Sending and redacting a message after marking the thread as read makes it unread", () => {}); it.skip("?? Redacting a message after marking the thread as read makes it unread", () => {}); From a35c2db4217869be44ad98e0964dc94388866408 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 8 Sep 2023 14:41:03 +0100 Subject: [PATCH 6/8] Failing tests for redacted messages in threads --- cypress/e2e/read-receipts/high-level.spec.ts | 104 ++++++++++++++++++- 1 file changed, 99 insertions(+), 5 deletions(-) diff --git a/cypress/e2e/read-receipts/high-level.spec.ts b/cypress/e2e/read-receipts/high-level.spec.ts index 16ac8fd97c5..a29f951ed91 100644 --- a/cypress/e2e/read-receipts/high-level.spec.ts +++ b/cypress/e2e/read-receipts/high-level.spec.ts @@ -1751,15 +1751,109 @@ describe("Read receipts", () => { goTo(room2); assertReadThread("Root"); }); - it.skip("Marking an unread thread as read after a redaction makes it read", () => {}); - it.skip("Sending and redacting a message after marking the thread as read makes it unread", () => {}); - it.skip("?? Redacting a message after marking the thread as read makes it unread", () => {}); - it.skip("Reacting to a redacted message leaves the thread read", () => {}); - it.skip("Editing a redacted message leaves the thread read", () => {}); + // XXX: fails because the room has an unread dot after I marked it as read + it.skip("Marking an unread thread as read after a redaction makes it read", () => { + // Given an unread thread where an older message was redacted + goTo(room1); + receiveMessages(room2, ["Root", threadedOff("Root", "ThreadMsg1"), threadedOff("Root", "ThreadMsg2")]); + assertUnread(room2, 3); + receiveMessages(room2, [redactionOf("ThreadMsg1")]); + assertUnread(room2, 2); + + // When I mark the room as read + markAsRead(room2); + assertRead(room2); + + // Then the thread is read + assertRead(room2); + goTo(room2); + assertReadThread("Root"); + }); + // XXX: fails because the room has an unread dot after I marked it as read + it.skip("Sending and redacting a message after marking the thread as read leaves it read", () => { + // Given a thread exists and is marked as read + goTo(room1); + receiveMessages(room2, ["Root", threadedOff("Root", "ThreadMsg1"), threadedOff("Root", "ThreadMsg2")]); + assertUnread(room2, 3); + markAsRead(room2); + assertRead(room2); + + // When I send and redact a message + receiveMessages(room2, [threadedOff("Root", "Msg3")]); + assertUnread(room2, 1); + receiveMessages(room2, [redactionOf("Msg3")]); + + // Then the room and thread are read + assertRead(room2); + goTo(room2); + assertReadThread("Root"); + }); + // XXX: fails because the room has an unread dot after I marked it as read + it.skip("Redacting a message after marking the thread as read leaves it read", () => { + // Given a thread exists and is marked as read + goTo(room1); + receiveMessages(room2, ["Root", threadedOff("Root", "ThreadMsg1"), threadedOff("Root", "ThreadMsg2")]); + assertUnread(room2, 3); + markAsRead(room2); + assertRead(room2); + + // When I redact a message + receiveMessages(room2, [redactionOf("ThreadMsg1")]); + + // Then the room and thread are read + assertRead(room2); + goTo(room2); + assertReadThread("Root"); + }); + // TODO: Doesn't work because the test setup can't (yet) find the ID of a redacted message + it.skip("Reacting to a redacted message leaves the thread read", () => { + // Given a message in a thread was redacted and everything is read + goTo(room1); + receiveMessages(room2, ["Root", threadedOff("Root", "Msg2"), threadedOff("Root", "Msg3")]); + receiveMessages(room2, [redactionOf("Msg2")]); + assertUnread(room2, 2); + goTo(room2); + assertUnread(room2, 1); + openThread("Root"); + assertRead(room2); + goTo(room1); + + // When we receive a reaction to the redacted event + // TODO: doesn't work yet because we need to be able to look up + // the ID of Msg2 even though it has now disappeared from the + // timeline. + receiveMessages(room2, [reactionTo(room2, "Msg2")]); + + // Then the room is unread + assertStillRead(room2); + }); + // TODO: Doesn't work because the test setup can't (yet) find the ID of a redacted message + it.skip("Editing a redacted message leaves the thread read", () => { + // Given a message in a thread was redacted and everything is read + goTo(room1); + receiveMessages(room2, ["Root", threadedOff("Root", "Msg2"), threadedOff("Root", "Msg3")]); + receiveMessages(room2, [redactionOf("Msg2")]); + assertUnread(room2, 2); + goTo(room2); + assertUnread(room2, 1); + openThread("Root"); + assertRead(room2); + goTo(room1); + + // When we receive an edit of the redacted message + // TODO: doesn't work yet because we need to be able to look up + // the ID of Msg2 even though it has now disappeared from the + // timeline. + receiveMessages(room2, [editOf("Msg2", "New Msg2")]); + + // Then the room is unread + assertStillRead(room2); + }); it.skip("?? Reading a reaction to a redacted message marks the thread as read", () => {}); it.skip("?? Reading an edit of a redacted message marks the thread as read", () => {}); it.skip("Reading a reply to a redacted message marks the thread as read", () => {}); + it.skip("Reading a thread root when its only message has been redacted leaves the room read", () => {}); it.skip("A thread with an unread redaction is still unread after restart", () => {}); it("A thread with a read redaction is still read after restart", () => { From e0915768966b6db695a94de996105afc0a9451f2 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 18 Sep 2023 11:46:26 +0100 Subject: [PATCH 7/8] More tests for reactions to messages in threads --- cypress/e2e/read-receipts/high-level.spec.ts | 94 ++++++++++++++++++-- 1 file changed, 89 insertions(+), 5 deletions(-) diff --git a/cypress/e2e/read-receipts/high-level.spec.ts b/cypress/e2e/read-receipts/high-level.spec.ts index a29f951ed91..99b2c8b1841 100644 --- a/cypress/e2e/read-receipts/high-level.spec.ts +++ b/cypress/e2e/read-receipts/high-level.spec.ts @@ -1314,24 +1314,108 @@ describe("Read receipts", () => { describe("in threads", () => { it("A reaction to a threaded message does not make the room unread", () => { + // Given a thread exists and I have read it goTo(room1); assertRead(room2); receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Reply1")]); assertUnread(room2, 2); - goTo(room2); openThread("Msg1"); assertRead(room2); + goTo(room1); + + // When someone reacts to a thread message + receiveMessages(room2, [reactionTo("Reply1", "🪿")]); + // Then the room remains read + assertStillRead(room2); + }); + // XXX: fails because the room is still "bold" even though the notification counts all disappear + it.skip("Marking a room as read after a reaction in a thread makes it read", () => { + // Given a thread exists with a reaction goTo(room1); + assertRead(room2); + receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Reply1"), reactionTo("Reply1", "🪿")]); + assertUnread(room2, 2); + + // When I mark the room as read + markAsRead(room2); + + // Then it becomes read + assertRead(room2); + }); + // XXX: fails because the room is still "bold" even though the notification counts all disappear + it.skip("Reacting to a thread message after marking as read does not make the room unread", () => { + // Given a thread exists and I have marked it as read + goTo(room1); + assertRead(room2); + receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Reply1"), reactionTo("Reply1", "🪿")]); + assertUnread(room2, 2); + markAsRead(room2); + assertRead(room2); + + // When someone reacts to a thread message receiveMessages(room2, [reactionTo("Reply1", "🪿")]); + // Then the room remains read + assertStillRead(room2); + }); + it.skip("A room with a reaction to a threaded message is still unread after restart", () => { + // Given a thread exists and I have read it + goTo(room1); assertRead(room2); + receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Reply1")]); + assertUnread(room2, 2); + goTo(room2); + openThread("Msg1"); + assertRead(room2); + goTo(room1); + + // And someone reacted to it, which doesn't stop it being read + receiveMessages(room2, [reactionTo("Reply1", "🪿")]); + assertStillRead(room2); + + // When I restart + saveAndReload(); + + // Then the room is still read + assertRead(room2); + }); + it("A room where all reactions in threads are read is still read after restart", () => { + // Given multiple threads with reactions exist and are read + goTo(room1); + assertRead(room2); + receiveMessages(room2, [ + "Msg1", + threadedOff("Msg1", "Reply1a"), + reactionTo("Reply1a", "r"), + "Msg2", + threadedOff("Msg1", "Reply1b"), + threadedOff("Msg2", "Reply2a"), + reactionTo("Msg1", "e"), + threadedOff("Msg2", "Reply2b"), + reactionTo("Reply2a", "a"), + reactionTo("Reply2b", "c"), + reactionTo("Reply1b", "t"), + ]); + assertUnread(room2, 6); + goTo(room2); + openThread("Msg1"); + assertReadThread("Msg1"); + openThread("Msg2"); + assertReadThread("Msg2"); + assertRead(room2); + goTo(room1); + + // When I restart + saveAndReload(); + + // Then the room is still read + assertRead(room2); + goTo(room2); + assertReadThread("Msg1"); + assertReadThread("Msg2"); }); - it.skip("Marking a room as read after a reaction in a thread makes it read", () => {}); - it.skip("Reacting to a thread message after marking as read makes the room unread", () => {}); - it.skip("A room with a reaction to a threaded message is still unread after restart", () => {}); - it.skip("A room where all reactions in threads are read is still read after restart", () => {}); }); describe("thread roots", () => { From 494119042ff07bd39b7de08f0bafdb06ba7641fe Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 19 Sep 2023 11:44:35 +0100 Subject: [PATCH 8/8] Wait before looking for the thread list, to let the room settle --- cypress/e2e/read-receipts/high-level.spec.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cypress/e2e/read-receipts/high-level.spec.ts b/cypress/e2e/read-receipts/high-level.spec.ts index 99b2c8b1841..bdec38a409c 100644 --- a/cypress/e2e/read-receipts/high-level.spec.ts +++ b/cypress/e2e/read-receipts/high-level.spec.ts @@ -431,6 +431,11 @@ describe("Read receipts", () => { function openThreadList() { cy.log("Open threads list"); + + // If we've just entered the room, the threads panel takes a while to decide + // whether it's open or not - wait here to give it a chance to settle. + cy.wait(200); + cy.findByTestId("threadsButton", { log: false }).then(($button) => { if ($button?.attr("aria-current") !== "true") { cy.findByTestId("threadsButton", { log: false }).click(); @@ -441,7 +446,8 @@ describe("Read receipts", () => { .should("exist") .then(($panel) => { const $button = $panel.find('.mx_BaseCard_back[title="Threads"]'); - // If the Threads back button is present then click it, the threads button can open either threads list or thread panel + // If the Threads back button is present then click it - the + // threads button can open either threads list or thread panel if ($button.length) { $button.trigger("click"); } @@ -454,6 +460,7 @@ describe("Read receipts", () => { } function assertReadThread(rootMessage: string) { + cy.log("Assert thread read", rootMessage); return getThreadListTile(rootMessage).within(() => { cy.get(".mx_NotificationBadge", { log: false }).should("not.exist"); });