-
Notifications
You must be signed in to change notification settings - Fork 176
test(unit): add comprehensive unit test suites for core library classes & lib #1696
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.
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
base: master
Are you sure you want to change the base?
Conversation
2db6857
to
856f038
Compare
6ec3c46
to
ac583ca
Compare
tests/phpunit/lib/searchable.php
Outdated
|
||
public function setUp(): void { | ||
if (!defined('APP_PATH')) { | ||
require_once __DIR__.'/../bootstrap.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proper bootstrap means we don't call these inside the unit tests themselves. Why would the app path not be defined? Tests should be executed by a single phpunit command which should always properly bootstrap the framework.
tests/phpunit/lib/searchable.php
Outdated
} | ||
// Load mocks first | ||
if (!class_exists('Hm_Mock_Session', false)) { | ||
require_once APP_PATH.'tests/phpunit/mocks.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the correct place to include such files. Do it in the bootstrap system.
tests/phpunit/mocks.php
Outdated
} | ||
} | ||
|
||
trait Mock_Searchable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the classes or traits called mocks are actual mocks - you copy/implement details from the actual code in the test suite - this is counterproductive as code will diverge over time. See the documentation on how to mock objects https://docs.phpunit.de/en/9.6/test-doubles.html#mock-objects - you add some expectations to methods that should be called and what should be returned but you actually work with the real objects and not some classes that resemble the real classes partially. In theory, it is best to follow this pattern:
- use the object itself whenever possible
- if testing becomes complicated as the original class depends on 3rd party external connections, you can mock the result of some of its methods and return what you want to test
- note that merely testing the return of data that you supplied as a mock is not a productive test. The test objective is to test code that runs in the framework and not code that is written in the tests themselves. So, whenever you want to test a method, see what external connections it uses, mock them and actually call that method on that original class/object and test that it works correctly. There is a subtle but important difference here. The way you did it with these mocks is that you test the mocks themselves which doesn't make sense.
Also, you shouldn't use reflection to set private properties or do similar changes that were not designed to be done with the class in question. If you want to test something that's not available for some reason, do:
- think if it actually needs to be tested. Private methods usually are not tested. Testing the public interface of the class is enough to ensure it works fine as it calls private methods internally, so they become part of the tests.
- if something private really needs to be tested, refactor the original class/interface and make it public or separate it to parts that are testable
Refactoring original code because of testing is a legitimate use-case, so don't be afraid to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kroky I’ve applied all the requested corrections and fixed the other failing tests.
For Hm_Test_Mailbox, I added some imports in setUp to avoid conflicts. These are only required there and not elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, in hm-jmap.php I added a few missing compatibility functions (is_supported, get_message_sort_order, and sort_by_fetch) with TODOs and comments. These provide placeholders so we can later implement proper JMAP behavior and confirm everything works correctly with JMAP as well.
ac583ca
to
d4516b0
Compare
4415838
to
8970896
Compare
…arch content tests
da74152
to
0a4b43f
Compare
0a4b43f
to
896b622
Compare
New Test Suites
tests/phpunit/modules/core/mailbox.php – Tests for Hm_Mailbox
tests/phpunit/lib/searchable.php – Tests for Searchable trait
tests/phpunit/lib/scram_authenticator.php – SCRAM authentication
tests/phpunit/lib/ini_set.php – PHP configuration validation