WordPress / theme-review-action

Other
31 stars 10 forks source link

Improve messaging #56

Open StevenDufresne opened 3 years ago

StevenDufresne commented 3 years ago

This PR moves most of the logging logic to a custom jest reporter which will allow us to string together reporters and manage the output better. This also groups the errors by test to reduce the redundancy of the messaging. See #27.

Example Output

Test Name: Page should contain body class

Found on: 
"/?p=1241","/","/?cat=1","/?tag=alignment-2","/?post_format=gallery"

Details: 
"/?p=1241" (via: singular.php) does not contain a body class
"/" (via: index.php) does not contain a body class
"/?cat=1" (via: index.php) does not contain a body class
"/?tag=alignment-2" (via: index.php) does not contain a body class
"/?post_format=gallery" (via: index.php) does not contain a body class

Help: 
https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-contain-body-class

Test Name: Should have appropriate submenus

Found on: 
"/"

Details: 
Submenus should become visible when :focus is added to the link through the main navigation.

Help: 
https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-warnings.md#should-have-appropriate-submenus
StevenDufresne commented 3 years ago

Thoughts on this?

@kafleg @carolinan

carolinan commented 3 years ago

I like that it no longer says test theme because that was confusing.

I found a report that was empty: https://themes.trac.wordpress.org/ticket/103795#comment:1

"Found on" is confusing to me because the result has not been presented yet. I don't think it is clear to everyone that these are links or pages on a WordPress installation using the theme test data. Because of that, "/?tag=alignment-2" does not provide useful information on its own.


Front-end tests

Tests performed on the active theme with the theme test data installed.

Test Name: Page should contain body class

Result

Problems were found on the following links:

"example.com/?p=1241" (via: singular.php) does not contain a body class "example.com/" (via: index.php) does not contain a body class "example.com/?cat=1" (via: index.php) does not contain a body class "example.com/?tag=alignment-2" (via: index.php) does not contain a body class "example.com/?post_format=gallery" (via: index.php) does not contain a body class

Help: https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-contain-body-class

carolinan commented 3 years ago

Do you think the links would be more clear if the permalink setting was updated to post name?

carolinan commented 3 years ago

We need help from UX experts who knows the best way to deliver negative messages. 🤔

StevenDufresne commented 3 years ago

I found a report that was empty:

That was a mistake, I deleted that comment.

"Found on" is confusing to me because the result has not been presented yet.

I think you're correct. I think we can maybe remove the Found on section and the URL from the details.

What about: "We couldn't find a body class when loading a page using singular.php" "We couldn't find a body class when loading a page using index.php"

I think talking in terms of template files should make sense for authors?

carolinan commented 3 years ago

Yes I think using template files is easier. Also because it is more similar to how the older tools like theme check and the theme sniffer works.

carolinan commented 3 years ago

"We" is problematic when it is not known who "we" are, But using passive voice should also be avoided. I want to avoid it sounding like "The themes team couldn't find..."/ "The reviewer couldn't find..." Since these are automated messages.

StevenDufresne commented 3 years ago

Alright, I made some updates to provide the template instead of the URL when possible. When we don't have access to the template file name we just use the URL. I also added some logic to remove duplicates although it only removes exact duplicates so there are more improvements that we can make in the future. I also think there are endless improvements to messaging (stripping out test information, wording, linking to helpful docs, etc...) but we can work on those as we go.

@carolinan @kafleg Any objections to merging this PR?

Test Name: Page should not have PHP errors

Details: 
Loading the page using single.php contains PHP errors: Notice: Undefined variable: something in wp-content/themes/test-theme/header.php on line 14
Loading the page using "/" contains PHP errors: Notice: Undefined variable: something in wp-content/themes/test-theme/header.php on line 14
Loading the page using archive.php contains PHP errors: Notice: Undefined variable: something in wp-content/themes/test-theme/header.php on line 14

Help: 
https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-not-have-php-errors

Test Name: Browser console should not contain errors

Details: 
Loading the page using single.php contains javascript errors. Found Error: ReferenceError: sdfasdf is not defined  at http://localhost:8889/?p=1241:178:23
Loading the page using "/" contains javascript errors. Found Error: ReferenceError: sdfasdf is not defined  at http://localhost:8889/:174:23
Loading the page using archive.php contains javascript errors. Found Error: ReferenceError: sdfasdf is not defined  at http://localhost:8889/?cat=1:175:23
Loading the page using archive.php contains javascript errors. Found Error: ReferenceError: sdfasdf is not defined  at http://localhost:8889/?tag=alignment-2:175:23
Loading the page using archive.php contains javascript errors. Found Error: ReferenceError: sdfasdf is not defined  at http://localhost:8889/?post_format=gallery:175:23

Help: 
https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#browser-console-should-not-contain-errors

Test Name: Page should not have unexpected links

Details: 
"unexpectedlin.com" found when viewing single.php is not an approved link.
"unexpectedlin.com" found when viewing "/" is not an approved link.
"unexpectedlin.com" found when viewing archive.php is not an approved link.

Help: 
https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-not-have-unexpected-links

Test Name: Page should have complete output

Details: 
Loading a page using "/" contains incomplete output. Make sure the page contains valid html.

Help: 
https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-have-complete-output
carolinan commented 3 years ago

The use of "test-theme" is confusing. Can the visual presentation of the name be replaced with the theme slug? Notice: Undefined variable: something in wp-content/themes/test-theme/header.php on line 14