Experiment with better error messaging. (#19)

WordPress · Mar 16, 2021 · 0fedb20 · 0fedb20
1 parent f97655e
commit 0fedb20
Show file tree

Hide file tree

Showing 7 changed files with 183 additions and 52 deletions.
diff --git 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
@@ -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
@@ -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
@@ -3,3 +3,4 @@ export * from './formatting';
 export * from './messaging';
 export * from './queries';
 export * from './screenshots';
+export * from './settings';
diff --git 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,15 +60,15 @@ 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 );
 };
 
 /**
  * Outputs messages as warning
  * @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
@@ -0,0 +1,2 @@
+export const ERROR_DOCS_URL =
+	'https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md';
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		export const ERROR_DOCS_URL =
		'https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md';