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

Remove mx_HeaderButtons class and add a test #10713

Merged
merged 8 commits into from
May 5, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
27 changes: 0 additions & 27 deletions cypress/e2e/create-room/create-room.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,31 +64,4 @@ describe("Create Room", () => {
cy.findByText(topic);
});
});

it("should create a room with a long room name, which is displayed with ellipsis", () => {
let roomId: string;
const LONG_ROOM_NAME =
"Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore " +
"et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut " +
"aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum " +
"dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui " +
"officia deserunt mollit anim id est laborum.";

cy.createRoom({ name: LONG_ROOM_NAME }).then((_roomId) => {
roomId = _roomId;
cy.visit("/#/room/" + roomId);
});

// Wait until the room name is set
cy.get(".mx_RoomHeader_nametext").contains("Lorem ipsum");

// Make sure size of buttons on RoomHeader (except .mx_RoomHeader_name) are specified
// and the buttons are not compressed
// TODO: use a same class name
cy.get(".mx_RoomHeader_button").should("have.css", "height", "32px").should("have.css", "width", "32px");
cy.get(".mx_HeaderButtons > .mx_RightPanel_headerButton")
.should("have.css", "height", "32px")
.should("have.css", "width", "32px");
cy.get(".mx_RoomHeader").percySnapshotElement("Room header with a long room name");
});
});
2 changes: 1 addition & 1 deletion cypress/e2e/lazy-loading/lazy-loading.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ describe("Lazy Loading", () => {
}

function openMemberlist(): void {
cy.get(".mx_HeaderButtons").within(() => {
cy.get(".mx_RoomHeader").within(() => {
cy.findByRole("button", { name: "Room info" }).click();
});

Expand Down
182 changes: 182 additions & 0 deletions cypress/e2e/room/room-header.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
/*
Copyright 2023 Suguru Hirahara

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

/// <reference types="cypress" />

import { HomeserverInstance } from "../../plugins/utils/homeserver";
import { SettingLevel } from "../../../src/settings/SettingLevel";

describe("Room Header", () => {
let homeserver: HomeserverInstance;

beforeEach(() => {
cy.startHomeserver("default").then((data) => {
homeserver = data;
cy.initTestUser(homeserver, "Sakura");
});
});

afterEach(() => {
cy.stopHomeserver(homeserver);
});

it("should render seven buttons by default", () => {
cy.createRoom({ name: "Test Room" }).viewRoomByName("Test Room");

cy.get(".mx_RoomHeader").within(() => {
cy.findAllByRole("button").should("have.length", 7).should("be.visible");

cy.findByRole("button", { name: "Room options" }).should("be.visible");
cy.findByRole("button", { name: "Voice call" }).should("be.visible");
cy.findByRole("button", { name: "Video call" }).should("be.visible");
cy.findByRole("button", { name: "Search" }).should("be.visible");
cy.findByRole("button", { name: "Threads" }).should("be.visible");
cy.findByRole("button", { name: "Notifications" }).should("be.visible");
cy.findByRole("button", { name: "Room info" }).should("be.visible");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll preface this comment with saying I'm pretty new to cypress (and specifically the testing-library stuff with it).

To me, it seems that on line 40 we're already doing this test, aren't we? We're grabbing all the buttons, checking there are 7 of them, and I think checking that each thing with the role of button is visible (I could be misunderstanding how chaining a .should after a findAllBy* works though).

There may still be value in checking that we have these 7 specific buttons on the page though (ie checking the name of each button).

I've had a look to see if there's a neater way of doing this, nothing leaps out at me from the jest documentation. Perhaps something like:

const expectedButtonNames = [`Room options`...];
for (const name of expectedButtonNames) {
  cy.findByRole("button", { name}).should("be.visible");
}

Might make it a bit easier to read. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically the object of this test is to ensure that just those seven (not six or eight) buttons are rendered by default. This can be achieved by combining findAllBy query with should(have.length) by counting the exact number of the all found buttons.

Each findByRole query does ensure that button whose name is specified exists, but it cannot ensure that there was not another button rendered besides them unexpectedly.

Based on your hint, b20ceec should optimize the readability and the test flow.

});

cy.get(".mx_RoomHeader").percySnapshotElement("Room header", {
widths: [300, 600], // Magic numbers
Copy link
Contributor

Choose a reason for hiding this comment

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

Again I'm unfamiliar with percy too - what are these magic numbers for? Are they the widths the snapshots are performed at? How come you chose these values?

Copy link
Contributor Author

@luixxiul luixxiul Apr 28, 2023

Choose a reason for hiding this comment

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

Values specified with widths are used to set custom widths of a snapshot of the chained element. The default values are 1024 and 1920 (both in pixel) specified here on .percy.yml.

On this case, Percy creates a snapshot of mx_RoomHeader (mind this is not the whole UI) in 300px and 600px. Similarly, this snapshot was captured in 320px and 640px, in order to emulate the narrow UI.

The custom values are indeed relevant only for Room header - with a long room name, since the object of the snapshot with overriding the default values should be to emulate the narrow UI and ensure that room name overflow is properly hidden with horizontal ellipsis. I'm going to remove unrelated widths settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed with 2922c66.

});
});

it("should render the pin button for pinned messages card", () => {
cy.enableLabsFeature("feature_pinning");

cy.createRoom({ name: "Test Room" }).viewRoomByName("Test Room");

cy.getComposer().type("Test message{enter}");

cy.get(".mx_EventTile_last").realHover().findByRole("button", { name: "Options" }).click();

cy.findByRole("menuitem", { name: "Pin" }).should("be.visible").click();

cy.get(".mx_RoomHeader").within(() => {
cy.findByRole("button", { name: "Pinned messages" }).should("be.visible");
});
});

it("should render a very long room name without collapsing the buttons", () => {
const LONG_ROOM_NAME =
"Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore " +
"et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut " +
"aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum " +
"dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui " +
"officia deserunt mollit anim id est laborum.";

cy.createRoom({ name: LONG_ROOM_NAME }).viewRoomByName(LONG_ROOM_NAME);

cy.get(".mx_RoomHeader").within(() => {
// Wait until the room name is set
cy.get(".mx_RoomHeader_nametext").within(() => {
cy.findByText(LONG_ROOM_NAME).should("exist");
});

// Assert the size of buttons on RoomHeader (except mx_RoomHeader_name) are specified
// and the buttons are not compressed
cy.get(".mx_RoomHeader_button")
.should("have.length", 3)
.should("be.visible")
.should("have.css", "height", "32px")
.should("have.css", "width", "32px");
cy.get(".mx_RightPanel_headerButton") // TODO: use the same class name
.should("have.length", 3)
.should("be.visible")
.should("have.css", "height", "32px")
.should("have.css", "width", "32px");
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we have a test for 7 buttons earlier on, but we only check 6 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because mx_RoomHeader_name (the room options button with dropdown menu on click) is also a button element.

This is the mx_RoomHeader_name:
1

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'm going to add a comment about it. It is indeed confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed with de2dd1c

});

cy.get(".mx_RoomHeader").percySnapshotElement("Room header - with a long room name", {
widths: [300, 600], // Magic numbers
});
});

it("should have a button highlighted by being clicked", () => {
cy.createRoom({ name: "Test Room" }).viewRoomByName("Test Room");

cy.findByRole("button", { name: "Room info" })
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to test for all buttons here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently only buttons which display content on the right panel by being clicked are highlighted; Thread, Notification, and Room Info, though I am not sure if this is intended or not.

8c9eee1 should address this.

.click() // Highlight the button
.then(($btn) => {
// Note it is not possible to get CSS values of a pseudo class with "have.css".
const color = $btn[0].ownerDocument.defaultView // get window reference from element
.getComputedStyle($btn[0], "before") // get the pseudo selector
.getPropertyValue("background-color"); // get "background-color" value

// Assert the value is equal to $accent == hex #0dbd8b == rgba(13, 189, 139)
expect(color).to.eq("rgb(13, 189, 139)");
});

cy.get(".mx_RoomHeader").percySnapshotElement("Room header - with a highlighted button", {
widths: [300, 600], // Magic numbers
});
});

describe("with a video room", () => {
const createVideoRoom = () => {
// Enable video rooms. This command reloads the app
cy.setSettingValue("feature_video_rooms", null, SettingLevel.DEVICE, true);

cy.get(".mx_LeftPanel_roomListContainer", { timeout: 20000 })
.findByRole("button", { name: "Add room" })
.click();

cy.findByRole("menuitem", { name: "New video room" }).click();

cy.findByRole("textbox", { name: "Name" }).type("Test video room");

cy.findByRole("button", { name: "Create video room" }).click();

cy.viewRoomByName("Test video room");
};

it("should render buttons for room options, beta pill, invite, chat, and room info", () => {
createVideoRoom();

cy.get(".mx_RoomHeader").within(() => {
cy.findByRole("button", { name: "Room options" }).should("be.visible");
cy.findByRole("button", { name: "Video rooms are a beta feature Click for more info" }).should(
"be.visible",
); // Beta pill
cy.findByRole("button", { name: "Invite" }).should("be.visible");
cy.findByRole("button", { name: "Chat" }).should("be.visible");
cy.findByRole("button", { name: "Room info" }).should("be.visible");

// Assert that there is not a button except those buttons
cy.findAllByRole("button").should("have.length", 5);
});

cy.get(".mx_RoomHeader").percySnapshotElement("Room header - with a video room", {
widths: [300, 600], // Magic numbers
});
});

it("should render a working chat button which opens the timeline on a right panel", () => {
createVideoRoom();

cy.get(".mx_RoomHeader").findByRole("button", { name: "Chat" }).click();

// Assert that the video is rnedered
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny typo here in rnedered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed with d7a700d

cy.get(".mx_CallView video").should("exist");

cy.get(".mx_RightPanel .mx_TimelineCard")
.should("exist")
.within(() => {
// Assert that GELS is visible
cy.findByText("Sakura created and configured the room.").should("exist");
});
});
});
});
1 change: 0 additions & 1 deletion res/css/_components.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@
@import "./structures/_FilePanel.pcss";
@import "./structures/_GenericDropdownMenu.pcss";
@import "./structures/_GenericErrorPage.pcss";
@import "./structures/_HeaderButtons.pcss";
@import "./structures/_HomePage.pcss";
@import "./structures/_LargeLoader.pcss";
@import "./structures/_LeftPanel.pcss";
Expand Down
19 changes: 0 additions & 19 deletions res/css/structures/_HeaderButtons.pcss

This file was deleted.

2 changes: 1 addition & 1 deletion src/components/views/right_panel/HeaderButtons.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,6 @@ export default abstract class HeaderButtons<P = {}> extends React.Component<IPro
public abstract renderButtons(): JSX.Element;

public render(): React.ReactNode {
return <div className="mx_HeaderButtons">{this.renderButtons()}</div>;
return this.renderButtons();
}
}