Routing: Introducing unittests for core app router classes #3759

Closed
wants to merge 46 commits into
from

Conversation

Projects
None yet
@Hackwar
Member

Hackwar commented Jun 11, 2014

This PR introduces a large chunk of unittests to document the behavior of the core application routing classes.

This was made possible through the generous donation of the people mentioned in the following link via an Indiegogo campaign: http://joomlager.de/crowdfunding/5-contributors

tests/unit/core/mock/menu.php
+ */
+ public static function create(PHPUnit_Framework_TestCase $test)
+ {
+ // Collect all the relevant methods in JApplication (work in progress).

This comment has been minimized.

@wilsonge

wilsonge Jun 11, 2014

Contributor

Guessing this is a list of methods in JMenu?

@wilsonge

wilsonge Jun 11, 2014

Contributor

Guessing this is a list of methods in JMenu?

This comment has been minimized.

@Hackwar

Hackwar Jun 11, 2014

Member

yes, you are right. Will change that in an upcoming commit.

@Hackwar

Hackwar Jun 11, 2014

Member

yes, you are right. Will change that in an upcoming commit.

@@ -497,10 +531,6 @@ protected function buildSefRoute(&$uri)
{
$route .= '/' . $tmp;
}
- elseif ($route == 'index.php')

This comment has been minimized.

@Hackwar

Hackwar Jun 11, 2014

Member

This conidition is unreachable. I couldn't find one situation where this could ever be true.

@Hackwar

Hackwar Jun 11, 2014

Member

This conidition is unreachable. I couldn't find one situation where this could ever be true.

@@ -572,15 +572,14 @@ protected function _createURI($url)
*/
protected function createURI($url)
{
- if (is_array($url))

This comment has been minimized.

@Hackwar

Hackwar Jun 11, 2014

Member

Just to clarify: The behavior of createURI() was to either create JURI object from urls that were a "complete" relative URL, like 'index.php?option=com_content', but merge the current request variables into URLs that were similar to '&format=feed&type=rss' in order to complete them. A short time ago, the possibility of array-URL-parameters was introduced. With this change, those are behaving similar to the incomplete URLs now.

@Hackwar

Hackwar Jun 11, 2014

Member

Just to clarify: The behavior of createURI() was to either create JURI object from urls that were a "complete" relative URL, like 'index.php?option=com_content', but merge the current request variables into URLs that were similar to '&format=feed&type=rss' in order to complete them. A short time ago, the possibility of array-URL-parameters was introduced. With this change, those are behaving similar to the incomplete URLs now.

+ $object2 = JRouter::getInstance('administrator');
+ $this->assertSame($object, $object2);
+
+ //Check if a proper exception is thrown if there is no router class

This comment has been minimized.

@wilsonge

wilsonge Jun 11, 2014

Contributor

Surely we should assert an exception here?

@wilsonge

wilsonge Jun 11, 2014

Contributor

Surely we should assert an exception here?

This comment has been minimized.

@mbabker

mbabker Jun 11, 2014

Member

See the @expectedException in the doc block. Personally, I'd split this method into 2 methods; one testing the stuff above and one to test for the exception.

@mbabker

mbabker Jun 11, 2014

Member

See the @expectedException in the doc block. Personally, I'd split this method into 2 methods; one testing the stuff above and one to test for the exception.

This comment has been minimized.

@wilsonge

wilsonge Jun 11, 2014

Contributor

my bad. i always forget about the doc block thingy :/ agree a separate test would be easier to spot that :)

@wilsonge

wilsonge Jun 11, 2014

Contributor

my bad. i always forget about the doc block thingy :/ agree a separate test would be easier to spot that :)

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Jun 12, 2014

Member

I'm going to add further commits to this and further test this until we reach 100% code coverage. With the commits above this comment, we got this done for JRouter and JRouterAdministrator.

Member

Hackwar commented Jun 12, 2014

I'm going to add further commits to this and further test this until we reach 100% code coverage. With the commits above this comment, we got this done for JRouter and JRouterAdministrator.

libraries/cms/router/site.php
+ {
+ parent::__construct($options);
+
+ if ($app)

This comment has been minimized.

@wilsonge

wilsonge Jun 13, 2014

Contributor

I know I wrote this code (sorry!) but this would be better as

$this->app = $app ? $app : JApplication::getInstance('site'); $this->menu = $menu ? $menu : $this->app->getMenu();

in retrospect :)

@wilsonge

wilsonge Jun 13, 2014

Contributor

I know I wrote this code (sorry!) but this would be better as

$this->app = $app ? $app : JApplication::getInstance('site'); $this->menu = $menu ? $menu : $this->app->getMenu();

in retrospect :)

Hackwar added some commits Jun 15, 2014

Fixing JRouter test,
fixing JRouter,
adding mock methods to JMenu Mock
@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Jun 15, 2014

This is done in $this->getMockCmsApp(); I think?

This is done in $this->getMockCmsApp(); I think?

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Jun 17, 2014

Member

Ok, so, I guess this PR would be ready to test now... 😉

Member

Hackwar commented Jun 17, 2014

Ok, so, I guess this PR would be ready to test now... 😉

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Jul 18, 2014

Member

This PR is still up-to-date

Member

Hackwar commented Jul 18, 2014

This PR is still up-to-date

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Aug 30, 2014

Contributor

Ok, so, I guess this PR would be ready to test now...

This PR is still up-to-date

@Hackwar how can we test it? Can we have test instructions?

Contributor

zero-24 commented Aug 30, 2014

Ok, so, I guess this PR would be ready to test now...

This PR is still up-to-date

@Hackwar how can we test it? Can we have test instructions?

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Aug 30, 2014

Member

This needs code reviews by maintainers and not tests.

Member

Hackwar commented Aug 30, 2014

This needs code reviews by maintainers and not tests.

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Aug 30, 2014

Contributor

thanks @Hackwar

Contributor

zero-24 commented Aug 30, 2014

thanks @Hackwar

tests/unit/core/mock/application.php
@@ -45,6 +46,11 @@ public static function create($test)
// Call original constructor.
false
);
+

This comment has been minimized.

@zero-24

zero-24 Aug 30, 2014

Contributor

can we fix the space here?

@zero-24

zero-24 Aug 30, 2014

Contributor

can we fix the space here?

tests/unit/core/mock/menu.php
+class TestMockMenu
+{
+ protected static $data = array();
+

This comment has been minimized.

@zero-24

zero-24 Aug 30, 2014

Contributor

same here

@zero-24

zero-24 Aug 30, 2014

Contributor

same here

tests/unit/core/mock/menu.php
+ // Call original constructor.
+ false
+ );
+

This comment has been minimized.

@zero-24

zero-24 Aug 30, 2014

Contributor

and here

@zero-24

zero-24 Aug 30, 2014

Contributor

and here

tests/unit/core/mock/menu.php
+ 'tree' => array(42, 46),
+ 'query' => array('option' => 'com_test', 'view' => 'test'));
+
+ /** self::$data[47] = (object) array(

This comment has been minimized.

@zero-24

zero-24 Aug 30, 2014

Contributor

it is expected that this part is commented

@zero-24

zero-24 Aug 30, 2014

Contributor

it is expected that this part is commented

zero-24 and others added some commits Aug 30, 2014

Fix Travis
FILE: /home/travis/build/joomla/joomla-cms/libraries/cms/router/site.php

--------------------------------------------------------------------------------

FOUND 1 ERROR(S) AFFECTING 1 LINE(S)

--------------------------------------------------------------------------------

413 | ERROR | Expected 2 spaces after the longest type

--------------------------------------------------------------------------------
@hans2103

This comment has been minimized.

Show comment
Hide comment
@hans2103

hans2103 Sep 29, 2014

Contributor

Any news here?

Contributor

hans2103 commented Sep 29, 2014

Any news here?

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Sep 29, 2014

Contributor

@Bakual @phproberto @mbabker can you guys do the code review?

Contributor

roland-d commented Sep 29, 2014

@Bakual @phproberto @mbabker can you guys do the code review?

@allrude

This comment has been minimized.

Show comment
Hide comment
@allrude

allrude Sep 29, 2014

Love to here the status of this, wil it be in J 3.4 ?

allrude commented Sep 29, 2014

Love to here the status of this, wil it be in J 3.4 ?

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Sep 29, 2014

Member

Don't know how this got RTC, but Imho needs tests as the PR does not only contains unittests but extensive changes in code.

Member

infograf768 commented Sep 29, 2014

Don't know how this got RTC, but Imho needs tests as the PR does not only contains unittests but extensive changes in code.

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Sep 29, 2014

Contributor

I set it RTC. I have tested as has @zero-24

Contributor

wilsonge commented Sep 29, 2014

I set it RTC. I have tested as has @zero-24

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Sep 29, 2014

Contributor

I set it RTC. I have tested as has @zero-24

I have test navigation in Front- and Backend on a mono langauge site (en) and on a multilingual site (de and en). If we have more that need testing or spezial cases/szenarios that need testing let me know 👍

Contributor

zero-24 commented Sep 29, 2014

I set it RTC. I have tested as has @zero-24

I have test navigation in Front- and Backend on a mono langauge site (en) and on a multilingual site (de and en). If we have more that need testing or spezial cases/szenarios that need testing let me know 👍

@mbabker mbabker added this to the Joomla! 3.4.0 milestone Sep 29, 2014

phproberto added a commit that referenced this pull request Oct 2, 2014

[imp] Routing: Introducing unittests for core app router classes. Fixes
#3759

Introducing more unittests for the application router classes

Improve DI aspect

Update doc block

Add mocking for getMenu method

Use getMockCmsApp

Keep code style consistent

Fixing Unittest

Splitting getInstance tests for exceptions

Adding further tests for JRouter

Implementing unittest for JRouterSite::build

Fixing JRouter test,
fixing JRouter,
adding mock methods to JMenu Mock

More unittests

Fixing JRouter Tests

Try setting the default server vars

Fix build expected results

Cleaning up tests

More cleanup

Removing const's

Fixing unittests... again...

Replacing JApplication with JApplicationCms

Finalising Unittests for JRouter and its child-classes

Update router.php

Update site.php

Update application.php

Update menu.php

Update JApplicationHelperInspector.php

Update JRouterAdministratorTest.php

Update JRouterSiteTest.php

Update JRouterAdministratorTest.php

Update JRouterTest.php

Update JRouterInspector.php

Update JRouterSiteInspector.php

Update JRouterSiteTest.php

Fix Travis

FILE: /home/travis/build/joomla/joomla-cms/libraries/cms/router/site.php

--------------------------------------------------------------------------------

FOUND 1 ERROR(S) AFFECTING 1 LINE(S)

--------------------------------------------------------------------------------

413 | ERROR | Expected 2 spaces after the longest type

--------------------------------------------------------------------------------
@phproberto

This comment has been minimized.

Show comment
Hide comment
@phproberto

phproberto Oct 2, 2014

Contributor

Merged in 3.4-dev branch c8c6306

Thanks!!

Contributor

phproberto commented Oct 2, 2014

Merged in 3.4-dev branch c8c6306

Thanks!!

@phproberto phproberto closed this Oct 2, 2014

@zero-24 zero-24 removed the RTC label Oct 14, 2015

@Hackwar Hackwar deleted the Hackwar:routing_unittests branch Jan 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment