Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add E2E test of MessageEditHistoryDialog #10406

Merged
merged 21 commits into from
Mar 30, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
272 changes: 266 additions & 6 deletions cypress/e2e/editing/editing.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ limitations under the License.
import type { EventType, MsgType } from "matrix-js-sdk/src/@types/event";
import type { ISendEventResponse } from "matrix-js-sdk/src/@types/requests";
import type { IContent } from "matrix-js-sdk/src/models/event";
import { SettingLevel } from "../../../src/settings/SettingLevel";
import { HomeserverInstance } from "../../plugins/utils/homeserver";
import Chainable = Cypress.Chainable;

Expand All @@ -41,12 +42,16 @@ function mkPadding(n: number): IContent {

describe("Editing", () => {
let homeserver: HomeserverInstance;
let roomId: string;

beforeEach(() => {
cy.startHomeserver("default").then((data) => {
homeserver = data;
cy.initTestUser(homeserver, "Edith").then(() => {
cy.injectAxe();
cy.createRoom({ name: "Test room" }).then((_room1Id) => {
roomId = _room1Id;
}),
cy.injectAxe();
});
});
});
Expand All @@ -55,14 +60,269 @@ describe("Editing", () => {
cy.stopHomeserver(homeserver);
});

it("should close the composer when clicking save after making a change and undoing it", () => {
cy.createRoom({ name: "Test room" }).as("roomId");
it("should render and interact with the message edit history dialog", () => {
cy.visit("/#/room/" + roomId);

// Send "Message"
sendEvent(roomId);

cy.get(".mx_RoomView_MessageList").within(() => {
// Edit message
cy.get(".mx_EventTile_last").realHover();
cy.get(".mx_EventTile_last .mx_MessageActionBar_optionsButton", { timeout: 1000 })
.should("exist")
.realHover()
.get('.mx_EventTile_last [aria-label="Edit"]')
.click({ force: false });
cy.get(".mx_BasicMessageComposer_input").type("{selectAll}{del}Massage{enter}");
});

// Assert that the edit label is visible
cy.get(".mx_EventTile_edited").should("be.visible");

cy.get(".mx_RoomView_MessageList").within(() => {
// Assert that the message was edited
cy.contains(".mx_EventTile", "Massage")
.should("exist")
.within(() => {
// Click to display the message edit history dialog
cy.contains(".mx_EventTile_edited", "(edited)").click();
});
});

cy.get(".mx_Dialog").within(() => {
// Assert that the message edit history dialog is rendered
cy.get(".mx_MessageEditHistoryDialog").within(() => {
// Assert that CSS styles which cannot be detected with snapshots are applied as expected
cy.get("li").should("have.css", "clear", "both");
cy.get(".mx_EventTile")
.should("have.css", "max-width", "100%")
.should("have.css", "clear", "both")
.should("have.css", "position", "relative")
.should("have.css", "padding-block-start", "0px");
cy.get(".mx_EventTile .mx_MessageTimestamp")
.should("have.css", "position", "absolute")
.should("have.css", "inset-inline-start", "0px")
.should("have.css", "text-align", "center");
cy.get(".mx_EventTile .mx_EventTile_content").should("have.css", "margin-inline-end", "0px");
Copy link
Contributor Author

@luixxiul luixxiul Mar 23, 2023

Choose a reason for hiding this comment

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

This line also should be checked because this margin value is specific to the message edit history dialog (See this line for comparison) and it is difficult to detect a regression with a snapshot because text-align is not set to justify, which makes it hard to judge whether the margin setting is really respected or a line is wrapped just because of the length of a word, even if we use a sentence long enough to be wrapped as an example.

For comparison, languages such as Chinese/Japanese/Korean do not have the concept of wrapping a sentence based on the word length, nor hyphenation, therefore any sentence uses every space available inside a line before being wrapped (ie. the same number of characters on each line inside a paragraph), which essentially means that every wrapped sentence is aligned to be justified. This it not perfectly correct, but you could regard CJK characters as monospace in most cases. Therefore checking the margin here makes sense for those languages.

This also means that using a sample text in CJK would make it unnecessary to check this value, but I think it is not practical, considering the possibility of outputting tofu characters if your computer does not have a CJK font, etc., which increases the possibility of flaky test results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1336573 adds a comment about this.


// Assert that the date separator is rendered
cy.get("li:nth-child(1) .mx_DateSeparator").within(() => {
cy.get("h2").should("have.text", "Today");
});

// Assert that the edited message is rendered
cy.get("li:nth-child(2) .mx_EventTile").within(() => {
cy.get(".mx_EventTile_content").within(() => {
cy.get(".mx_EventTile_body").should("have.text", "Meassage");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment why it is „Meassage“ here?
Took me some moments to understand ⌛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, it should be noted (as Message and Massage and Meassage are all resemble :-D)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This commit b930982 added a comment to explain it.


cy.get(".mx_EventTile_body").within(() => {
// "e" was replaced with "a"
cy.get(".mx_EditHistoryMessage_deletion").should("have.text", "e");
cy.get(".mx_EditHistoryMessage_insertion").should("have.text", "a");
});
});
});

// Assert that the original message is rendered
cy.get("li:nth-child(3) .mx_EventTile").within(() => {
cy.get(".mx_EventTile_content .mx_EventTile_body").should("have.text", "Message");
});
});
});

// Exclude timestamps from a snapshot
const percyCSS = ".mx_MessageTimestamp { visibility: hidden !important; }";

// Take a snapshot
cy.get(".mx_Dialog .mx_MessageEditHistoryDialog").percySnapshotElement("Message edit history dialog", {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all the CSS checks probably unnecessary if a snapshot is taken here?
If we change the CSS structure this test will fail, even if the resulting view is the same.
In my opinion we should me more interested about the result here.

Copy link
Contributor Author

@luixxiul luixxiul Mar 23, 2023

Choose a reason for hiding this comment

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

I would agree with that checking these CSS styles of .mx_EventTile is unnecessary here, as they are not overridden by _MessageEditHistoryDialog.pcss.

Rather, this line below should be checked in case of a regression after removing !important flag eventually, considering Percy test is not going to be executed on each commit:

padding-top: 0 !important; /* Override mx_EventTile:not([data-layout="bubble"]) */

Since we do not have a Percy test for timestamps yet (timestamps are hidden to prevent a flaky result), checking their style rules should be worth. I cannot remember exactly when, but I think the position has regressed once due to an issue related to CSS style rules inheritance.

Copy link
Contributor Author

@luixxiul luixxiul Mar 23, 2023

Choose a reason for hiding this comment

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

This commit 203c8c8 addresses this. It removes redundant checks, saving ones for style rules which are hard to detect or cannot be detected with a snapshot.

percyCSS,
widths: [704], // See: .mx_Dialog_fixedWidth max-width
});

cy.get(".mx_Dialog").within(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to join the three within?

Something like

cy.get(".mx_Dialog .mx_MessageEditHistoryDialog li:nth-child(2) .mx_EventTile").within(() => {

(may also apply to other places)

This comment was marked as outdated.

Copy link
Contributor Author

@luixxiul luixxiul Mar 23, 2023

Choose a reason for hiding this comment

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

I found there were cases which actually should be joined. Working on it..

Copy link
Contributor Author

@luixxiul luixxiul Mar 23, 2023

Choose a reason for hiding this comment

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

This commit 5c58427 addresses this. It does not join every within for readability and to clarify the relationship between mx_TextInputDialog and mx_MessageEditHistoryDialog under mx_Dialog.

// Click "Remove" button on MessageActionBar
cy.get(".mx_MessageEditHistoryDialog").within(() => {
// Assert that the edited message is rendered
cy.get("li:nth-child(2) .mx_EventTile").within(() => {
// Click the "Remove" button
cy.get(".mx_EventTile_line")
.realHover()
.contains(".mx_AccessibleButton", "Remove")
.click({ force: false });
});
});

// Do nothing and close the dialog to confirm that the message edit history dialog is rendered
cy.get(".mx_TextInputDialog").within(() => {
cy.get("[aria-label='Close dialog']").click();
});

// Assert that the message edit history dialog is rendered again after it was closed
cy.get(".mx_MessageEditHistoryDialog").within(() => {
// Assert that the edited message is rendered again
cy.get("li:nth-child(2) .mx_EventTile").within(() => {
cy.get(".mx_EventTile_content").within(() => {
cy.get(".mx_EventTile_body").should("have.text", "Meassage");
});

// Click the "Remove" button again
cy.get(".mx_EventTile_line")
.realHover()
.contains(".mx_AccessibleButton", "Remove")
.click({ force: false });
});
});

// This time remove the message really
cy.get(".mx_TextInputDialog").within(() => {
cy.get(".mx_TextInputDialog_input").type("This is a test."); // Reason
cy.contains("[data-testid='dialog-primary-button']", "Remove").click();
});

// Assert that the message edit history dialog is rendered again
cy.get(".mx_MessageEditHistoryDialog").within(() => {
// Assert that the date is rendered
cy.get("li:nth-child(1) .mx_DateSeparator").within(() => {
cy.get("h2").should("have.text", "Today");
});

// Assert that the original message is rendered on the dialog
cy.get("li:nth-child(2) .mx_EventTile").within(() => {
cy.get(".mx_EventTile_content .mx_EventTile_body").should("have.text", "Message");
});

// Assert that the edited message is gone
cy.get("li:nth-child(3) .mx_EventTile").should("not.exist");
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be better to check for the non-existence of „Meassage“ here.
The test already heavily relies on CSS structure.
At least in this particular place it can be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd agree, let me check how it should be changed...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This commit a457fd0 addresses this.


// Close the dialog
cy.get("[aria-label='Close dialog']").click();
});
});

cy.get<string>("@roomId").then((roomId) => {
sendEvent(roomId);
cy.visit("/#/room/" + roomId);
// Assert that the main timeline is rendered
cy.get(".mx_RoomView_MessageList").within(() => {
cy.get(".mx_EventTile_last").within(() => {
// Assert that the placeholder is rendered
cy.contains(".mx_RedactedBody", "Message deleted");
});
});
});

it("should render 'View Source' button in developer mode on the message edit history dialog", () => {
cy.visit("/#/room/" + roomId);

// Send "Message"
sendEvent(roomId);

cy.get(".mx_RoomView_MessageList").within(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to extract shared code to their own functions?
This would make the tests better maintainable.

Copy link
Contributor Author

@luixxiul luixxiul Mar 23, 2023

Choose a reason for hiding this comment

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

I've added some functions:

  • editLastMessage()
  • clickButtonRemove()
  • clickButtonViewSource()
  • clickEditedMessage()

// Edit message
cy.get(".mx_EventTile_last").realHover();
cy.get(".mx_EventTile_last .mx_MessageActionBar_optionsButton", { timeout: 1000 })
.should("exist")
.realHover()
.get('.mx_EventTile_last [aria-label="Edit"]')
.click({ force: false });
cy.get(".mx_BasicMessageComposer_input").type("{selectAll}{del}Massage{enter}");
});

// Assert that the edit label is visible
cy.get(".mx_EventTile_edited").should("be.visible");

cy.get(".mx_RoomView_MessageList").within(() => {
// Assert that the message was edited
cy.contains(".mx_EventTile", "Massage")
.should("exist")
.within(() => {
// Click to display the message edit history dialog
cy.contains(".mx_EventTile_edited", "(edited)").click();
});
});

cy.get(".mx_Dialog").within(() => {
cy.get(".mx_MessageEditHistoryDialog").within(() => {
// Assert that the original message is rendered
cy.get("li:nth-child(3) .mx_EventTile").within(() => {
// Assert that "View Source" is not rendered
cy.get(".mx_EventTile_line")
.realHover()
.contains(".mx_AccessibleButton", "View Source")
.should("not.exist");
});
});

// Close the dialog
cy.get("[aria-label='Close dialog']").click();
});

// Enable developer mode
cy.setSettingValue("developerMode", null, SettingLevel.ACCOUNT, true);

cy.get(".mx_RoomView_MessageList").within(() => {
cy.contains(".mx_EventTile", "Massage")
.should("exist")
.within(() => {
// Click again to display the message edit history dialog
cy.contains(".mx_EventTile_edited", "(edited)").click();
});
});

cy.get(".mx_Dialog").within(() => {
cy.get(".mx_MessageEditHistoryDialog").within(() => {
// Assert that the edited message is rendered
cy.get("li:nth-child(2) .mx_EventTile").within(() => {
// Assert that "Remove" buttons for the edited message exists
cy.get(".mx_EventTile_line").realHover().contains(".mx_AccessibleButton", "Remove").should("exist");

// Assert that "View Source" button is rendered and click it
cy.get(".mx_EventTile_line")
.realHover()
.contains(".mx_AccessibleButton", "View Source")
.should("exist")
.click();
});
});

// Assert that view source dialog is rendered
cy.get(".mx_ViewSource").within(() => {
// Close the dialog
cy.get("[aria-label='Close dialog']").click();
});

cy.get(".mx_MessageEditHistoryDialog").within(() => {
// Assert that the original message is rendered
cy.get("li:nth-child(3) .mx_EventTile").within(() => {
// Assert that "Remove" button for the original message does not exist
cy.get(".mx_EventTile_line")
.realHover()
.contains(".mx_AccessibleButton", "Remove")
.should("not.exist");

// Assert that "View Source" button is rendered and click it
cy.get(".mx_EventTile_line")
.realHover()
.contains(".mx_AccessibleButton", "View Source")
.should("exist")
.click();
});
});

// Assert that view source dialog is rendered
cy.get(".mx_ViewSource").within(() => {
// Close the dialog
cy.get("[aria-label='Close dialog']").click();
});
});

// Disable developer mode
cy.setSettingValue("developerMode", null, SettingLevel.ACCOUNT, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be necessary as we start with a fresh account every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is right. It was removed by 952f370.

});

it("should close the composer when clicking save after making a change and undoing it", () => {
cy.visit("/#/room/" + roomId);

sendEvent(roomId);

// Edit message
cy.contains(".mx_RoomView_body .mx_EventTile .mx_EventTile_line", "Message").within(() => {
cy.get('[aria-label="Edit"]').click({ force: true }); // Cypress has no ability to hover
Expand Down
6 changes: 5 additions & 1 deletion src/components/views/messages/EditHistoryMessage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,13 @@ export default class EditHistoryMessage extends React.PureComponent<IProps, ISta
);
}

const classes = classNames("mx_MessageActionBar", {
"mx_MessageActionBar--developer--not": !SettingsStore.getValue("developerMode"),
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should have not been added here. Removing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was removed with 77db44c.

// disabled remove button when not allowed
return (
<div className="mx_MessageActionBar">
<div className={classes}>
{redactButton}
{viewSourceButton}
</div>
Expand Down