-
Notifications
You must be signed in to change notification settings - Fork 16
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
272 move filepreview permissions #290
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, this refactor gives me such peace of mind. I think the permissions code is much easier to read now.
Regarding the Cypress timeout error, I believe that in end-to-end tests, it's nearly impossible not to add some wait() here and there, although it's not advisable. I think we'll have to rely on extending the timeout for specific cy.wrap() calls and on using it('test description', { retries: 3 }) in some cases.
I've left a few comments; the main one is that we need to merge the FilePermissions and FileUserPermissions models
package.json
Outdated
@@ -107,6 +107,7 @@ | |||
"@auto-it/released": "10.46.0", | |||
"@csstools/postcss-cascade-layers": "3.0.1", | |||
"@cypress/code-coverage": "3.10.7", | |||
"@cypress/grep": "^4.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the ^ symbol to avoid unexpected upgrades?
.../file-actions/file-actions-cell/file-action-buttons/access-file-menu/AccessFileMenu.spec.tsx
Outdated
Show resolved
Hide resolved
tests/e2e-integration/integration/files/FileJSDataverseRepository.spec.ts
Outdated
Show resolved
Hide resolved
tests/e2e-integration/integration/files/FileJSDataverseRepository.spec.ts
Outdated
Show resolved
Hide resolved
// @ts-ignore | ||
import registerCypressGrep from '@cypress/grep' | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-call | ||
registerCypressGrep() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find where the grep library is being used, is it to use the cy.wait(500) command in the integration test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used it from the command line when I was testing locally. For example,
npx cypress run --spec tests/e2e-integration/e2e/sections/dataset/Dataset.spec.tsx --browser chrome --env grep="loads the restricted files when the user is logged in as owner",burn=10
Even though it's not a part of the automated test, I thought it would be good to have in the project, to make it easier to run tests like this locally, for timing errors. Does that sound ok, or should I keep it part of my local environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! We could document this command in the Dataverse Frontend Guide so that everyone is aware that tests can be run with this command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, in the meantime I will update the current README to document that.
… File.ts, rename FileUserPermissions.ts to FilePermissions.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few remaining methods that need to be removed
// @ts-ignore | ||
import registerCypressGrep from '@cypress/grep' | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-call | ||
registerCypressGrep() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! We could document this command in the Dataverse Frontend Guide so that everyone is aware that tests can be run with this command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Can you resolve merge conflicts? If I am not wrong, they are related to #285 merge |
…into 272-move-filepreview-permissions
272 move filepreview permissions
What this PR does / why we need it:
The original purpose of this PR was to the file permissions hook, which was believed to be causing a timing error in Cypress e2e tests.
After refactoring the code to remove the hook, the error is still occuring! For example in this run:
https://github.com/IQSS/dataverse-frontend/actions/runs/7671731412/job/20910744807
I did some more reading about Cypress handling of click actions, and found this article, which explains that sometimes click() fails because Cypress clicks the button before the event handler is attached:
https://www.cypress.io/blog/2019/01/22/when-can-the-test-click.
This article suggests using cypress-pipe to retry the click until the event handler is attached, but I couldn't get it to work in Electron, and when I tried to switch headless mode to Chrome I ran into another issue: cypress-io/cypress#14175
So I added a cy.wait() after finding the button, and before clicking, and that fixed it.
I added another library, cypress-grep, that allows a flaky test to be run multiple times. Without the cy.wait(), it would fail about 25% of the time, and with cy.wait() it succeeded for up to 50 runs, so I think that fixed it.
Also on this PR another unrelated Cypress failure is happening, I created an issue for it here #291
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: