-
Notifications
You must be signed in to change notification settings - Fork 87
It's time to add some test for the installer itself #48
It's time to add some test for the installer itself #48
Conversation
It's better to require all packages once for testing in stead of running composer for x times for all possible options.
@@ -42,27 +42,27 @@ class OptionalPackages | |||
/** | |||
* @var array | |||
*/ | |||
private static $config; | |||
public static $config; |
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.
Why public?
If it's for testing, the various assertAttribute*()
assertions can do assertions on non-public properties. That said, I've not tried on static properties; if those present an issue, I'd argue reflection is still a better way to test these.
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.
Those are not being tested. They are public so I can set them from within a test. Doing it this way I don't need to repeat the code to setup the OptionalPackages class.
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.
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.
Couldn't these assignments be done with the Reflection API instead? That way we protect encapsulation for normal consumption.
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'm not sure how this would work with static classes.
…-tests It's time to add some test for the installer itself
It's time to add some test for the installer itself
It tests: