Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Plugin: Resolve warning notice displayed for deprecated function #13446

Merged
merged 1 commit into from
Jan 24, 2019
Merged

Plugin: Resolve warning notice displayed for deprecated function #13446

merged 1 commit into from
Jan 24, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 23, 2019

Regression introduced in: #13408

This pull request seeks to resolve an issue where a warning notice is printed to the page when the site is configured to use the WP_DEBUG constant. Further, it adds a new observePageWarnings to @wordpress/e2e-test-utils, used in the end-to-end test suite to fail tests immediately if there are any warnings present on the page.

Testing instructions:

Verify end-to-end tests pass:

npm run test-e2e

Verify end-to-end tests fail after git revert 5c604f5 in your local copy of the branch:

npm run test-e2e

@aduth aduth added [Type] Bug An existing feature does not function as intended Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts labels Jan 23, 2019
@aduth aduth requested a review from gziolo January 23, 2019 17:17
youknowriad
youknowriad reviewed Jan 23, 2019
View reviewed changes
bin/install-wordpress.sh Outdated
@@ -87,6 +87,10 @@ if [ "$CURRENT_URL" != "http://localhost:$HOST_PORT" ]; then
docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm -u 33 $CLI option update siteurl "http://localhost:$HOST_PORT" --quiet
fi

# Configure site constants.
echo -e $(status_message "Configuring site constants...")
docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm -u 33 $CLI config set WP_DEBUG true --quiet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to run the e2e tests in debug mode?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's risky but we can give it a try and see if it doesn't make the experience harder.

By the way, it would solve the issue @iseulde was trying to tackle earlier. Maybe we could put it behind some flag? It seems to be very useful when developing locally.

Copy link
Member

@gziolo gziolo Jan 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #12457 for reference. Let's make we close the other one if we decide to land this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now see that it's different SCRIPT_DEBUG vs WP_DEBUG. I think I should finish my work for today 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what would be risky about it. I run with WP_DEBUG constantly in my development environment, exactly to be able to catch an issue like this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question was:

Could a debug environment be different from a regular prod environment we should mimic in tests.

I don't have objections though.

@gziolo
Copy link
Member

gziolo commented Jan 23, 2019

There e2e test failures which don't make too much sense to me. They seem to be unrelated.

@aduth
Copy link
Member Author

aduth commented Jan 23, 2019

Trying to debug what's going on with the end-to-end tests, I'm inclined to think there could be some race condition where page.evaluate is not able to complete before some page navigation occurs. Based on the failures, it appears to affect tests which activate plugins during its setup steps.

Interestingly, it seems to work correctly when bumping Puppeteer to the latest version; though I'm not sure what would have changed. It seems like it could still remain prone to race conditions (same applies to any page evaluations we do which aren't "blocking" some other action from taking place).

@aduth aduth mentioned this pull request Jan 23, 2019
Merged
@aduth aduth force-pushed the fix/silence-rest-errors-warning branch from 1787e0b to f909853 Compare January 23, 2019 22:11
@aduth
Copy link
Member Author

aduth commented Jan 23, 2019

In the interest of fixing the bug earlier than later, I've extracted the problematic E2E enhancements to a separate pull request at #13452 .

@aduth aduth force-pushed the fix/silence-rest-errors-warning branch from f909853 to 8733812 Compare January 24, 2019 13:38
@aduth
Copy link
Member Author

aduth commented Jan 24, 2019

I must have messed up the rebase, squashing instead of dropping commits. It's now back to the three line change it's meant to be.

gziolo
gziolo approved these changes Jan 24, 2019
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@gziolo gziolo added this to the 5.0 (Gutenberg) milestone Jan 24, 2019
@aduth aduth merged commit eab7642 into master Jan 24, 2019
@aduth aduth deleted the fix/silence-rest-errors-warning branch January 24, 2019 14:44
daniloercoli added a commit that referenced this pull request Jan 24, 2019
@daniloercoli
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
b5e679b 
…rnmobile/372-add-title-to-gutenberg-mobile

* 'master' of https://github.com/WordPress/gutenberg: (56 commits)
  Save package-lock.json file changes (#13481)
  Plugin: Deprecate gutenberg_add_responsive_body_class (#13461)
  Add speak messages to the feature toggle component. (#13385)
  Plugin: Deprecate gutenberg_kses_allowedtags (#13460)
  Plugin: Deprecate gutenberg_bulk_post_updated_messages (#13472)
  Plugin: Avoid calling deprecated gutenberg_silence_rest_errors (#13446)
  Plugin: Deprecate gutenberg_remove_wpcom_markdown_support (#13473)
  Fix: Categories block: add custom classes only to wrapper (#13439)
  is-shallow-equal: Use ES5 ruleset from eslint-plugin module (#13428)
  Update and Organize Contributors Guide per #12916 (#13352)
  Dismissible-notices: fix text overlapping icon (X) (#13371)
  Framework: Remove 5.0-merged REST API integrations (#13408)
  Plugin: Remove 5.0-merged block registration functions, integrations (#13412)
  Framework: Bump minimum required WP to 5.x (#13370)
  [Mobile] Improve keyboard hide button (#13415)
  Improve castError handling of non strings (#13315)
  Fix: File block add custom class (#13432)
  Consider making Fullscreen Mode effects visible only on larger screens (#13425)
  Update plugin version to 4.9.0 (#13436)
  DateTimePicker: fix prop warning for (#12933)
  ...
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@aduth @youknowriad
Plugin: Avoid calling deprecated gutenberg_silence_rest_errors (#13446)
e8d19c0
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@aduth @youknowriad
Plugin: Avoid calling deprecated gutenberg_silence_rest_errors (#13446)
b8080fa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad left review comments

@gziolo gziolo gziolo approved these changes

Assignees
No one assigned
Labels
Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
5.0 (Gutenberg)
Development

Successfully merging this pull request may close these issues.
3 participants
@aduth @gziolo @youknowriad