-
Notifications
You must be signed in to change notification settings - Fork 5
Parse output from composer and list missing extensions #19
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
Conversation
Current coverage is 99.04% (diff: 96.52%)@@ master #19 diff @@
==========================================
Files 25 26 +1
Lines 649 735 +86
Methods 81 102 +21
Messages 0 0
Branches 0 0
==========================================
+ Hits 646 728 +82
- Misses 3 7 +4
Partials 0 0
|
Yo @mikeymike can you review please? |
@@ -14,17 +14,23 @@ | |||
*/ | |||
class ComposerInstallerFactoryTest extends PHPUnit_Framework_TestCase | |||
{ | |||
public function testCreate() | |||
public function testTrue() |
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.
mistake or coming back to it later?
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.
Yeah should be removed :D
$this->assertFileExists(sprintf('%s/vendor', $this->tempDir)); | ||
$this->assertFileExists(sprintf('%s/composer.lock', $this->tempDir)); | ||
|
||
$expectedOutput = "/Loading composer repositories with package information\n"; |
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.
Just a thought... are we not making this a fragile test by testing the output of another package? I see why it's done I'm just wondering if it's the best thing to do.
Unfortunately I don't really have alternative other than checking that there was output which I guess in it self is a pretty weak/fragile test
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.
it has no requires and it's not a real package - so it should be fine imo
@@ -14,40 +14,45 @@ | |||
*/ | |||
class IOFactoryTest extends PHPUnit_Framework_TestCase |
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.
safe to remove this test right?
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.
yep!
@AydinHassan nice stuff dude... I like the approach with the IO much cleaner 👍 |
*/ | ||
public function __construct($exitCode, $output) | ||
{ | ||
$this->exitCode = $exitCode; |
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.
nice find @scrutinizer-auto-fixer !
a947866
to
1ddddf2
Compare
TODO:
Fixes #18
Features:
composer.json
so this can only be global installed if required ext's are there.