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

Add E2E test of MessageEditHistoryDialog #10406

merged 21 commits into from
Mar 30, 2023

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 18, 2023

This PR intends to add E2E test of MessageEditHistoryDialog.

The main intention of this PR is to make sure that EventTiles on the dialog keep being rendered in the same way after mx_EventTile:not([data-layout=bubble]) is removed from _EventTile.pcss.

Closes element-hq/element-web#24902

For #9033 (checking EventTiles on MessageEditHistoryDialog)

type: task

Signed-off-by: Suguru Hirahara [email protected]

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@github-actions github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Task Refactoring, enabling or disabling functionality, other engineering tasks labels Mar 18, 2023
@luixxiul luixxiul changed the title Add E2E test of MessageEditHistoryDialog Add E2E test of MessageEditHistoryDialog Mar 18, 2023
@luixxiul luixxiul marked this pull request as ready for review March 18, 2023 18:01
@luixxiul luixxiul requested a review from a team as a code owner March 18, 2023 18:01
Comment on lines 87 to 96
cy.get(".mx_RoomView_MessageList").percySnapshotElement("Edited message on IRC layout", { percyCSS });
cy.setSettingValue("layout", null, SettingLevel.DEVICE, Layout.Group);
cy.get(".mx_RoomView_MessageList").percySnapshotElement("Edited message on modern layout", { percyCSS });
cy.setSettingValue("useCompactLayout", null, SettingLevel.DEVICE, true);
cy.get(".mx_RoomView_MessageList").percySnapshotElement("Edited message on compact modern layout", {
percyCSS,
});
cy.setSettingValue("useCompactLayout", null, SettingLevel.DEVICE, false); // Cancel the compact modern layout
cy.setSettingValue("layout", null, SettingLevel.DEVICE, Layout.Bubble);
cy.get(".mx_RoomView_MessageList").percySnapshotElement("Edited message on bubble layout", { percyCSS });

This comment was marked as outdated.

@luixxiul

This comment was marked as outdated.

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.

Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @luixxiul

Please have a look at my comments. I am happy to discuss any of them if you have a different opinion

// 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.

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.

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.

});

// 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.

// 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()

.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.

Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>

// Assert that zero block start padding is applied to mx_EventTile as expected
// See: .mx_EventTile on _EventTile.pcss
cy.get(".mx_EventTile").should("have.css", "padding-block-start", "0px");
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 is for #9033 (checking EventTiles on MessageEditHistoryDialog), one of the main aims of this PR.

Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test @luixxiul

});

// 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.

@weeman1337 weeman1337 added the X-Needs-Percy Whether to run Percy screenshot tests in Merge Queue label Mar 27, 2023
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Lets wait for a Percy run.

Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

@luixxiul can you push an empty commit to trigger CI again with Percy?

(Would be nice if you can enable „Allow edit by maintainers“ for your PRs. This can speed up things in some cases.)

@luixxiul

This comment was marked as outdated.

@luixxiul

This comment was marked as resolved.

Signed-off-by: Suguru Hirahara <[email protected]>
Have Percy take a snapshot of the dialog including its wrapper.

Signed-off-by: Suguru Hirahara <[email protected]>
@luixxiul
Copy link
Contributor Author

luixxiul commented Mar 27, 2023

The latest result indeed looks better (the timestamps next to the messages are hidden):

1

Signed-off-by: Suguru Hirahara <[email protected]>
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Thanks @luixxiul . Looks good now.

@luixxiul
Copy link
Contributor Author

@weeman1337 Thanks for the review!

@weeman1337 weeman1337 merged commit beb8861 into matrix-org:develop Mar 30, 2023
@luixxiul luixxiul deleted the test-message-edit-dialog branch March 30, 2023 09:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks X-Needs-Percy Whether to run Percy screenshot tests in Merge Queue Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add E2E test of MessageEditHistoryDialog
2 participants