fixing tested for windows system #1748

Merged
merged 3 commits into from Dec 17, 2012

Projects

None yet

6 participants

@fanno

This pull request is not done yet, i am still having some issues with the tester

21) JFormTest::testAddRulePath
Line:142 The libraries rule path should be included by default.
Failed asserting that false is true.

C:\Users\fanno\data\Projects\workspaces\phpstorm\joomla-platform\tests\suites\unit\joomla\form\JFormTest.php:143

now the issue is this ..

$valid = JPATH_PLATFORM . '/joomla/form/rules';

$valid is (C:\Users\fanno\data\Projects\workspaces\phpstorm\joomla-platform\libraries/joomla/form/rules)

and the path inside $paths is

C:\Users\fanno\data\Projects\workspaces\phpstorm\joomla-platform\libraries\joomla\form/rules

this comes from
https://github.com/fanno/joomla-platform/blob/staging/libraries/joomla/form/helper.php#L302

I am well aware that the user of DS is not needed anymore, however it complicates the testing

it could posible be solved by doing something like

$valid = realpath(JPATH_PLATFORM . '/joomla/form').'/rules';

however i think it is ugly but it it works it works ...

is this how the test should be made to look ? or how do you want to deal with it ?

@dongilbert

Are there many unit test failures that fail in a similar fashion? If not, you could add a special case to do some extra work before the assertion. (So put it after $valid is declared on L138)

if (IS_WIN)
{
    // Need to clean the path if we're on windows. It's a hack, I know.
    $valid = realpath(JPATH_PLATFORM . '/joomla/form') . '/rules';
}
@fanno

right now but i think they are all related to path / newline stuff (some are already fixed in this pull request)

@fanno

would the IS_WIN be needed ? would "realpath" not fix it for every system ?

@dongilbert

I would say yes, since it works every where except Windows. No need to add an extra function call if it already works. Also, it makes it clear what the purpose of the hack is to those in the future who look at it and might want to refactor the code. Because looking at it, the realpath call seems completely unnecessary and I would want to remove it. But having the context of IS_WIN makes it clear, and I wouldn't remove it.

@fanno

what do you want to do in the other locations i "fixed" in this pull request ?

@dongilbert

The PHP_EOL replacing "\n" is fine, nothing to do there, IMO

I'm not sure about the others - we'll need a professional opinion. I kind of still want to have a note saying not to remove the realpath.

@fanno

well the note could in all case just be a comment

// user of realpath to ensure test works for on all platforms
$valid = realpath(JPATH_PLATFORM . '/joomla/form') . '/rules';

....
....
// user of realpath to ensure test works for on all platforms
$this->assertEquals(realpath(DIR . '/layouts2/olivia.php'), $this->instance->getPath('olivia'));
$this->assertEquals(realpath(__DIR_
. '/layouts1/peter.php'), $this->instance->getPath('peter'));
$this->assertEquals(realpath(__DIR_
. '/layouts2/fauxlivia.php'), $this->instance->getPath('fauxlivia'));
$this->assertEquals(realpath(__DIR_
. '/layouts1/fringe/division.php'), $this->_instance->getPath('fringe/division'));
$this->assertFalse($this->_instance->getPath('walter'));

    // Check dirty path.
    $this->assertEquals(realpath(__DIR__ . '/layouts1/fringe/division.php'), $this->_instance->getPath('fringe//\\division'));
@dongilbert

That could work.

@fanno

fixed errors, now there is new error.

http://pastebin.com/ZQYCiRZt

my guess would be it also has to do with path but i am not sure ? but could also be a new line issue...

anyone have any hints ?

-Thanks

@fanno

as to http://pastebin.com/ZQYCiRZt i have figured out that the issue may be that

$content has "\r\n" in it

And

file_get_contents($path) do NOT.. (i would guess only \n i have not checked this yet)

what would be a good way to test this ?

$content = str_replace("\r\n","\n",$content);
$content = str_replace("\r","\n",$content);

$data = file_get_contents($path);

$data = str_replace("\r\n","\n",$data);
$data = str_replace("\r","\n",$data);

this would ensure that all line changes in the check are all the same ?

or maybe we should consider using regex in this case ?

eg:
$content = preg_replace('/\v/', '', $content);
$data = preg_replace('/\v/', '', $data);

it would when testing replace all vertical white space characters with nothing, and then they will compare equal. ?

-Thanks

@fanno

http://pastebin.com/QKWd3r73
Failures: 12

down to 12 .. realpath dont seem to help here, anyone have any suggestions ?

-Thanks

@elinw

It's a little hard to read parts of that but it looks like if we can figure out why the root is returning incorrectly it would probably tell how to fix the rest.

One issue you may be facing is that because some of those tests are really many separate tests in one method, as soon as the first test fails the method stops, so you fix one thing and then that part passes and you get further along and a test that wasn't running before fails.

@fanno

down to 8

Are the "errors" also a concern ?

@elinw

The link doesn't work. Yes errors matter.

@fanno

odd the link works for me =(

@pasamio

Looks like you're trying to run all of the database tests and they're failing because your system isn't configured to run the database tests. I didn't think they ran automatically however just copy the phpunit.xml.dist file (or similar) to phpunit.xml and disable the database driver tests.

@pasamio

The pull tester thinks that it's fine if that's any consolation:
http://developer.joomla.org/pulls/pulls/1748.html

@fanno

hmm i followed the instructings i believer it said to remove the comments ??

how do i disable the database test ?

@fanno

do i just re add the comments around the whole php tag ?

@fanno

i have tried:
const name="JTEST_DATABASE_MYSQLI_DSN" value="host=localhost;dbname=joomla_ut;user=utuser;pass=ut1234"

const name="JTEST_HTTP_STUB" value="http://localhost/joomla-platform/tests/suites/unit/stubs/jhttp_stub.php"

the others are commented out..

errors are down a lot but gut where do i find info on how to setup the databases for testing ? i cant find the docs ?..

@fanno

Think i figured it out ! =) progress is progress ! =P

much better now at least,

http://pastebin.com/2YGa5jLU

and now it looks like all falior and errors could be because of windows system path/newline issues.

@fanno

http://pastebin.com/JYZ7f6qj

gestting close. (only testing with mysql/mysqli atm)

-Thanks

@jfusion jfusion fixing tested for windows system
fixing tested for windows system

fixing tested for windows system

fixing tested for windows system

fixing tested for windows system

fixing tested for windows system
50b6875
@fanno

was finally able to make all the commits in to one this together, git still at times a bit tricky for me.

I have tested it on the unit tester with no fail / errors.

Someone with something other than windows need to run this test as well, as i cant be sure i have not broken something

Please also feel free to review the changes.

-Thanks

@pasamio pasamio commented on an outdated diff Dec 13, 2012
...uites/unit/joomla/application/JApplicationWebTest.php
@@ -471,6 +471,14 @@ public function testCompressWithDeflateEncoding()
*/
public function testCompressWithNoAcceptEncodings()
{
+ $string = 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do
+ eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
+ veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo
+ consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum
+ dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident,
+ sunt in culpa qui officia deserunt mollit anim id est laborum.';
+
+ //replace \r\n -> \n to ensure same length on all platforms
@pasamio
pasamio Dec 13, 2012

This should be // Replace to fit the check style standard.

@pasamio pasamio commented on an outdated diff Dec 13, 2012
...uites/unit/joomla/application/JApplicationWebTest.php
@@ -522,6 +525,14 @@ public function testCompressWithNoAcceptEncodings()
*/
public function testCompressWithHeadersSent()
{
+ $string = 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do
+ eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
+ veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo
+ consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum
+ dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident,
+ sunt in culpa qui officia deserunt mollit anim id est laborum.';
+
+ //replace \r\n -> \n to ensure same length on all platforms
@pasamio
pasamio Dec 13, 2012

See the earlier comment about this.

@pasamio pasamio commented on an outdated diff Dec 13, 2012
...omla/database/database/JDatabaseExporterMySqlTest.php
@@ -287,7 +285,12 @@ public function testBuildXml()
<key Table="#__test" Non_unique="0" Key_name="PRIMARY" Seq_in_index="1" Column_name="id" Collation="A" Null="" Index_type="BTREE" Comment="" />
</table_structure>
</database>
-</mysqldump>'
+</mysqldump>';
+ //replace used to prevent platform conflicts
@pasamio
pasamio Dec 13, 2012

See earlier comment about commenting style.

@pasamio pasamio commented on an outdated diff Dec 13, 2012
...database/database/JDatabaseExporterPostgresqlTest.php
@@ -291,11 +288,15 @@ public function test__toString()
<key Index="jos_dbtest_pkey" is_primary="TRUE" is_unique="TRUE" Query="ALTER TABLE "jos_dbtest" ADD PRIMARY KEY (id)" />
</table_structure>
</database>
-</postgresqldump>'
+</postgresqldump>';
+ //replace used to prevent platform conflicts
@pasamio pasamio commented on an outdated diff Dec 13, 2012
...database/database/JDatabaseExporterPostgresqlTest.php
@@ -365,7 +363,12 @@ public function testBuildXml()
<key Index="jos_dbtest_pkey" is_primary="TRUE" is_unique="TRUE" Query="ALTER TABLE "jos_dbtest" ADD PRIMARY KEY (id)" />
</table_structure>
</database>
-</postgresqldump>'
+</postgresqldump>';
+ //replace used to prevent platform conflicts
@pasamio
pasamio Dec 13, 2012

One more case.

@pasamio pasamio commented on an outdated diff Dec 13, 2012
...oomla/document/opensearch/JDocumentOpensearchTest.php
@@ -79,11 +79,10 @@ public function testRender()
$this->object->addUrl($item);
$this->object->addUrl($item2);
+ //replace used to prevent platform conflicts
@pasamio pasamio commented on an outdated diff Dec 13, 2012
...tes/unit/joomla/filesystem/JFilesystemPatcherTest.php
@@ -947,9 +949,14 @@ public function testApply($udiff, $root, $strip, $sources, $destinations, $resul
}
else
{
+ //remove all vertical characters to ensure system independed compare
@pasamio
pasamio Dec 13, 2012

One more case to fix up, just // Remove should do

@pasamio

The pull tester runs on Linux and appears to have run 15 minutes ago and was happy with things. I guess we're good to go with some updates that I've noted. Minor style things so that we're in a reasonable place.

@fanno

This should be // Replace to fit the check style standard.

What do you mean ?

@pasamio

The check style standard for comments says that comments should start with a capital and a space. You've got //replace which should be replaced with // Replace is all I mean.

@pasamio

It's not showing up on the pull tester because we're not testing the tests directory yet for that stuff however there is an effort to clean everything up so we can do it moving forwards.

@fanno

i will squash once again when its ready to be pulled =), when you think it is ready

@fanno

is there anyone with another system than windows that can give this pull request a test ?

@dongilbert
@fanno

Thanks very much =)

@elkuku
Joomla! member

I can confirm that the tests are still running good on my linux box using your branch.

@pasamio pasamio merged commit 84b4b65 into joomla:staging Dec 17, 2012
@pasamio

Looks good to me, merged.

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