Skip to content
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

feat: Make AppKernel with KernelTestCase compatible #44

Merged
merged 12 commits into from Aug 19, 2021

Conversation

chapterjason
Copy link
Contributor

@chapterjason chapterjason commented Jul 30, 2021

  • Fix AppKernel constructor to be compatible with the KernelTestCase
  • Add tests for usage with KernelTestCase
  • Remove BaseBundleTestCase
  • Update example in Readme

fixes: #39

@chapterjason chapterjason force-pushed the feature/framework-compatible branch 3 times, most recently from 7739be9 to f8f4f6f Compare July 30, 2021 05:07
@chapterjason
Copy link
Contributor Author

chapterjason commented Jul 30, 2021

@Nyholm I don't know how to deal with the failing tests:

PHP Fatal error:  Declaration of Symfony\Bundle\FrameworkBundle\Test\KernelTestCase::tearDown() must be compatible with PHPUnit\Framework\TestCase::tearDown(): void in /home/runner/work/symfony-bundle-test/symfony-bundle-test/vendor/symfony/framework-bundle/Test/KernelTestCase.php on line 24

Just found the following: symfony/symfony#30071

@chapterjason
Copy link
Contributor Author

@Nyholm I found a solution, hope we can use it like that.

src/AppKernel.php Outdated Show resolved Hide resolved
@Nyholm
Copy link
Member

Nyholm commented Aug 2, 2021

Awesome. Thank you.

Could you update this PR after bumping composer.json. See #45 (comment)

@chapterjason
Copy link
Contributor Author

@Nyholm Yes, I have done some other changes too, I would like do this at the end because of the renamed tests. :)

@chapterjason
Copy link
Contributor Author

@Nyholm Also ready now, without the breaking change layer. Ready for a major.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this

Readme.md Outdated Show resolved Hide resolved
src/AppKernel.php Outdated Show resolved Hide resolved
@@ -208,4 +214,35 @@ public function setRoutingFile($routingFile)
{
$this->routingFile = $routingFile;
}

public function handleOptions(array $options)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new feature you snuck in. Why is it better to have an array of values rather than calling the underlying methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 Yeah.

I liked the idea of the options in the KernelTestCase, you are right, the methods are better.

The reason why I introduced this is: You only have the possibility to change the config by override the createKernel this limits it to a configuration per file and not per test. (If you don't want to boot it yourself and just want to use static:bootKernel.)

By just call the handleOptions in the createKernel you can actually passthrough everything before the kernel is booted and the container is compiled.

Another Idea, that just came in my mind, we could just create a option like config which must be a callable:

static::bootKernel(['config' => static function(AppKernel $kernel){
    $kernel->setRoutingFile(__DIR__.'/Fixtures/routes.yml');
}]);

Then we can benefit from the methods and save some lines, cause the KernelTestCase makes the booting process.

tests/Functional/BundleConfigurationTest.php Outdated Show resolved Hide resolved
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

/**
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
abstract class BaseBundleTestCase extends TestCase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Im happy to remove this class.

@Nyholm Nyholm merged commit 1ea1ed9 into SymfonyTest:master Aug 19, 2021
@chapterjason chapterjason deleted the feature/framework-compatible branch August 21, 2021 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compatibility with FrameworkBundle Test Traits
2 participants