Testing: Fail E2E when page displays warning notices #13452

aduth · 2019-01-23T22:10:59Z

Extracted from: #13446

This pull request seeks to add new functionality to the end-to-end tests to fail immediately fail the tests if a page loads with warning notices present.

In Progress: Currently, the build fails, likely due to a race condition between page navigations and the page.evaluate result. See #13446 (comment) .

Testing instructions:

Ensure end-to-end tests pass:

npm run test-e2e

aduth · 2019-01-28T16:13:39Z

Idea: Navigation needs to be abstracted, where the validation occurs as a pre-condition for the navigation promise being resolved. For example, this could be incorporated into visitAdmin.

gziolo · 2019-03-11T12:17:28Z

I opened #14371 where I want to provide an option to override site constants through env variables.

aduth · 2019-03-22T21:02:59Z

I rebased to account for #14371, also introducing a revised approach which (per #13452 (comment)) checks for errors in the markup as part of visitAdminPage. This should also improve on the previous implementation which would only have caught notices which were output before the start of the page markup, when errors could in-fact occur anywhere in markup. It does so testing for the specific pattern of PHP error markup.

I haven't actually tested this yet, but pushing up what I have as I wrap up my day. It seems it should work fine; I'll test on Monday.

gziolo

It still needs npm run docs:build :)

gziolo · 2019-03-25T09:37:00Z

packages/e2e-test-utils/src/has-php-error.js

+ *
+ * @type RegExp
+ */
+const REGEXP_PHP_ERROR = /<b>(Fatal error|Recoverable fatal error|Warning|Parse error|Notice|Strict Standards|Deprecated|Unknown error)<\/b>/;


Should this match be more detailed? Notice or Warning might be too common and cause false positives. We could at least add : after closing tag:
https://github.com/php/php-src/blob/598175e/main/main.c#L1321

Yeah, I suppose it risks false positives in its current form. With the placeholder formatting and multiple variants, it's harder to get a good pattern to cover all the error outputs. I agree the colon : and even the subsequent double-space could be a good improvement to eliminate those false positives. I'll make the change.

Thank you, I will give it a spin as code wise it look good 👍

gziolo · 2019-03-27T11:01:02Z

I don't know why but this is how notices and warnings are printed in my local copy of Docker for Gutenberg:

Notice: Undefined property: WP_Block_Type_Registry::$x in /var/www/html/wp-content/plugins/gutenberg/lib/blocks.php on line 47

Warning: Cannot modify header information - headers already sent by (output started at /var/www/html/wp-content/plugins/gutenberg/lib/blocks.php:47) in /var/www/html/wp-admin/includes/misc.php on line 1198
<!DOCTYPE html>
 ```

aduth · 2019-03-27T19:52:02Z

@gziolo Hmm, by the code of main.c, there's a few variations of how the errors are displayed, based on the result of html_errors (some PHP configuration?).

https://github.com/php/php-src/blob/598175e/main/main.c#L1331
https://github.com/php/php-src/blob/598175e/main/main.c#L1336
https://github.com/php/php-src/blob/598175e/main/main.c#L1306

I suppose one option is to make the  matching optional, though the downside to this is re-opening the possibility of false positives.

Another option which comes to mind is to direct the PHP errors to write to a log file, and consider a build as successful only if the log file doesn't exist / is empty. Not sure off-hand how easy this would be to implement.

aduth · 2019-03-27T19:55:07Z

html_errors is a runtime configuration:

https://www.php.net/manual/en/errorfunc.configuration.php#ini.html-errors

Perhaps we could normalize this behavior to ensure HTML output. A matter of calling ini_set( "html_errors", 1 ); ?

gziolo · 2019-03-28T12:09:07Z

Another option which comes to mind is to direct the PHP errors to write to a log file, and consider a build as successful only if the log file doesn't exist / is empty. Not sure off-hand how easy this would be to implement.

Given that you can use any website to run e2e tests, I don't think that using log file would work as you might not have access to read from the filesystem in the place where such log file is stored.

This makes me wonder whether we should put it behind the flag and optimize against Travis and don't care too much about all other use cases?

youknowriad · 2020-03-19T08:26:25Z

Should we just land this? it seems still relevant?

aduth · 2020-03-19T21:16:28Z

It is relevant, and still useful. Based on earlier conversation, I think there were some challenges around particular configuration setups. We could maybe ship a semi-functional implementation and improve it later. If I recall correctly, it was at least capturing issues in Travis.

youknowriad · 2020-03-20T13:02:18Z

This works for me, I'd be curious to know if the tests still pass after a rebase and we can ship

aduth · 2020-04-03T21:47:32Z

Rebased to bring this back up to date.

A few changes:

I renamed hasPHPError to hasPageError. There's no reason for it to be so specific to PHP. Page-level errors could come from a multitude of sources. The implementation itself didn't change, and it's fine to target those PHP errors internally.
Expanded regular expression to match both plain-text and HTML error outputs.
Added unit tests.

github-actions · 2020-04-03T21:53:21Z

Size Change: 0 B

Total Size: 889 kB

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.02 kB	0 B
`build/annotations/index.js`	3.4 kB	0 B
`build/api-fetch/index.js`	3.79 kB	0 B
`build/autop/index.js`	2.58 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	6.03 kB	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	760 B	0 B
`build/block-editor/index.js`	102 kB	0 B
`build/block-editor/style-rtl.css`	10.2 kB	0 B
`build/block-editor/style.css`	10.2 kB	0 B
`build/block-library/editor-rtl.css`	7.22 kB	0 B
`build/block-library/editor.css`	7.22 kB	0 B
`build/block-library/index.js`	110 kB	0 B
`build/block-library/style-rtl.css`	7.54 kB	0 B
`build/block-library/style.css`	7.55 kB	0 B
`build/block-library/theme-rtl.css`	669 B	0 B
`build/block-library/theme.css`	671 B	0 B
`build/block-serialization-default-parser/index.js`	1.65 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/blocks/index.js`	57.5 kB	0 B
`build/components/index.js`	195 kB	0 B
`build/components/style-rtl.css`	16.6 kB	0 B
`build/components/style.css`	16.5 kB	0 B
`build/compose/index.js`	6.21 kB	0 B
`build/core-data/index.js`	10.7 kB	0 B
`build/data-controls/index.js`	1.04 kB	0 B
`build/data/index.js`	8.23 kB	0 B
`build/date/index.js`	5.36 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/dom/index.js`	3.05 kB	0 B
`build/edit-navigation/index.js`	2.75 kB	0 B
`build/edit-navigation/style-rtl.css`	279 B	0 B
`build/edit-navigation/style.css`	280 B	0 B
`build/edit-post/index.js`	92.9 kB	0 B
`build/edit-post/style-rtl.css`	12.3 kB	0 B
`build/edit-post/style.css`	12.3 kB	0 B
`build/edit-site/index.js`	10.1 kB	0 B
`build/edit-site/style-rtl.css`	5.02 kB	0 B
`build/edit-site/style.css`	5.02 kB	0 B
`build/edit-widgets/index.js`	7.17 kB	0 B
`build/edit-widgets/style-rtl.css`	3.74 kB	0 B
`build/edit-widgets/style.css`	3.73 kB	0 B
`build/editor/editor-styles-rtl.css`	399 B	0 B
`build/editor/editor-styles.css`	400 B	0 B
`build/editor/index.js`	42.8 kB	0 B
`build/editor/style-rtl.css`	3.49 kB	0 B
`build/editor/style.css`	3.49 kB	0 B
`build/element/index.js`	4.44 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	6.95 kB	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	1.93 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.57 kB	0 B
`build/is-shallow-equal/index.js`	710 B	0 B
`build/keyboard-shortcuts/index.js`	2.3 kB	0 B
`build/keycodes/index.js`	1.7 kB	0 B
`build/list-reusable-blocks/index.js`	2.99 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/media-utils/index.js`	4.84 kB	0 B
`build/notices/index.js`	1.57 kB	0 B
`build/nux/index.js`	3.01 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.54 kB	0 B
`build/primitives/index.js`	1.5 kB	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/rich-text/index.js`	14.5 kB	0 B
`build/server-side-render/index.js`	2.54 kB	0 B
`build/shortcode/index.js`	1.69 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/url/index.js`	4.01 kB	0 B
`build/viewport/index.js`	1.6 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

youknowriad

LGTM 👍

aduth · 2020-04-06T23:25:56Z

I ran through a final round of testing to see how it worked. It did work correctly, but the error output isn't especially helpful:

  ● new editor state › should be immediately saveable

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      at _callee$ (../e2e-test-utils/build/@wordpress/e2e-test-utils/src/visit-admin-page.js:20:8)
      at tryCatch (../../node_modules/regenerator-runtime/runtime.js:45:40)
      at Generator.invoke [as _invoke] (../../node_modules/regenerator-runtime/runtime.js:271:22)
      at Generator.prototype.<computed> [as next] (../../node_modules/regenerator-runtime/runtime.js:97:21)
      at asyncGeneratorStep (../../node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
      at _next (../../node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9)

It's also not entirely clear how to customize this messaging from within Jest (see jestjs/jest#3293). I think it can probably be done by either throwing a custom error, or by using Node's internal assert module. Thinking about this had me contemplating that it would probably be helpful to include the notice itself as part of the messaging output. I think it could be relatively straight-forward to change hasPageError to getPageError, where with the presence of a non-empty value we can output the message as part of the failure message.

I'll plan to put this together in the morning.

Return error message so it can be used in failure output

aduth · 2020-04-07T21:26:02Z

Thinking about this had me contemplating that it would probably be helpful to include the notice itself as part of the messaging output. I think it could be relatively straight-forward to change hasPageError to getPageError, where with the presence of a non-empty value we can output the message as part of the failure message.

Updated in 965900a and 228aad3.

New messaging example:

  ● new editor state › should be immediately saveable

    Unexpected error in page content: Notice: Undefined offset: 1 in /var/www/html/wp-content/plugins/gutenberg/gutenberg.php on line 164

      at _callee$ (../e2e-test-utils/build/@wordpress/e2e-test-utils/src/visit-admin-page.js:30:9)
      at tryCatch (../../node_modules/regenerator-runtime/runtime.js:45:40)
      at Generator.invoke [as _invoke] (../../node_modules/regenerator-runtime/runtime.js:271:22)
      at Generator.prototype.<computed> [as next] (../../node_modules/regenerator-runtime/runtime.js:97:21)
      at asyncGeneratorStep (../../node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
      at _next (../../node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9)

aduth added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Jan 23, 2019

aduth mentioned this pull request Jan 23, 2019

Plugin: Resolve warning notice displayed for deprecated function #13446

Merged

gziolo added this to the 5.2 (Gutenberg) milestone Feb 7, 2019

aduth mentioned this pull request Feb 15, 2019

Blocks: Re-register core blocks via build copy prefixing #13521

Merged

gziolo modified the milestones: 5.2 (Gutenberg), 5.3 (Gutenberg) Mar 4, 2019

gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Mar 8, 2019

gziolo requested review from ajitbohra, gziolo, nerrad, ntwb, talldan and youknowriad as code owners March 8, 2019 19:24

gziolo mentioned this pull request Mar 11, 2019

Add an option to configure site constants #14371

Merged

youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

aduth mentioned this pull request Mar 22, 2019

Undefined index: description in [...]/plugins/gutenberg/lib/widgets.php on line 98 #14552

Closed

aduth force-pushed the add/e2e-fail-notices branch from ee92968 to c22e3b7 Compare March 22, 2019 21:01

gziolo reviewed Mar 25, 2019

View reviewed changes

gziolo force-pushed the add/e2e-fail-notices branch from b524517 to 6bdd7aa Compare March 27, 2019 11:08

gziolo assigned aduth Mar 27, 2019

aduth force-pushed the add/e2e-fail-notices branch from 6bdd7aa to 8d9aff1 Compare April 3, 2020 21:47

youknowriad approved these changes Apr 6, 2020

View reviewed changes

aduth added 6 commits April 7, 2020 15:54

Testing: Fail E2E when page displays warning notices

6654219

Testing: Check markup for errors in visitAdminPage

08d43d2

E2E Test Utils: Rename hasPHPError to hasPageError

89165db

E2E Test Utils: Handle both plaintext and HTML errors

0164e95

E2E Test Utils: Refactor hasPageError as getPageError

965900a

Return error message so it can be used in failure output

E2E Test Utils: Display page error message in failure output

228aad3

aduth force-pushed the add/e2e-fail-notices branch from 8d9aff1 to 228aad3 Compare April 7, 2020 19:55

aduth merged commit 28848a4 into master Apr 7, 2020

aduth deleted the add/e2e-fail-notices branch April 7, 2020 21:26

github-actions bot added this to the Gutenberg 7.9 milestone Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing: Fail E2E when page displays warning notices #13452

Testing: Fail E2E when page displays warning notices #13452

aduth commented Jan 23, 2019

aduth commented Jan 28, 2019

gziolo commented Mar 11, 2019 •

edited

Loading

aduth commented Mar 22, 2019 •

edited

Loading

gziolo left a comment

gziolo Mar 25, 2019

aduth Mar 25, 2019

gziolo Mar 26, 2019

gziolo commented Mar 27, 2019 •

edited

Loading

aduth commented Mar 27, 2019

aduth commented Mar 27, 2019

gziolo commented Mar 28, 2019

youknowriad commented Mar 19, 2020

aduth commented Mar 19, 2020 •

edited

Loading

youknowriad commented Mar 20, 2020

aduth commented Apr 3, 2020

github-actions bot commented Apr 3, 2020 •

edited

Loading

youknowriad left a comment

aduth commented Apr 6, 2020

aduth commented Apr 7, 2020

Testing: Fail E2E when page displays warning notices #13452

Testing: Fail E2E when page displays warning notices #13452

Conversation

aduth commented Jan 23, 2019

aduth commented Jan 28, 2019

gziolo commented Mar 11, 2019 • edited Loading

aduth commented Mar 22, 2019 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

gziolo Mar 25, 2019

Choose a reason for hiding this comment

aduth Mar 25, 2019

Choose a reason for hiding this comment

gziolo Mar 26, 2019

Choose a reason for hiding this comment

gziolo commented Mar 27, 2019 • edited Loading

aduth commented Mar 27, 2019

aduth commented Mar 27, 2019

gziolo commented Mar 28, 2019

youknowriad commented Mar 19, 2020

aduth commented Mar 19, 2020 • edited Loading

youknowriad commented Mar 20, 2020

aduth commented Apr 3, 2020

github-actions bot commented Apr 3, 2020 • edited Loading

youknowriad left a comment

Choose a reason for hiding this comment

aduth commented Apr 6, 2020

aduth commented Apr 7, 2020

gziolo commented Mar 11, 2019 •

edited

Loading

aduth commented Mar 22, 2019 •

edited

Loading

gziolo commented Mar 27, 2019 •

edited

Loading

aduth commented Mar 19, 2020 •

edited

Loading

github-actions bot commented Apr 3, 2020 •

edited

Loading