-
-
Notifications
You must be signed in to change notification settings - Fork 355
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
make test directory configurable #283
Conversation
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 looks good to me. Just wondering if this could be done at Plugin level? https://github.com/pestphp/pest-plugin-coverage/blob/50260c8671377a5082a06b9e228a984c13ed199c/src/Plugin.php#L76
I tried that initially but there was one place that would result in issues because the custom path would be available too late, don't remember right now which. I'll take another look tomorrow at doing it that way. |
Just had a quick look and I think it was this line https://github.com/pestphp/pest/blob/master/src/Plugin.php#L25 that is causing issues with doing it inside of plugins because plugins require this method but this method already needs to know about the path before any plugins do so it'll fail at that point. |
@faustbrian This is ready to be merged? |
Should be if you have no further requests for changes. |
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.
@olivernybroe Plugin / IDE wise, any changes required?
src/Pest.php
Outdated
|
||
function testDirectory(string $file = ''): string | ||
{ | ||
return $_ENV['PEST_TEST_DIRECTORY'] . $file; |
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.
Replace this by $_SERVER
.
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 this occurrence or all $_ENV
usages in this PR?
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.
Can you actually use the TestSuite::getInstance()->setTestDirectory()
instead of using ENVs?
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.
Let me test but if I remember right it wasn't possible because the test path had to be set before the instance was 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.
Yeah that doesn't work because https://github.com/pestphp/pest/blob/master/src/TestSuite.php#L98-L100 ends up executing some things that already need access to the test path.
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.
An alternative could be this change.
$testSuite = TestSuite::getInstance($rootPath, (new ArgvInput())->getParameterOption('--test-directory', 'tests'));
public static function getInstance(string $rootPath = null, string $testPath = null): TestSuite
{
if (is_string($rootPath)) {
self::$instance = new TestSuite($rootPath);
if (is_string($testPath)) {
self::$instance->testPath = $testPath;
}
foreach (Plugin::$callables as $callable) {
$callable();
}
return self::$instance;
}
if (self::$instance === null) {
throw new InvalidPestCommand();
}
return self::$instance;
}
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.
Alternative https://github.com/faustbrian/pest/commit/c8a3e7e1f4747f838292b75e44be8ac33892f2ff but results in an odd issue I need to check if this is preferred.
❯ bin/pest --test-directory=tests
PHPUnit 9.5.3 by Sebastian Bergmann and contributors.
Unknown option "--test-directory"
Hmm, by a quick look I don't think it will have an impact on the plugin. The only thing is that this is of course not configurable via the plugin, so if we want the user to be able to configure this, we need to add a new feature. (if they don't use the env parameter mode) |
Pushed the changes from #283 (comment) but will need to check tomorrow what causes the |
@nunomaduro one issue with this implementation which might be worth considering is that there are a few places that create a |
@faustbrian That's fine. Can you make a pull request to docs? |
Will submit a PR for documentation tomorrow. Just pushed a fix for the |
Updated the PR description and submitted the docs PR https://github.com/pestphp/docs/pull/41/files |
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.
@owenvoke Can you test this pull request locally?
This seems to be working well for me, I was having issues originally (but they were due to symlinking when requiring the package), and then further issues (but that was as my I'd be happy if someone else could also test this just to be sure, but I think it should be fine. 👍🏻 |
It does so by introducing a new
--test-directory
argument. If this flag is set you can callPest\testDirectory
which will return the path value and optionally concatenate it with a filename. CallingtestDirectory('directory/File.php')
will returntests/directory/File.php
by default.