Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Files] Add view file information modal test #2130

Merged
merged 14 commits into from
May 17, 2022

Conversation

juans-chainsafe
Copy link
Contributor

closes #2124

@juans-chainsafe juans-chainsafe added the Testing Added to issues to signal that it is part of an epic label May 13, 2022
@juans-chainsafe juans-chainsafe self-assigned this May 13, 2022
@render
Copy link

render bot commented May 13, 2022

@render
Copy link

render bot commented May 13, 2022

@render
Copy link

render bot commented May 13, 2022

@github-actions github-actions bot added the Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes. label May 13, 2022
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Left 2 small comments, looks good otherwise, thanks for adding this test :)

packages/files-ui/cypress/tests/file-management-spec.ts Outdated Show resolved Hide resolved
@asnaith
Copy link
Member

asnaith commented May 13, 2022

Looks great for the most part, just some very minor suggestions.

I will approve once the issue with test execution in CI / headless is resolved :)

…b.com:ChainSafe/ui-monorepo into mnt/add-view-file-information-modal-test-2124
permissions: ['clipboardReadWrite', 'clipboardSanitizedWrite'],
origin: window.location.origin,
},
}))
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to take the above block out of the test layer and move it to cypress/support/index.ts.

If we get it working there then clipboard ability will be turned on for all tests by default which I think is what is best in this scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, done!

origin: window.location.origin,
},
}))
});
Copy link
Member

Choose a reason for hiding this comment

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

Good to put it in this area of code 👍

Just a small tidy up needed, the indentation is a little off in that block of code and the ; is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! and I added as well the "grant clipboard permissions" in storage

fileInfoModal.closeButton().click()
fileInfoModal.body().should("not.exist")
})

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Member

@asnaith asnaith 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 adding this and great job on getting Cypress to read the clipboard contents

Comment on lines +37 to +48

before(() => {
// grant clipboard read permissions to the browser
cy.wrap(
Cypress.automation("remote:debugger:protocol", {
command: "Browser.grantPermissions",
params: {
permissions: ["clipboardReadWrite", "clipboardSanitizedWrite"],
origin: window.location.origin,
}
})
)
Copy link
Collaborator

@Tbaut Tbaut May 16, 2022

Choose a reason for hiding this comment

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

I'd advise to only add things when they are needed. In this case, I would personally prefer to add this to storage cypress in the same PR that will actually use this reading from the clipboard in a test. I bet this will come shortly though, so I don't intend to bother you with that in this specific case :)

Copy link
Member

@asnaith asnaith May 16, 2022

Choose a reason for hiding this comment

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

@Tbaut This was my fault, I advised Juan to add to storage too as I knew we'd want the ability in Storage UI (when copying the API keys).

I hear what you are saying though - it could be confusing to have code that is not being utilized in the project and adding it when we need it and not in advance prevents that.

I believe we had this chat in the past so my bad for not remembering that 🤦‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

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

No prob at all, it's a small thing.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Great job, that's elegantly solved

@juans-chainsafe juans-chainsafe merged commit 443c575 into dev May 17, 2022
@juans-chainsafe juans-chainsafe deleted the mnt/add-view-file-information-modal-test-2124 branch May 17, 2022 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing Added to issues to signal that it is part of an epic Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Files] Add view file information via modal option test
3 participants