-
Notifications
You must be signed in to change notification settings - Fork 37
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
added 3-click centring tests #1258
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.
Very nice!
ui/cypress/e2e/sampleControls.cy.js
Outdated
.invoke('val') | ||
/* eslint-disable-next-line promise/prefer-await-to-then */ |
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.
You could disable those two warnings for all Cypress specs in https://github.com/mxcube/mxcubeweb/blob/develop/ui/.eslintrc.js#L59
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.
Thanks for the comment, you are right that would be a better solution!
ui/cypress/e2e/sampleControls.cy.js
Outdated
cy.contains('button', '3-click centring').click(); | ||
cy.get('.form-control[name="diffractometer.phi"]') | ||
.invoke('val') | ||
.should('equal', omegaValue.toFixed(2)); |
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.
Would be good to act more like a real user here (and in other places like in the takeControl
command), with queries like cy.findByLabelText
(to find an input field with its label) or cy.findByRole
(to find headings, buttons, etc.).
It can be done later though, as it may require "fixing" the accessibility/markup of the relevant features (like labeling form inputs).
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.
As you mentioned for some elements, I could not use the mentioned queries. However, I agree with you that I often did not act like a real user (especially for takeControl
and clearSamples
commands) and some parts can be handled differently, I will take a look at it and make the necessary changes. Thanks for the comment!
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.
👍 Thanks guys !
cy.findByRole('alert', 'Error: There is no sample mounted').should( | ||
'be.visible', | ||
); | ||
}); | ||
|
||
it('Each click is rotating the sample by 90 degrees', () => { | ||
cy.mountSample(); | ||
cy.contains('button', '3-click centring').click(); | ||
cy.findByRole('button', { name: '3-click centring' }).click(); | ||
cy.get('.form-control[name="diffractometer.phi"]') | ||
.invoke('val') | ||
.then((value) => { |
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.
While I changed most of the cypress queries to act more like a real User, there are a few input forms that do not provide a corresponding label (resp. the text that should be the label is not correctly linked). This might be investigated in future steps.
Really nice :), we can improve the access to some of the elements in a future PR :) |
This includes 3 changes to the e2e tests: