Skip to content

jetpack: Catch calls to exit() and die() in PHPUnit - #21043

Merged
anomiex merged 7 commits into
masterfrom
fix/jetpack-tests-catch-exit
Oct 22, 2021
Merged

anomiex merged 7 commits into
masterfrom
fix/jetpack-tests-catch-exit

Conversation

@anomiex

@anomiex anomiex commented Sep 10, 2021 •

Copy link
Copy Markdown
Contributor

Changes proposed in this Pull Request:

It's too easy to wind up having WordPress just exit(), for example by
having something call wp_die() while wp_doing_ajax() returns true.

Let's use antecedent/patchwork (which we already use as part of
brain/monkey in a bunch of package tests) to redefine these functions to
throw an exception instead, which PHPUnit will catch.

Jetpack product discussion

p9dueE-3sh-p2

Does this pull request change what data or activity we track or use?

No

Testing instructions:

  • Is CI happy?
  • Add an exit() call somewhere in code covered by a test. Does PHPUnit still run to completion, with the relevant test showing that an unexpected ExitException was thrown?

It's too easy to wind up having WordPress just `exit()`, for example by
having something call `wp_die()` while `wp_doing_ajax()` returns true.

Let's use antecedent/patchwork (which we already use as part of
brain/monkey in a bunch of package tests) to redefine these functions to
throw an exception instead, which PHPUnit will catch.
@anomiex anomiex added [Status] Needs Review This PR is ready for review. [Pri] Normal labels Sep 10, 2021
@anomiex
anomiex requested a review from brbrr September 10, 2021 17:39
@anomiex anomiex self-assigned this Sep 10, 2021
@github-actions github-actions Bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Sep 10, 2021
@github-actions

github-actions Bot commented Sep 10, 2021 •

Copy link
Copy Markdown
Contributor

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: November 2, 2021.
  • Scheduled code freeze: October 25, 2021.

@anomiex anomiex added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Sep 10, 2021
@brbrr

brbrr commented Sep 10, 2021 •

Copy link
Copy Markdown
Contributor

I like the approach. Although, I'm not sure how it actually works. There is an unrelated(?) failure in CI, and then there is a fatal ExitException thrown. I'm not sure if these are two separate errors, or somehow related. Also, fatal's stack trace is not useful at all, but it's probably better than nothing 😬

https://github.com/Automattic/jetpack/pull/21043/checks?check_run_id=3569848661#step:9:210

@anomiex

anomiex commented Sep 10, 2021

Copy link
Copy Markdown
Contributor Author

So unfortunately it's not self-testing after all because GH merges with master and master already has the fix.

The ExitException that was thrown there is a bug, which should be fixed by 3dc3d01. I didn't notice it locally because the Docker env uses a phar and a wrapper that loads all the PHPUnit classes before calling the bootstrap, while CI installs via composer global which loads the PHPUnit classes as needed via an autoloader.

Not that I blame them, PHP doesn't document the behavior or provide a
way to do it properly.
@matticbot

Copy link
Copy Markdown
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello anomiex! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D66707-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@anomiex

anomiex commented Sep 10, 2021

Copy link
Copy Markdown
Contributor Author

The PHP 8 failure is a Patchwork bug, antecedent/patchwork#117.

@anomiex anomiex added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Oct 22, 2021
@anomiex

anomiex commented Oct 22, 2021

Copy link
Copy Markdown
Contributor Author

Finally there's a release of patchwork to fix the bug, so this can be ready to review again.

@jeherve jeherve added this to the jetpack/10.3 milestone Oct 22, 2021
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 22, 2021
@anomiex
anomiex merged commit f8baeb7 into master Oct 22, 2021
@anomiex
anomiex deleted the fix/jetpack-tests-catch-exit branch October 22, 2021 17:11
@github-actions github-actions Bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 22, 2021
@github-actions

Copy link
Copy Markdown
Contributor

Great news! One last step: head over to your WordPress.com diff, D66707-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@anomiex

anomiex commented Oct 22, 2021

Copy link
Copy Markdown
Contributor Author

r233809-wpcom

davidlonjon added a commit that referenced this pull request Oct 26, 2021
* master: (27 commits)
  Admin Page: Update upgrade buttons Tracks events (#21483)
  Debug Helper IDC Simulator: Add the ability to spoof the home url (#21516)
  Map block: Add `address` block attribute (#21412)
  Update wordpress monorepo (#21522)
  Stats Page: Add VideoPress nudge (#21513)
  Improve Jetpack products illustrations file size (#21514)
  Update dependency mediawiki/mediawiki-codesniffer to v38 (#21523)
  Update dependency yoast/phpunit-polyfills to v1.0.2 (#21524)
  Widgets: add Strava to Social Icons Widget and Social Menu (#21518)
  Boost: Add active modules to speed score request (#21471)
  Markdown: Better accessibility for Footnotes (#21495)
  Jetpack Plugin: Add VideoPress upgrade page (#21497)
  Admin Menu: moves Add new (plugin) menu item to the top for Atomic sites (#21506)
  General: Check the return value of get_comment() (#21080)
  Instagram Widget: link to user connection if user is not connected (#21512)
  Search: Create a new search package, rename Search plugin (#21502)
  Identity Crisis: Remove the unused jetpack_idc_option transient (#21492)
  Stats: do not update the wpcom blog details (#21441)
  Admin: do not display upgrade messages when on offline mode (#21444)
  jetpack: Catch calls to `exit()` and `die()` in PHPUnit (#21043)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] Normal Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants