-
Notifications
You must be signed in to change notification settings - Fork 365
[WIP] - Add element validation commands #9635
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
base: master
Are you sure you want to change the base?
[WIP] - Add element validation commands #9635
Conversation
{ | ||
[LABEL_CONFIG_KEYS.FOR_VALUE]: 'endDate', | ||
[LABEL_CONFIG_KEYS.EXPECTED_TEXT]: END_DATE_FIELD_LABEL, | ||
}, |
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 have to be honest, this is hard for me to read. I would expect the validations to be an array of key, value pairs where the key should be the identifier/locator for the lookup, and the value is the expected value. Maybe we should in-line the constant values here so it's clear what the test is doing. In other words, what we're hiding in constants here is directly relevant to the test I think it's fine to have some duplication. There's no need to abstract them away or add a layer of indirection.
What do they all have in common? A CSS selector or similar and a contain.text assertion. Maybe an array of key/value pairs where the key is the selector and the value is the text to be asserted?
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.
key/value pairs where the key is the selector and the value is the text to be asserted?
If we just turn this into a simple object with selectors/locators(id, for...) as keys and expected text as values, it’d look something like this:
Cypress.Commands.add('validateFormLabels', (labelConfigs) => {
if (!labelConfigs || typeof labelConfigs !== 'object' || Array.isArray(labelConfigs)) {
cy.logAndThrowError('labelConfigs must be a non-array object');
}
const entries = Object.entries(labelConfigs);
if (!entries.length) {
cy.logAndThrowError('labelConfigs object cannot be empty');
}
entries.forEach(([forValue, expectedText]) => {
if (!forValue) {
cy.logAndThrowError('Label selector (for attribute) cannot be empty');
}
const labelCheck = cy
.getFormLabelByForAttribute({ forValue })
.should('be.visible');
if (expectedText) {
labelCheck.and('contain.text', expectedText);
}
});
});
The issue here is that when we need to support additional options(like in the other 2 commands validateFormFields
& validateFormFooterButtons
), the only way to do so is by adding a delimiter (like - or |) in the value string(which currently only contains the expected text) and then do the other options logic in the command, which feels quite messy to me.
For example, if we want to scroll the label element into view before making assertions, we’d have to write something like:
cy.validateFormLabels({ timezone: Timezone | scrollIntoView' });
But with our current structure we can just add a new key:
cy.validateFormLabels([
{
scrollIntoView: true,
forValue: 'timezone',
expectedVale: 'Timezone',
},
]);
what we're hiding in constants here is directly relevant to the test I think it's fine to have some duplication
If we're referring to the config-keys object(LABEL_CONFIG_KEYS
), its main purpose is to prevent the use of unknown or misspelled keys. For example, if I am using "expectedVale" instead of "expectedValue":
cy.validateFormLabels([
{
forValue: 'timezone',
expectedVale: 'Timezone',
},
{
forValue: 'startDate',
expectedVale: 'Start Date',
},
]);

The test will still run without throwing any errors, unless we explicitly validate that every key passed to the command exists in the config-keys object(LABEL_CONFIG_KEYS
).
Let me know if anything stands out or needs tweaking...
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.
hmm, good point.. I'm not sure what's preferable... the current interface is hard to reason about for me right now. I feel like the tester should know what fields it wants to validate... if they typo the field, we should assert it's wrong so they can fix it. I "think" class/ids in the dom are less likely to change than message catalogs / strings so I'm fine with the repetitive nature of putting the field id/class/selector in the key. I'm not sure.
The test will still run without throwing any errors, unless we explicitly validate that every key passed to the command exists in the config-keys object(LABEL_CONFIG_KEYS).
I think if you assert a field has a value and the field doesn't exist, we should raise an error.
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 think if you assert a field has a value and the field doesn't exist, we should raise an error.
Cypress will fail if the field isn’t found (like if the ID is incorrect):
I was referring to configuration keys (like forValue
, expectedValue
). Earlier, typos in these keys wouldn't cause any errors, the test would still execute. I've now added validation to catch unrecognised keys upfront.


8cc33a7
to
da4ffee
Compare
Follow up work from #9535:
PR introducing commands for form element validation, focusing on labels, user inputs, and footer buttons - more can be added later.
@miq-bot assign @jrafanie
@miq-bot add-label cypress
@miq-bot add-label test
@miq-bot add-label enhancement