Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Tests] StreamlineCompiledUrlGenerator tests#52481
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $compiledUrlGenerator->generate('a'); | ||
| } | ||
| publicfunctiontestTargetAliasWithNamePrefixNotExisting() |
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.
after investigating why testTargetAliasWithNamePrefixNotExisting are now failing I found out, that both, CompiledUrlGeneratorDUMPER and CompiledUrlGenerator throw a RouteNotFoundException. We are testing $compiledUrlGenerator->generate('sub_a) here, but the exception is already thrown in file_put_contents($this->testTmpFilepath, $this->generatorDumper->dump()); when generating the dump to prepare a file.
So now after moving the expectedException method call down, I can say, that we are actually not testing the behavior of CompiledUrlGenerator in this test 🤷♂️
Same applies to testTargetAliasNotExisting
CompiledUrlGenerator tests35dec94 to9aaf1f0Comparealexandre-daubois commentedDec 27, 2023
What about: diff --git a/src/Symfony/Component/Routing/Tests/Generator/Dumper/CompiledUrlGeneratorDumperTest.php b/src/Symfony/Component/Routing/Tests/Generator/Dumper/CompiledUrlGeneratorDumperTest.phpindex 8603ab6d98..572f524a05 100644--- a/src/Symfony/Component/Routing/Tests/Generator/Dumper/CompiledUrlGeneratorDumperTest.php+++ b/src/Symfony/Component/Routing/Tests/Generator/Dumper/CompiledUrlGeneratorDumperTest.php@@ -267,66 +272,72 @@ class CompiledUrlGeneratorDumperTest extends TestCase public function testTargetAliasNotExisting() {- $this->expectException(RouteNotFoundException::class);-- $this->routeCollection->addAlias('a', 'not-existing');+ $this->routeCollection->add('not-existing', new Route('/not-existing'));+ $this->routeCollection->addAlias('alias', 'not-existing'); file_put_contents($this->testTmpFilepath, $this->generatorDumper->dump());- $compiledUrlGenerator = new CompiledUrlGenerator(require $this->testTmpFilepath, new RequestContext());+ $compiledRoutes = require $this->testTmpFilepath;+ unset($compiledRoutes['alias']);+ $this->expectException(RouteNotFoundException::class);++ $compiledUrlGenerator = new CompiledUrlGenerator($compiledRoutes, new RequestContext()); $compiledUrlGenerator->generate('a'); } public function testTargetAliasWithNamePrefixNotExisting() {- $this->expectException(RouteNotFoundException::class);- $subCollection = new RouteCollection();- $subCollection->addAlias('a', 'not-existing');+ $subCollection->add('not-existing', new Route('/not-existing'));+ $subCollection->addAlias('alias', 'not-existing'); $subCollection->addNamePrefix('sub_'); $this->routeCollection->addCollection($subCollection); file_put_contents($this->testTmpFilepath, $this->generatorDumper->dump());- $compiledUrlGenerator = new CompiledUrlGenerator(require $this->testTmpFilepath, new RequestContext());+ $compiledRoutes = require $this->testTmpFilepath;+ unset($compiledRoutes['sub_alias']);- $compiledUrlGenerator->generate('sub_a');+ $this->expectException(RouteNotFoundException::class);++ $compiledUrlGenerator = new CompiledUrlGenerator($compiledRoutes, new RequestContext());+ $compiledUrlGenerator->generate('sub_alias'); } We generate the file with the existing route then we remove it from compiled ones. This way, we ensure that we test the exception coming from route generation. Works on my computer 😄 |
OskarStark commentedDec 27, 2023
Thanks, lets see |
alexandre-daubois commentedDec 27, 2023
Seems to do the job! |
cdc20fb toa778254CompareOskarStark commentedDec 27, 2023
@nicolas-grekas I think this should be merged into |
nicolas-grekas commentedDec 27, 2023
Works for me, please rebase. |
a778254 to807d70eCompareOskarStark commentedDec 27, 2023
Rebased ✅ |
fabpot commentedDec 29, 2023
Thank you@OskarStark. |
Uh oh!
There was an error while loading.Please reload this page.
weird, it looks like the expected exception is thrown in $this->generatorDumper->dump() 🤔 So the following code with the CompiledUrlGenerator is not needed, so maybe the test is not needed anymore or the test must be adjusted somehow to test the behavior of CompiledUrlgenerator....