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.

Sign up for GitHub

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

Jump to bottom

[Files] Add download file test #2139

Merged
merged 10 commits into from
May 23, 2022
Merged

[Files] Add download file test #2139

merged 10 commits into from
May 23, 2022

Conversation

juans-chainsafe
Copy link
Contributor

closes #2132

Probably is a good idea to add this test case to Storage too, if this PR goes well, I will add that case after that.

@juans-chainsafe juans-chainsafe added the Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes. label May 19, 2022
@juans-chainsafe juans-chainsafe self-assigned this May 19, 2022
@render
Copy link

render bot commented May 19, 2022

Your Render PR Server URL is https://chainsafe-components-stage-pr-2139.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-ca36ufvh8vl0h151h8r0.

@render
Copy link

render bot commented May 19, 2022

Your Render PR Server URL is https://files-ui-stage-pr-2139.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-ca36ugvh8vl0h151h92g.

@render
Copy link

render bot commented May 19, 2022

Your Render PR Server URL is https://storage-ui-stage-pr-2139.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-ca36ui7h8vl0h151h9k0.

FSM1
FSM1 approved these changes May 19, 2022
View reviewed changes
Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

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

I guess testing whether the downloaded file contents match what was uploaded is a bit out of scope? Might be an interesting addition to this test.

@juans-chainsafe
Copy link
Contributor Author

I guess testing whether the downloaded file contents match what was uploaded is a bit out of scope? Might be an interesting addition to this test.

I didn't though about that, but seems a nice validation, apparently Cypress has a function readFile so I think I can add it in this PR, doesn't seem very complicated.

asnaith
asnaith approved these changes May 19, 2022
View reviewed changes
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.

Nicely written test :) I can see why you used the const instead of a cypress alias. Makes sense for the path assertion.

Good use of the intercept and wait for reliability too 🚀

@juans-chainsafe
Copy link
Contributor Author

File validation added! @FSM1

tanmoyAtb
tanmoyAtb approved these changes May 20, 2022
View reviewed changes
Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Great work. Testing that the content matches is really reassuring !

Tbaut
Tbaut reviewed May 20, 2022
View reviewed changes
packages/files-ui/cypress/tests/file-management-spec.ts Outdated
downloadCompleteToast.body().should("be.visible")
downloadCompleteToast.closeButton().click()
cy.get<string>("@fileContent").then((fileContent) => {
cy.readFile(path.join(downloadsFolder, fileName))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since path isn't a direct dependency of this project, I wouldn't use it here, just to make sure nothing breaks unexpectedly. It's also super simple in this case.

Suggested change
cy.readFile(path.join(downloadsFolder, fileName))
cy.readFile(`${downloadsFolder}/${fileName}`)

Tbaut
Tbaut reviewed May 20, 2022
View reviewed changes
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 test Juan! I've just been messing a bit with the code, trying to not bring in any unneeded dependency.

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

Tbaut commented May 23, 2022

I didn't want to drag this branch too long so I pushed to it directly to:

  • remove the .only
  • remove some stray empty spaces (I'll get in touch to help you with some setup of your IDE to prevent that)
  • simplify a bit these name stuff

@Tbaut Tbaut merged commit 7071348 into dev May 23, 2022
@Tbaut Tbaut deleted the mnt/add-download-file-test-2132 branch May 23, 2022 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tbaut Tbaut Tbaut left review comments

@asnaith asnaith asnaith approved these changes

@tanmoyAtb tanmoyAtb tanmoyAtb approved these changes

@FSM1 FSM1 FSM1 approved these changes

Assignees

@juans-chainsafe juans-chainsafe

Labels
Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Files] Add download file from file browser test
5 participants
@juans-chainsafe @Tbaut @asnaith @tanmoyAtb @FSM1