-
Notifications
You must be signed in to change notification settings - Fork 7
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
Chore: Add integration test suite #35
base: develop
Are you sure you want to change the base?
Conversation
require_once dirname( __FILE__, 3 ) . '/vendor/autoload.php'; | ||
} | ||
|
||
$this->s3_media_sync = S3_Media_Sync::init(); |
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.
I don't think I'm a fan of this line.
Why? Two reasons.
Firstly, the init()
call is attempting to create the class as a singleton pattern (even though the constructor is public), and while that is often sean as negative for production code, it seems even worse for test code; any changes made in one test may carry through to further tests, rather than keeping everything in isolation.
Secondly, as this is assigned to the public property and presumably called from each test class extending this test case, it's effectively double-globalised, which again could affect isolation.
Let's see what the constructor of the class being tested does:
s3-media-sync/inc/class-s3-media-sync.php
Lines 20 to 22 in 97616c3
public function __construct() { | |
$this->settings = get_option( 's3_media_sync_settings' ); | |
} |
So I look at this and think: "What if there's a test where I need to update the saved options before I instantiate the class (directly, or through the init()
call)? I wouldn't be able to, because it's all nailed down once the test has started due to the structure of the set_up()
(or, at least, one would need to be careful when the parent::set_up()
is called).
I can see how an integration test suite may want to limit the number of get_options()
calls to the database, but maybe that then becomes a code smell that the constructor of the S3_Media_Sync class shouldn't be making calls to the database just to set up a class - a separate PR should look at instantiating the class and dependency-injecting the retrieved options (a Settings data transfer object, for instance, which can be started and injected via a Factory class), which then allows flexibility of running unit tests against this class since the database retrieval can be mocked instead.
I'll leave this up to you to consider for this particular PR, but it's probably worth thinking and capturing notes into an issue even if you can't work on this at the moment.
$this::set_private_property( | ||
$this->s3_media_sync::class, | ||
$this->s3_media_sync, | ||
'instance', | ||
null | ||
); |
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.
If the above changes were made in terms of the simplified setup, this tear_down reset would seem un-needed.
Setting this PR to draft as it needs refactoring independent of any |
Specific |
Description
Introduces a framework for writing integration tests, which also supports unit testing.
For integration testing, a base test case class is provided that contains
set_up
andtear_down
methods that future class tests can use to ensure each test is run with a new instantiation of the mainS3_Media_Sync
plugin class.