From 0fedb20c9db376b0149e5f7a2c7c5c8f80cc378c Mon Sep 17 00:00:00 2001 From: Steven Dufresne Date: Tue, 16 Mar 2021 11:46:34 +0900 Subject: [PATCH] Experiment with better error messaging. (#19) --- .../tests/e2e/specs/a11y/index.test.js | 1 - .../tests/e2e/specs/page/index.test.js | 8 +- .../ui-check/tests/e2e/specs/ui/index.test.js | 78 +++++------ actions/ui-check/tests/utils/index.js | 1 + actions/ui-check/tests/utils/messaging.js | 18 ++- actions/ui-check/tests/utils/settings.js | 2 + docs/ui-errors.md | 127 ++++++++++++++++++ 7 files changed, 183 insertions(+), 52 deletions(-) create mode 100644 actions/ui-check/tests/utils/settings.js create mode 100644 docs/ui-errors.md diff --git a/actions/ui-check/tests/e2e/specs/a11y/index.test.js b/actions/ui-check/tests/e2e/specs/a11y/index.test.js index 1cadcc2..8841fe5 100644 --- a/actions/ui-check/tests/e2e/specs/a11y/index.test.js +++ b/actions/ui-check/tests/e2e/specs/a11y/index.test.js @@ -33,7 +33,6 @@ describe( 'Accessibility', () => { } ); } catch ( e ) { printMessage( noticeType, [ - `[ Accessibility - ${ accessibilityTest } Tests ]:`, `Running tests on ${ name } ${ getDefaultUrl( path, query diff --git a/actions/ui-check/tests/e2e/specs/page/index.test.js b/actions/ui-check/tests/e2e/specs/page/index.test.js index e5eb1d8..21d9ee0 100644 --- a/actions/ui-check/tests/e2e/specs/page/index.test.js +++ b/actions/ui-check/tests/e2e/specs/page/index.test.js @@ -37,6 +37,7 @@ describe.each( urls )( 'Test URL %s%s', ( url, queryString, bodyClass ) => { errorWithMessageOnFail( `${ url } does not contain a body class`, + 'page-should-contain-body-class', () => { expect( bodyClassName.split( ' ' ) ).toContain( bodyClass ); } @@ -48,7 +49,8 @@ describe.each( urls )( 'Test URL %s%s', ( url, queryString, bodyClass ) => { const pageError = await getPageError(); errorWithMessageOnFail( - `Page contains PHP errors: ${ JSON.stringify( pageError ) }`, + `Page contains PHP errors: ${ pageError }`, + 'page-should-not-have-php-errors', () => { expect( pageError ).toBe( null ); } @@ -63,6 +65,7 @@ describe.each( urls )( 'Test URL %s%s', ( url, queryString, bodyClass ) => { errorWithMessageOnFail( `${fullUrl} contains incomplete output. Make sure the page contains valid html.`, + 'page-should-have-complete-output', () => { expect( responseText ).toMatch( /<\/(html|rss)>\s*$/ ); } @@ -75,6 +78,7 @@ describe.each( urls )( 'Test URL %s%s', ( url, queryString, bodyClass ) => { errorWithMessageOnFail( `Expected to received a 200 status for ${ url }. Received ${ status }.`, + 'page-should-return-200-status', () => { expect( status ).toBe( 200 ); } @@ -95,6 +99,7 @@ describe.each( urls )( 'Test URL %s%s', ( url, queryString, bodyClass ) => { `Page should not contain javascript errors. Found ${ jsError }`, + 'browser-console-should-not-contain-errors', () => { expect( jsError ).toBeFalsy(); } @@ -150,6 +155,7 @@ describe.each( urls )( 'Test URL %s%s', ( url, queryString, bodyClass ) => { url, queryString.replace( '?', '' ) ) } is not an approved link.`, + 'page-should-not-have-unexpected-links', () => { expect( allowed_hosts ).toContain( hostname ); } diff --git a/actions/ui-check/tests/e2e/specs/ui/index.test.js b/actions/ui-check/tests/e2e/specs/ui/index.test.js index 73ab28f..cc69556 100644 --- a/actions/ui-check/tests/e2e/specs/ui/index.test.js +++ b/actions/ui-check/tests/e2e/specs/ui/index.test.js @@ -10,7 +10,7 @@ const pixelmatch = require( 'pixelmatch' ); */ import { createURL, - printMessage, + warnWithMessageOnFail, meetsChangeThreshold, getPercentOfOpaqueness, getFocusableElementsAsync, @@ -62,12 +62,7 @@ const testSkipLinks = async () => { ).toBeTruthy(); } catch ( e ) { throw new FailedTestException( [ - '[ Accessibility - Skip Link Test ]:', - "This tests whether the first 'tabbable' element on the page is a skip link with an '#' symbol.", - '[ Result ]', - 'Page: "/"', 'Unable to find a legitimate skip link. Make sure your theme includes skip links where necessary.', - 'You can read more about our expectations at https://make.wordpress.org/themes/handbook/review/required/#skip-links.', ] ); } @@ -78,16 +73,11 @@ const testSkipLinks = async () => { expect( el ).not.toBeNull(); } catch ( e ) { throw new FailedTestException( [ - '[ Accessibility - Required Tests ]:', - 'Running tests on "/".', - "This tests whether the first 'tabbable' element on the page is a skip link that has a matching element to navigate to.", - '[ Result ]', "The skip link doesn't have a matching element on the page.", `Expecting to find an element with an id matching: "${ activeElement.hash.replace( '#', '' ) }".`, - 'See https://make.wordpress.org/themes/handbook/review/required/#skip-links for more information.', ] ); } }; @@ -97,17 +87,6 @@ const testSkipLinks = async () => { * @param {Puppeteer|ElementHandle} listItem */ const testLiSubMenu = async ( listItem ) => { - const getFailureMessage = ( message ) => [ - '[ Accessibility - Submenu Test ]:', - 'This tests whether your navigational menus are accessible and working as expected.', - '[ Result ]', - 'Page: "/"', - "Your theme's navigation is not working as expected.", - message, - 'This test assumes your menu structure follows these guidelines: https://www.w3.org/WAI/tutorials/menus/structure.', - 'See https://make.wordpress.org/themes/handbook/review/required/#keyboard-navigation for more information.', - ]; - const link = await listItem.$( 'a' ); const submenu = await listItem.$( 'ul' ); @@ -148,9 +127,7 @@ const testLiSubMenu = async ( listItem ) => { if ( ! submenuIsVisible ) { throw new FailedTestException( - getFailureMessage( - 'Submenus should be become visible when :hover is added to the navigational menu.' - ) + 'Submenus should be become visible when :hover is added to the navigational menu.' ); } @@ -168,9 +145,7 @@ const testLiSubMenu = async ( listItem ) => { if ( ! ( await elementIsVisibleAsync( submenu ) ) ) { throw new FailedTestException( - getFailureMessage( - 'Submenus should become visible when :focus is added to the link through the main navigation.' - ) + 'Submenus should become visible when :focus is added to the link through the main navigation.' ); } } @@ -293,16 +268,10 @@ const testElementFocusState = async () => { // Break out of the loop forcefully throw new FailedTestException( [ - '[ Accessibility - Element Focus Test ]:', - "This tests that all 'focusable' elements have a visible :focus state.", - '[ Result ]', - 'Page: "/"', `The element "${ truncateElementHTML( domElement, 300 ) }" does not have enough visible difference when focused. `, - 'Download the screenshots to see the offending element.', - 'See https://make.wordpress.org/themes/handbook/review/required/#keyboard-navigation for more information.', ] ); } } @@ -361,13 +330,9 @@ const testForLogicalTabbing = async () => { ); throw new FailedTestException( [ - '[ Accessibility - Tabbing Test ]:', - "This tests that all 'focusable' elements on the page are tabbable in the expected order.", - '[ Result ]', - 'Page: "/"', + 'Tabbing is not working as expected', `Expected: ${ truncateElementHTML( expectedElement, 300 ) }`, `Current: ${ truncateElementHTML( currentFocus, 300 ) }`, - 'See https://make.wordpress.org/themes/handbook/review/required/#keyboard-navigation for more information.', ] ); } @@ -377,12 +342,18 @@ const testForLogicalTabbing = async () => { }; describe( 'Accessibility: UI', () => { - it( 'Should have skip links:', async () => { + it( 'Should have skip links', async () => { try { await testSkipLinks(); } catch ( ex ) { if ( ex instanceof FailedTestException ) { - printMessage( 'warnings', ex.messages ); + warnWithMessageOnFail( + ex.messages, + 'should-have-skip-links', + () => { + expect( false ).toEqual( true ); + } + ); } else { console.log( ex ); } @@ -394,7 +365,14 @@ describe( 'Accessibility: UI', () => { await testSubMenus(); } catch ( ex ) { if ( ex instanceof FailedTestException ) { - printMessage( 'warnings', ex.messages ); + warnWithMessageOnFail( + ex.messages, + 'should-have-appropriate-submenus', + + () => { + expect( false ).toEqual( true ); + } + ); } else { console.log( ex ); } @@ -406,7 +384,13 @@ describe( 'Accessibility: UI', () => { await testElementFocusState(); } catch ( ex ) { if ( ex instanceof FailedTestException ) { - printMessage( 'warnings', ex.messages ); + warnWithMessageOnFail( + ex.messages, + 'should-have-element-focus-state', + () => { + expect( false ).toEqual( true ); + } + ); } else { console.log( ex ); } @@ -421,7 +405,13 @@ describe( 'Accessibility: UI', () => { // We will make a gif to help understand what went wrong if ( process.env.UI_DEBUG ) { await makeGif( 1280, 800, SCREENSHOT_TABBING_TEST, 100 ); - printMessage( 'warnings', ex.messages ); + warnWithMessageOnFail( + ex.messages, + 'should-have-logical-tabbing', + () => { + expect( false ).toEqual( true ); + } + ); } } else { console.log( ex ); diff --git a/actions/ui-check/tests/utils/index.js b/actions/ui-check/tests/utils/index.js index 1271fb4..1956517 100644 --- a/actions/ui-check/tests/utils/index.js +++ b/actions/ui-check/tests/utils/index.js @@ -3,3 +3,4 @@ export * from './formatting'; export * from './messaging'; export * from './queries'; export * from './screenshots'; +export * from './settings'; \ No newline at end of file diff --git a/actions/ui-check/tests/utils/messaging.js b/actions/ui-check/tests/utils/messaging.js index d1244c1..9bd2180 100644 --- a/actions/ui-check/tests/utils/messaging.js +++ b/actions/ui-check/tests/utils/messaging.js @@ -1,4 +1,5 @@ const fs = require( 'fs' ); +import { ERROR_DOCS_URL } from './index'; /** * Removes some noise that exists in the testing framework error messages. @@ -17,7 +18,7 @@ export const cleanErrorMessage = ( msg ) => { * @param {array} lines */ const appendToLog = ( command, lines ) => { - const path = `../../logs/ui-check-${command}.txt`; + const path = `../../logs/ui-check-${ command }.txt`; fs.appendFileSync( path, [ '\n\n', ...lines ].join( '\n' ), { encoding: 'utf8', @@ -39,9 +40,14 @@ export const printMessage = ( command, lines ) => { * @param {string|string[]} message Messages to display * @param {function} testToRun The test that will be executed */ -const expectWithMessage = ( type, message, testToRun ) => { +const expectWithMessage = ( type, message, testId, testToRun ) => { const output = Array.isArray( message ) ? message : [ message ]; + if ( testId ) { + // Append information about the error. + output.push( `See: ${ ERROR_DOCS_URL }#${ testId }` ); + } + try { testToRun(); } catch ( e ) { @@ -54,8 +60,8 @@ const expectWithMessage = ( type, message, testToRun ) => { * @param {string|string[]} message Messages to output * @param {function} test Function to run */ -export const errorWithMessageOnFail = ( message, test ) => { - return expectWithMessage( 'errors', message, test ); +export const errorWithMessageOnFail = ( message, testId = false, test ) => { + return expectWithMessage( 'errors', message, testId, test ); }; /** @@ -63,6 +69,6 @@ export const errorWithMessageOnFail = ( message, test ) => { * @param {string|string[]} message Messages to output * @param {function} test Function to run */ -export const warnWithMessageOnFail = ( message, test ) => { - return expectWithMessage( 'warnings', message, test ); +export const warnWithMessageOnFail = ( message, testId = false, test ) => { + return expectWithMessage( 'warnings', message, testId, test ); }; diff --git a/actions/ui-check/tests/utils/settings.js b/actions/ui-check/tests/utils/settings.js new file mode 100644 index 0000000..b0a9cfe --- /dev/null +++ b/actions/ui-check/tests/utils/settings.js @@ -0,0 +1,2 @@ +export const ERROR_DOCS_URL = + 'https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md'; diff --git a/docs/ui-errors.md b/docs/ui-errors.md new file mode 100644 index 0000000..0411ef0 --- /dev/null +++ b/docs/ui-errors.md @@ -0,0 +1,127 @@ + + + +# UI Errors Documentation + +## Table of Contents + +- [Page should contain body class](#page-should-contain-body-class) +- [Page should not have PHP errors](#page-should-not-have-php-errors) +- [Page should return 200 status](#page-should-return-200-status) +- [Browser console should not contain errors](#browser-console-should-not-contain-errors) +- [Page should have complete output](#page-should-have-complete-output) +- [Page should not have unexpected links](#page-should-not-have-unexpected-links) +- [Should have skip links](#should-have-skip-links) +- [Should have appropriate submenus](#should-have-appropriate-submenus) +- [Should have element focus state](#should-have-element-focus-state) +- [Should have logical tabbing](#should-have-logical-tabbing) + +## Page should contain body class + +This test expects the page's body class to be present on the page. + +[WordPress.org rule definition.](https://make.wordpress.org/themes/handbook/review/required/#templates) + +### Troubleshooting + +Make sure all your relevant templates include something like the following: + +``` +> +``` + +[Read more](https://developer.wordpress.org/reference/functions/body_class/) + +## Page should not have PHP errors + +This test expects that the plugin does not output any PHP errors. + +[WordPress.org rule definition.](https://make.wordpress.org/themes/handbook/review/required/#code) + +### Troubleshooting + +Verify that the page does not display any PHP errors. + +## Page should return 200 status + +This test expects that pages return a `200` http status code when visiting in the browser. + +### Troubleshooting + +Make sure your plugin does not have any unnecessary redirects. + +## Browser console should not contain errors + +This test expects that the plugin doesn't not output anything to the browser console. + +[WordPress.org rule definition.](https://make.wordpress.org/themes/handbook/review/required/#code) + +### Troubleshooting + +Open your Browser Developer tools: +- Verify that all assets (images, JavaScript, css, etc..) are loaded properly. +- Verify that JavaScript errors are not present + +## Page should have complete output + +This test expects that pages have proper HTML code. + +### Troubleshooting + +Verify that pages have both opening and closing tags. Focus on the following elements: + +- `` +- `` +- `` +- `` - If necessary + +You can also use the [Markup Validation Service](https://validator.w3.org/) to help. + +## Page should not have unexpected links + +This test expects to **not** find links that are not approved. + +### Troubleshooting + +Verify that the theme only includes links in the [hosts List](https://github.com/WordPress/theme-review-action/blob/f97655ebfbd5602686b62491dda36f0de4a60bd7/actions/ui-check/tests/e2e/specs/page/index.test.js#L114). + +## Should have skip links + +This test expects that pages include skip links for accessibility. + +[WordPress.org rule definition.](https://make.wordpress.org/themes/handbook/review/required/#skip-links) + +### Troubleshooting + +Verify that on pages that include content, the first time user types the `Tab` key, a skip link is present. + +[Read more](https://make.wordpress.org/themes/handbook/review/accessibility/required/#skip-links) + +## Should have appropriate submenus + +This test expects that navigational menus are useable using a keyboard. + +[WordPress.org rule definition.](https://make.wordpress.org/themes/handbook/review/accessibility/required/#keyboard-navigation) + +### Troubleshooting + +Verify that you can use the `Tab` keyboard button to access and enter all items in the page's menu. + +## Should have element focus state + +This test expects that all focusable element have a visible focus state. + +### Troubleshooting + +- `Tab` through the tab and verify that all focusable elements have a visible difference when in focus. +- Hover over focusable elements and verify they have visible difference. + +[Read more](https://make.wordpress.org/themes/handbook/review/accessibility/required/#contrasts) + +## Should have logical tabbing + +This test expects tabbing to move logically through the page. + +### Troubleshooting + +- Tab through the page and verify that focus moves through the page visibly and as expected. \ No newline at end of file