-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add git packages as VCS repositories #79
base: master
Are you sure you want to change the base?
Conversation
Sorry about that. I'll take a look at the failing test case. I'll probably have it fixed later this weekend. |
Allow package-skeleton to be cached. Only test download of skeleton once.
Okay, looks like the tests are green again. Regarding style rules, I've tried a few different settings and plugins for PhpStorm, but nothing seems to match the StyleCI laravel preset. Do you have any tips you want to share? |
Perhaps you can also use the skeleton for the tests, or perhaps a proper composer.json is required? Regarding styleci, I use their standard Laravel preset as far as I know: https://styleci.readme.io/docs/presets |
Reuse package created by first test. Ensure composer commands are run without Xdebug enabled.
Yeah, I've just changed the So... quite a lot of changes. I've tried my best to make the tests run faster, but I can't speed up the I did manage to save some time, by not repeating the entire The "cached skeleton" feature is not used in the tests any longer, since the other way is much faster. So you're welcome to remove the feature if you like, but I figure maybe some users would want that option. I've also refactored a lot of code, so most of my changes have ended up in traits. The intention was to make it less messy, but I'm open to your input if you want it structured differently. Besides that, the biggest change is probably in the way I test whether packages packages are installed/removed correctly. I trust the tests a lot more now. And I've added a few extra assertions where I thought they made sense. Anyhoo, let me know what you think. |
At this point, the speed of the tests don't really bother me. The fact that you trust the tests more now is much more appealing to me! |
src/ComposerHandler.php
Outdated
|
||
/** | ||
* Determines the path to Composer executable | ||
* @todo Might not work on Windows |
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 have a Windows machine, so I will test this.
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.
Cool. I've removed the @todo
} | ||
|
||
/** | ||
* @depends test_new_package_is_created |
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 didn't know about @depends
, looks very clever.
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 normally like writing tests that rely on other tests, but sometimes it's super convenient.
@@ -7,6 +7,7 @@ | |||
* Default: http://github.com/Jeroen-G/packager-skeleton/archive/master.zip | |||
*/ | |||
'skeleton' => 'http://github.com/Jeroen-G/packager-skeleton/archive/master.zip', | |||
'cache_skeleton' => false, |
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.
What would be a proper place to document this? I'm thinking a line in just the config file might be enough?
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.
Now I wonder if it's even worth keeping. The only benefit is that you save about a second of download time (if even that). On the other hands, it adds a layer of complexity, and people might end up using an outdated skeleton without knowing it.
src/ComposerHandler.php
Outdated
* @todo Might not work on Windows | ||
* @return string | ||
*/ | ||
protected static function getComposerExecutable(): string |
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 did you choose to make this (and the ones below) static?
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.
Hmm. Mostly just for convenience and old habits.
The whole initial setup of the Testbench environment runs in a static context (setupBeforeClass
), before Laravel has even been initialized. This is necessary because I can't switch the app's base_path
once it's already up and running.
But honestly I should change it back to non-static, since I can just call the methods from the $instance
variable. I just didn't notice earlier :)
|
||
namespace JeroenG\Packager; | ||
|
||
trait ProcessRunner |
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 refactor 👍
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.
Glad you like it. I considered calling the other one ComposerCommander
or something like that, but decided to keep it boring...
WTF. One of the tests fail now, but only in PHP 7.2. Edit: Super weird. All tests are green when i run them on 7.2. But Travis is unhappy. |
Have you tried installing a package with tests? So far I can't get those tests to work :/ |
Default timeout is 60 seconds, which is usually enough. Sometimes running composer require takes longer, especially when resolving a package version, which exists in multiple repositories
The failing unit test was caused by a simple timeout, which is fixed now.
Nope, I hadn't tried that until now, and I've been pulling my hair for a couple of hours without finding a good solution. I tried using a composer merge-plugin which merges multiple composer.json files, but that only works if all the installed packages are compatible and don't have conflicting dependencies. The way I see it, there's only one way to run package tests right now:
I tried writing a new Command to automate that workflow, but for some reason it won't work. I'm afraid I'm all out of ideas here. |
I've changed the way
packager:git
works.In stead of cloning the repository directly to the
/packages/myVendor/myPackage
folder, it is added as a VCS repository incomposer.json
and installed like any other package to the/vendor
folder. After installing, it is symlinked to the/packages
folder.If the package origin is only specified as
vendor/package
in stead of a complete URL, it is assumed that the repository is located athttps://github.com/vendor/package
.The
packager:list
command now has an extra column showing the Git status of the package. If packages are version controlled, it will whether they'reUp to date
,Ahead {number of commits}
orBehind {number of commits}
. If not, they're displayed asNot initialized
.I've add a couple of tests to ensure the new features are working correctly. They both clone your
Jeroen-G/testassist
repository, but it doesn't really matter which repository they use, if you want to change it to something else.Let me know if anything needs to be changed.
/Morten