-
-
Notifications
You must be signed in to change notification settings - Fork 438
Tweaks to the new make:test #810
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
| $command | ||
| ->addArgument('name', InputArgument::OPTIONAL, 'The name of the test class (e.g. <fg=yellow>BlogPostTest</>)') | ||
| ->addArgument('type', InputArgument::OPTIONAL, 'The type of test: '.implode(', ', $typesDesc)) | ||
| ->addArgument('name', InputArgument::OPTIONAL, 'The name of the test class (e.g. <fg=yellow>BlogPostTest</>)') |
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.
Once I played with this, I decided to reverse the arguments. It occurred to me that knowing the "type" as early as possible could help us. For example, I don't do it in this PR, but we could suggest different example filenames on the next question based on the type.
| $io->warning([ | ||
| 'API Platform is required for this test type. Install it with', | ||
| 'composer require api', | ||
| ]); |
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.
Unfortunately, the dependency system is called before the interactive step. So previously, no error or warning was shown. That's actually ok - I like showing a warning better than exploding with an error that they must stop and run the composer require command before starting the make:test command again.
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.
Agreed, seeing as we're not actually collecting a bunch of input that is dependent on Api Platform at this time - the warning makes sense..
| 'Choose a class name for your test, like:', | ||
| ' * <fg=yellow>UtilTest</> (to create tests/UtilTest.php)', | ||
| ' * <fg=yellow>Service\\UtilTest</> (to create tests/Service/UtilTest.php)', | ||
| ' * <fg=yellow>\\App\Tests\\Service\\UtilTest</> (to create tests/Service/UtilTest.php)', |
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.
Could we (should we) suggest the class based on the test? e.g. if the type is UnitTest - we suggest 'App\\Tests\\UnitTests\\{NAME} or something along those lines?
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 thought about that too. Let's save it for a future PR... I don't think there is clear agreement on the directory conventions of these things. I personally don't sub-divide things into UnitTest or IntegrationTest directories, but I know that some people do this.
src/Maker/MakeTest.php
Outdated
| return; | ||
|
|
||
| case 'make:functional-test': | ||
| $input->setArgument('type', trait_exists(PantherTestCaseTrait::class) ? 'WebTestCase' : 'PantherTestCase'); |
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 caught this - shouldn't this be asked? What if I want a WebTestCase to check the status codes of a couple routes but want PantherTestCase for browser based tests.
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 guess even before this, if you used make:functional-test, we already looked to see if you had Panther available and then used it automatically. It's a minor thing, since this is deprecated. The new system of actually asking is much better. But I'll keep this the way it is now... which is the way it was before mostly.
662e37b to
a004185
Compare
1b2e043 to
70f63b0
Compare
This follows #807 - I played with it locally and had some minor tweaks :).
Cheers!