Account Protection: Add tests for newly added code #41463
Conversation
…hing to screwed up with the base branch.
ArSn
changed the title
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
github-actions
bot
added
the
[Status] Needs Author Reply
| * | ||
| * @return never | ||
| */ | ||
| protected function redirect_and_exit( string $redirect_location ) { |
There was a problem hiding this comment.
I added these functions because if you exit; in your code, you'll have a hard time running PHPUnit tests against it. This way, it can be disabled dynamically by the tests.
| * @return \WP_User|null The user object. | ||
| */ | ||
| protected function load_user( int $user_id ) { | ||
| return get_user_by( 'ID', $user_id ); |
There was a problem hiding this comment.
I tried hard to find a way to solve this with filters or something, but was ultimately unable to do so.
For example, I tried using wp_cache_set so that this function could just read the user from the cache under the hood, but that didn't work with WordDBless.
I even added Brain\Monkey as a dev requirement, however, then found that it does not seem to go well together with WorDBless, as that is loaded first and then the function already exists, so brain monkey can not redefine it.
Happy to hear suggestions for better solutions.
ArSn
added
[Status] Needs Team Review
There was a problem hiding this comment.
All 45 tests pass running cd projects/packages/account-protection && composer phpunit.
I agree with all the proposed adjustments to class methods and all functionality appears to remain intact.
Very nicely done - a fantastic addition that we can use as a reference and build upon as we go. I'll make a not to consider adding E2Es once the remainder of the features are in place.
github-actions
bot
removed
[Status] Needs Author Reply
* Created new branch with cherrypicked changes from tests because something to screwed up with the base branch. * Added tests for email service. * Added tests for account protection module class. * Added tests for password detection class. * Added changelog entry. * Fix PHP 8 consistency issue in test. * Fixed phan issues.
Proposed changes:
After this PR, test coverage looks as follows:
Which means we still have room for improvement. It has to be said though, that some of the methods are for decoupling dependencies, like exiting the script, or doing a static call to the Jetpack Client for the connection API call. It is sad that it is a static method call, but nothing we can easily change in this PR.
Other information:
Jetpack product discussion
https://github.com/Automattic/jetpack-scan-team/issues/1669
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
As an aside, I think this whole thing might also warrant some E2E tests, but these need to go into the jetpack and protect plugins, and are not part of this PR.