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

Add security-user-settings-tab.spec.ts #10925

Merged
merged 2 commits into from
May 24, 2023
Merged

Add security-user-settings-tab.spec.ts #10925

merged 2 commits into from
May 24, 2023

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented May 16, 2023

This PR intends to add security-user-settings-tab.spec.ts as a follow-up to #10924. The test this PR suggests to add is simple for now, and other scenarios should be implemented later.

This suggests to take a Percy snapshot of AnalyticsLearnMoreDialog.

type: task

Signed-off-by: Suguru Hirahara luixxiul@users.noreply.github.com

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 May 16, 2023
@luixxiul luixxiul marked this pull request as ready for review May 17, 2023 15:42
@luixxiul luixxiul requested a review from a team as a code owner May 17, 2023 15:42
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 . Looks good with some small questions.

cy.initTestUser(homeserver, "Hanako");
});

cy.openUserSettings("Security");
Copy link
Contributor

Choose a reason for hiding this comment

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

This it-block does not contain any test (or I don't see it). What is the purpose of it?

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 have thought cy.openUserSettings itself included assertions in it and it was worth checking whether Security tab could be opened properly, but this can be surely removed in favor of the test below, which calls openUserSettings("Security").

I am going to remove this.

.should("exist")
.closest(".mx_Toast_toast")
.within(() => {
cy.get(".mx_Toast_buttons .mx_AccessibleButton_kind_danger_outline").click(); // Click "Dismiss"
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 click a button with the name „Dismiss“ here?
Then the comment would be unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right- I addressed it with 6df45a4.

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 @luixxiul 👍

@weeman1337 weeman1337 added this pull request to the merge queue May 24, 2023
Merged via the queue into matrix-org:develop with commit 7248878 May 24, 2023
@luixxiul luixxiul deleted the test-security-user-settings-tab2 branch May 24, 2023 13:42
it("should be rendered properly", () => {
cy.findByRole("button", { name: "Learn more" }).click();

cy.get(".mx_AnalyticsLearnMoreDialog_wrapper").percySnapshotElement("AnalyticsLearnMoreDialog");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

2 participants