-
-
Notifications
You must be signed in to change notification settings - Fork 438
Make the root package name and path dynamically discovered in MakerTestCase
#1770
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: 1.x
Are you sure you want to change the base?
Conversation
src/Test/MakerTestEnvironment.php
Outdated
| unset($composerJson['require-dev']); | ||
|
|
||
| $composerJson['repositories']['symfony/maker-bundle'] = [ | ||
| $composerJson['repositories'][$this->packageName] = [ |
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.
Composer 2.9 is deprecating the usage of object-based structure for repositories (as JSON technically does not guarantee an order for object keys). It might make sense to avoid relying on it, using the array-based structure instead (maybe using the composer repo command)
|
|
||
| namespace Symfony\Bundle\MakerBundle\Test; | ||
|
|
||
| use Composer\InstalledVersions; |
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 should declare a dependency on composer-runtime-api with a constraint matching the version of the runtime API providing the info you rely on (probably 2.0 as lowest bound for that case, unless some of the feature you rely on was added 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.
The install_path was added in Composer 2.1 by composer/composer#9699
MakerTestCaseMakerTestCase
src/Test/MakerTestEnvironment.php
Outdated
|
|
||
| file_put_contents($composerJsonPath, json_encode($composerJson, \JSON_THROW_ON_ERROR | \JSON_PRETTY_PRINT | \JSON_UNESCAPED_SLASHES)); | ||
| MakerTestProcess::create(\sprintf( | ||
| 'composer repo add %s \'%s\' ', |
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 won't work on Windows (where you cannot use single-quote to wrap arguments).
I think the best solution would be to make MakerTestProcess support the array-based signature to define a Process (which is a lot safer than building a command-line that is parsed by a shell as it solves the need for shell escaping)
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 realize that was already possible. I added the param type to the relevant methods (all internal or final).
Use Composer\InstalledVersions::getRootPackage() to retrieve the path and name of the root package under test Locate the fixtures and cache directory relatively to this package
An issue when using the
MakerTestCaseto test maker classes provided by other packages, is that it depends on thetestsdirectory of thesymfony/maker-bundlepackage.In this PR, we use
Composer\InstalledVersions::getRootPackage()to retrieve the path and name of the root package under test. This way, that's the tested package that is linked into the temporary test projects.This assumes a certain structure of the maker package (
src,testsandtests/fixturesdirs,tests/tmpignored,tools/twigcssubproject).