VACMS-15783: Alt-Text Validation Testing #16159
Conversation
There was a problem hiding this comment.
Not sure what's going on but the tests don't seem to be passing here. Also, I'll defer to @edmund-dunn for a more thorough review but I had a few questions.
| @@ -291,7 +291,8 @@ | |||
| "dealerdirect/phpcodesniffer-composer-installer": true, | |||
| "php-http/discovery": true, | |||
| "cweagans/composer-patches": true, | |||
| "orakili/composer-drupal-info-file-patch-helper": true | |||
| "orakili/composer-drupal-info-file-patch-helper": true, | |||
| "digitalrevolution/php-codesniffer-baseline": true | |||
There was a problem hiding this comment.
Strange, this should have been updated in 531ad7f. Can you double check why this change show up here?
There was a problem hiding this comment.
@tonytaylor you might need to merge in a fresh copy of main?
There was a problem hiding this comment.
This branch isn't based off main, but 15785-alt-text-validation. So it should merge from that branch, not main. If that branch is being used as an integration branch, then it needs to be kept updated to avoid freaking out passersby.
Since this merged from main, it now includes changes to main that weren't in 15785-alt-text-validation, which makes this PR longer and weird AF to review.
There was a problem hiding this comment.
In general, I also recommend for PRs to use rebase instead of merge to simplify the commit history for review purposes. Let me know if you need help with untangling this @tonytaylor.
There was a problem hiding this comment.
@JunTaoLuo Thanks, I appreciate it. I could definitely stand to work on my git-fu.
There was a problem hiding this comment.
Feel free to ping me on Slack whenever you want to sort out the git history tomorrow!
There was a problem hiding this comment.
Noting here that this is needed because the base Tugboat image is based off of main and the base branch 15785-alt-text-validation is out of date with main, hence the need to add this tactical fix. This change will go away when the base branch is updated but we won't be blocking on that to merge the PR.
tests/cypress/integration/step_definitions/common/i_create_an_image_with_alt_text.js
Outdated
Show resolved
Hide resolved
tests/cypress/integration/step_definitions/common/i_create_an_image_with_alt_text.js
Outdated
Show resolved
Hide resolved
tests/cypress/integration/features/platform/alt_text_validation.feature
Outdated
Show resolved
Hide resolved
GitHub Workflows (.github/workflows/*.yml)Have you...
|
tonytaylor
force-pushed
the
VACMS-15783-alt-text-validation-testing
branch
from
724d0e0 to
a4ce74f
Compare
|
va/tests/status-error:
|
|
The cypress tests are failing in the base branch as well so the flakiness is probably introduced there. @tonytaylor can you update the PR description? Otherwise I think this PR is ready to be merged. |
* VACMS-15435: hard validation for alt text field * VACMS-15436: clientside validation for alt text field * VACMS-15436: updated composer.lock * VACMS-15436: fix linting errors * VACMS-15436: one more linting error fixed * VACMS-15436: more linting fixes * VACMS-15785: updated composer.lock * VACMS-15962: theming changes for image form * VACMS-15962: fix linting errors * VACMS-15962: final fix * VACMS-15783: Alt-Text Validation Testing (#16159) * VACMS-15783 adds alt-text validation tests * VACMS-15785: update composer.lock, fix merge error * VACMS-15785: fix for spacing around error * VACMS-15785: updated composer.lock * Updates content * smdh --------- Co-authored-by: Tony Taylor <[email protected]> Co-authored-by: Nathan Douglas <[email protected]>
Description
Relates to #15783
Testing done
Added Cypress Tests in support of #15785
Select Team for PR review
CMS TeamPublic websitesFacilitiesUser supportAccelerated Publishing