Testing fixes and improvements #276
Conversation
|
awesome, the tests are run and green, according to travis! to test the extensions, lets just configure them and in the tests we already have check if they injected their bits. oh, for the RouteReferrersExtension, this would be very complicated. we would need to create a mock document and an admin for it, just to test the tab. maybe lets only test the FrontendLink here, and for the rest see if we can add some tests in the cmf-sandbox where we actually have admins configured. |
|
I thought about decoupling the extension's tests from the rest (writing a separate test case), because the FrontendLinkExtension is not an integral component of the bundle and it simplifies understanding of the tests in the future. What do you think about 75aa756 (incomplete yet)? |
|
looks sane to me. the only reason why i would have mixed this test into the normal admin test is that if something is wrong with the admin in general, we now will have 2 tests failing. if you configure the test application to load the extension, the inverse is likely true too: if the extension is broken, the base test will also fail as the page is not loading anymore. thats why i thought of testing that bit at the end of the existing admin tests (maybe with a method like |
|
Extending the existing tests seems the better variant. Changed code accordingly... I also did some improvement on the Extension itself to ensure compatibility with Symfony 2.6 |
|
Is here anything left to do? /cc @dbu @ElectricMaxxx |
| * @param string $action | ||
| * @param AdminInterface $childAdmin | ||
| * @return mixed|void | ||
| * @throws \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException |
There was a problem hiding this comment.
cs: please have an empty line between param and return and between return and throws.
actually, please either explain the parameters or do not add a phpdoc - as is, everything except throws can be seen in the method signature.
There was a problem hiding this comment.
the @throws should stay either way. we rely on the use statement so it should just be the local class name of the exception without path
There was a problem hiding this comment.
As this is a public method I would say that at least a minimal description of the method should be required. and I would always document the parameters and returns even without descriptions - if we add docblocks to methods then they should be complete imo.
There was a problem hiding this comment.
i just hate @param string $action The action style comments. rather really miss them so its visible. but indeed it would make a lot of sense to explain for example what that string means and what it may be.
oh, or as this actually is implementing an interface method, the right thing would be {@inheritDoc} first and then whatever specific doc we have for this class ("Adds a tab linking to this document in the frontend.")
There was a problem hiding this comment.
Yeah of course if this is an interface method then we happily just use {@inheritDoc} :)
|
excellent! can you cleanup the CS issues and squash the commits. i totally forgot about #232 - i think most of 232 will be resolved by what you do here. |
frne
force-pushed
the
sonata-extension-testing
branch
from
1fe70dd to
18ad220
Compare
frne
force-pushed
the
sonata-extension-testing
branch
from
18ad220 to
ff2c76a
Compare
|
done. |
dbu
changed the title
|
awesome, thanks a lot! |
TODO
Part of the discussion in #275...
PR-Doc