Replace SimpleTest with PHPUnit #96

Merged
merged 102 commits into from May 1, 2012

Conversation

Projects
None yet
8 participants
@dom-mel
Collaborator

dom-mel commented Apr 18, 2012

This pull request migrates the SimpleTest test suite to PHPUnit. The initial ideas came from http://www.dokuwiki.org/devel:ideas:testing_system .

Detailed information about the testing framework can be found at https://github.com/dom-mel/dokuwiki/blob/phpunit/_test/README

Most unittests are migrated to PHPUnit, except for:

  • inc/html_hilight (runkit)
  • inc/parser/xhtml_htmlphp (runkit)
  • inc/indexer_idx_indexlengths (fs dependencies)
  • inc/mail_send (integration test)
  • inc/parser/parser_formatting
  • inc/parser/xhtml_links

In addition to unit tests, it is now possible to easily create integration tests. It allows to fake whole HTTP requests to DokuWiki with runtime inspection of those requests. You can also hook DokuWiki events. An example can be found in https://github.com/dom-mel/dokuwiki/blob/phpunit/_test/tests/test/hooks.test.php .

dom-mel added some commits Apr 1, 2012

added plugin unit tests to phpunit config
plugin unit tests are in /lib/plugins/<pluginname>/_testing

sarnowski and others added some commits Apr 18, 2012

Merge branch 'master' into phpunit
* master: (29 commits)
  some edge case checking in search result highlighting
  Resolve empty page ID to configured start page
  Added more detail error code for unauthorized calls in xmlrpc interface.
  changed internal Mailer members from private to protected
  made it possible to disable HTML mails in the config
  removed commented code left from the quoted_printable days
  add missing table tags for HTML diff mails
  fixed subscriber mail signatures
  use real HRs in HTML mails
  Add various headers to the mails FS#2247. pull request #83 closed
  removed footer image from HTML mails
  use inlinestyles for diffs in HTML mails
  fixed signature stripping
  fixed mailprefix handling
  fixed missing replacement for HTML notify mails
  allow image embeds in HTML mail templates
  Added HTML wrapper for mails
  allow non-txt extensions when accessing locales
  added Mailer class to autoloader
  Replaced mail_send calls with new Mailer class
  ...
added html_hilight test
This didn't really need runkit. The way runkit was used didn't add any
more credibility to the test result
some cleanup
removed unneeded data files, converted tabs to spaces
@splitbrain

This comment has been minimized.

Show comment Hide comment
@splitbrain

splitbrain Apr 20, 2012

Owner

I'd be fine with merging this as is, because its already much better than the old suite and users aren't affected anyway. @adrianlang, @Chris--S, @michitux what's your opinion?

About the missing tests:

  • we should provide the adequate data directory for FS-dependent tests
  • runkit could be replaced by PHPunit test_helper extension
  • mail_send should be replaced by a test for Mailer class
  • the broken tests from the old suite can be dropped
Owner

splitbrain commented Apr 20, 2012

I'd be fine with merging this as is, because its already much better than the old suite and users aren't affected anyway. @adrianlang, @Chris--S, @michitux what's your opinion?

About the missing tests:

  • we should provide the adequate data directory for FS-dependent tests
  • runkit could be replaced by PHPunit test_helper extension
  • mail_send should be replaced by a test for Mailer class
  • the broken tests from the old suite can be dropped

splitbrain and others added some commits Apr 20, 2012

@dom-mel

This comment has been minimized.

Show comment Hide comment
@dom-mel

dom-mel Apr 21, 2012

Collaborator

I've test the suite on windows. It worked ;-)

But for some reasons you may have to enable HTTPS support for php.
I've added a note in the README.

Collaborator

dom-mel commented Apr 21, 2012

I've test the suite on windows. It worked ;-)

But for some reasons you may have to enable HTTPS support for php.
I've added a note in the README.

@splitbrain

This comment has been minimized.

Show comment Hide comment
@splitbrain

splitbrain Apr 21, 2012

Owner

Nice, could you add a simple description on how to install phpunit on Windows?

Owner

splitbrain commented Apr 21, 2012

Nice, could you add a simple description on how to install phpunit on Windows?

@michitux

This comment has been minimized.

Show comment Hide comment
@michitux

michitux Apr 25, 2012

Collaborator

I think this is great, the only thing that didn't work for me (after adapting or removing the plugin tests I had) is the http client test, somehow it wasn't able to reach httpbin.org and I also couldn't test it alone, then it fails with an error that it can't find the HTTPClient class. But as we had similar and most probably even more problems with the old testing system I don't think this is a reason against merging this branch.

Collaborator

michitux commented Apr 25, 2012

I think this is great, the only thing that didn't work for me (after adapting or removing the plugin tests I had) is the http client test, somehow it wasn't able to reach httpbin.org and I also couldn't test it alone, then it fails with an error that it can't find the HTTPClient class. But as we had similar and most probably even more problems with the old testing system I don't think this is a reason against merging this branch.

@dom-mel

This comment has been minimized.

Show comment Hide comment
@dom-mel

dom-mel Apr 26, 2012

Collaborator

Sounds like you habe not installed the PHP SSL handler, can you provide the stacktrace?

Collaborator

dom-mel commented Apr 26, 2012

Sounds like you habe not installed the PHP SSL handler, can you provide the stacktrace?

splitbrain added some commits Apr 26, 2012

make HTTPClient loadable via autoloader
this fixes the HTTP tests which do test the base class directly instead
of the DokuHTTPClient subclass
@splitbrain

This comment has been minimized.

Show comment Hide comment
@splitbrain

splitbrain Apr 26, 2012

Owner

@michitux I fixed the problem with running the tests on its own and also check for SSL support now before running SSL tests. Could you try again?

Owner

splitbrain commented Apr 26, 2012

@michitux I fixed the problem with running the tests on its own and also check for SSL support now before running SSL tests. Could you try again?

splitbrain added some commits Apr 24, 2012

reenabled password test
according to Dominik Eckelmann  one of the tests fails on certain servers. I can't
reproduce it. If you can, please open a bug report with as much info as
possible.
@sarnowski

This comment has been minimized.

Show comment Hide comment
@sarnowski

sarnowski Apr 28, 2012

Contributor

@splitbrain dom-mel's server is one of those machines which hangs on this test and iirc my old computer @ cosmocode also ran forever.

http://dwbuilds.linesofcode.org/phpunit-57.txt runs for nearly two days now ;-)

edit: we thought that it may be some cpu flags (maybe AES-NI[1]?)

[1] http://en.wikipedia.org/wiki/AES_instruction_set

Contributor

sarnowski commented Apr 28, 2012

@splitbrain dom-mel's server is one of those machines which hangs on this test and iirc my old computer @ cosmocode also ran forever.

http://dwbuilds.linesofcode.org/phpunit-57.txt runs for nearly two days now ;-)

edit: we thought that it may be some cpu flags (maybe AES-NI[1]?)

[1] http://en.wikipedia.org/wiki/AES_instruction_set

@sarnowski

This comment has been minimized.

Show comment Hide comment
@sarnowski

sarnowski Apr 28, 2012

Contributor

I tracked it down to the pmd5 and hmd5 tests - both are extreme cpu intensive (and do not finish on this test machine within an acceptable time frame).

Contributor

sarnowski commented Apr 28, 2012

I tracked it down to the pmd5 and hmd5 tests - both are extreme cpu intensive (and do not finish on this test machine within an acceptable time frame).

@sarnowski

This comment has been minimized.

Show comment Hide comment
@sarnowski

sarnowski Apr 28, 2012

Contributor

Another hint I want to mention is that the @author annotation is an alias for @group[1]. This results in the following group list:

$ phpunit --list-groups
PHPUnit 3.6.10 by Sebastian Bergmann.

Available test group(s):
 - Andreas Gohr <andi@splitbrain.org>
 - Denis Scheither <amorphis@uni-bremen.de>
 - __nogroup__
 - integration
 - internet
 - slow

This is a "feature" of phpunit which may be a good use case but is senseless right now (only the romanize utf8 test contains two @author annotations). Just give it a thought if this is ok or not.

[1] http://www.phpunit.de/manual/3.6/en/appendixes.annotations.html

Contributor

sarnowski commented Apr 28, 2012

Another hint I want to mention is that the @author annotation is an alias for @group[1]. This results in the following group list:

$ phpunit --list-groups
PHPUnit 3.6.10 by Sebastian Bergmann.

Available test group(s):
 - Andreas Gohr <andi@splitbrain.org>
 - Denis Scheither <amorphis@uni-bremen.de>
 - __nogroup__
 - integration
 - internet
 - slow

This is a "feature" of phpunit which may be a good use case but is senseless right now (only the romanize utf8 test contains two @author annotations). Just give it a thought if this is ok or not.

[1] http://www.phpunit.de/manual/3.6/en/appendixes.annotations.html

@splitbrain

This comment has been minimized.

Show comment Hide comment
@splitbrain

splitbrain Apr 29, 2012

Owner

Hmm, the PMD5 hashing method is supposed to be CPU intensive (similar to bcrypt) bt of course should still be reasonable. There's a iteraration loop in it (https://github.com/splitbrain/dokuwiki/blob/master/inc/PassHash.class.php#L346) my only explanation would be that this loop is running forever on your machine for some reason. Could you try adding some debug statements to see if that's the case and where it goes wrong?

Owner

splitbrain commented Apr 29, 2012

Hmm, the PMD5 hashing method is supposed to be CPU intensive (similar to bcrypt) bt of course should still be reasonable. There's a iteraration loop in it (https://github.com/splitbrain/dokuwiki/blob/master/inc/PassHash.class.php#L346) my only explanation would be that this loop is running forever on your machine for some reason. Could you try adding some debug statements to see if that's the case and where it goes wrong?

@dom-mel

This comment has been minimized.

Show comment Hide comment
@dom-mel

dom-mel Apr 29, 2012

Collaborator

I've noticed the PMD5 hang on my Server and laptop. (Debian/Ubuntu - PHP 5.3.3-7+squeeze8/PHP5.3.--). On my Windows desktop it runs within seconds.

On laptop i tracked it down to [1] caused by a very high value in $iter.
[1] https://github.com/splitbrain/dokuwiki/blob/master/inc/PassHash.class.php#L339

To the CPU-Flag, my Windows desktop has AES-IN, laptop hasn't, server i don't know ;-)

@splitbrain if you want to try it yourself, i can give you access to my server.

Collaborator

dom-mel commented Apr 29, 2012

I've noticed the PMD5 hang on my Server and laptop. (Debian/Ubuntu - PHP 5.3.3-7+squeeze8/PHP5.3.--). On my Windows desktop it runs within seconds.

On laptop i tracked it down to [1] caused by a very high value in $iter.
[1] https://github.com/splitbrain/dokuwiki/blob/master/inc/PassHash.class.php#L339

To the CPU-Flag, my Windows desktop has AES-IN, laptop hasn't, server i don't know ;-)

@splitbrain if you want to try it yourself, i can give you access to my server.

@splitbrain

This comment has been minimized.

Show comment Hide comment
@splitbrain

splitbrain Apr 29, 2012

Owner

Hmm, could this be a 64bit issue? Would be the bitshift in line 333 be affected by 64 vs. 32 bit?

Owner

splitbrain commented Apr 29, 2012

Hmm, could this be a 64bit issue? Would be the bitshift in line 333 be affected by 64 vs. 32 bit?

@dom-mel

This comment has been minimized.

Show comment Hide comment
@dom-mel

dom-mel Apr 29, 2012

Collaborator

nope, all my machines are x64

Collaborator

dom-mel commented Apr 29, 2012

nope, all my machines are x64

@michitux

This comment has been minimized.

Show comment Hide comment
@michitux

michitux Apr 29, 2012

Collaborator

I have the same problem here, debugging shows:

$salt (from parameter): abcdefgh12345678912345678912345678
$iterc ($salt[0]): a
$iter(strpos($itoa64,$iterc)): 38
$iter(1 << $iter) 274877906944

So I think this could be a 64bit issue, as on 32 bit systems I would expect an integer overflow in the calculation of $iter. Is that overflow intended, i.e. should we "simulate" that overflow on 64bit systems?

[edit]
Even though the PHP documentation says that integers are automatically converted to floats when the integer range is exceeded, that doesn't happen for bitshifts, some examples from my 64bit system here:

php > echo 1 << 62;
4611686018427387904
php > echo 1 << 63;
-9223372036854775808
php > echo 1 << 64;
1
php > echo 1 << 65;
2

On a 32bit VM I have the same effects around 32.

Collaborator

michitux commented Apr 29, 2012

I have the same problem here, debugging shows:

$salt (from parameter): abcdefgh12345678912345678912345678
$iterc ($salt[0]): a
$iter(strpos($itoa64,$iterc)): 38
$iter(1 << $iter) 274877906944

So I think this could be a 64bit issue, as on 32 bit systems I would expect an integer overflow in the calculation of $iter. Is that overflow intended, i.e. should we "simulate" that overflow on 64bit systems?

[edit]
Even though the PHP documentation says that integers are automatically converted to floats when the integer range is exceeded, that doesn't happen for bitshifts, some examples from my 64bit system here:

php > echo 1 << 62;
4611686018427387904
php > echo 1 << 63;
-9223372036854775808
php > echo 1 << 64;
1
php > echo 1 << 65;
2

On a 32bit VM I have the same effects around 32.

@sarnowski

This comment has been minimized.

Show comment Hide comment
@sarnowski

sarnowski Apr 29, 2012

Contributor

I got the exact same results here. Initial $iter is 32 and will be shifted to 274877906944. 274 billion md5 iterations in php will take a really long time.

Contributor

sarnowski commented Apr 29, 2012

I got the exact same results here. Initial $iter is 32 and will be shifted to 274877906944. 274 billion md5 iterations in php will take a really long time.

@michitux

This comment has been minimized.

Show comment Hide comment
@michitux

michitux Apr 29, 2012

Collaborator

When I replace $iter by ($iter%32) in the bitshift, all tests pass for me, but I still think this isn't right. The first byte of the salt should be the iteration count and there should imho be some verification that this is a sane number, i.e. not larger than something like 12 (default is 8). I think for our test cases the problem is that the salt that is used for pmd5 has the wrong format, i.e. is not prefixed by a sane iteration count as the one that is generated by pmd5.

Collaborator

michitux commented Apr 29, 2012

When I replace $iter by ($iter%32) in the bitshift, all tests pass for me, but I still think this isn't right. The first byte of the salt should be the iteration count and there should imho be some verification that this is a sane number, i.e. not larger than something like 12 (default is 8). I think for our test cases the problem is that the salt that is used for pmd5 has the wrong format, i.e. is not prefixed by a sane iteration count as the one that is generated by pmd5.

splitbrain added some commits May 1, 2012

avoid integer overflow in PassHash::pmd5 method
Input iteration counts are squared in the function and passing something
above 30 is giving integer overflows on 32 bit systems (and causes insane
iteration counts on 64bit systems).
@splitbrain

This comment has been minimized.

Show comment Hide comment
@splitbrain

splitbrain May 1, 2012

Owner

I checked the original code. They had a check to abort on iteration counts > 30. The bitshift will square the input iteration count and anything above 30 doesn't make much sense anyway. Hell, more than 15 is probably overkill already.

The pushed changes should fix this.

Owner

splitbrain commented May 1, 2012

I checked the original code. They had a check to abort on iteration counts > 30. The bitshift will square the input iteration count and anything above 30 doesn't make much sense anyway. Hell, more than 15 is probably overkill already.

The pushed changes should fix this.

@splitbrain

This comment has been minimized.

Show comment Hide comment
@splitbrain

splitbrain May 1, 2012

Owner

And because nobody objected the idea in general, I'll just go ahead and merge this now.

Owner

splitbrain commented May 1, 2012

And because nobody objected the idea in general, I'll just go ahead and merge this now.

splitbrain added a commit that referenced this pull request May 1, 2012

Merge pull request #96 from dom-mel/phpunit
Replace SimpleTest with PHPUnit

@splitbrain splitbrain merged commit 3c0d44f into splitbrain:master May 1, 2012

@selfthinker

This comment has been minimized.

Show comment Hide comment
@selfthinker

selfthinker May 7, 2012

Collaborator

Won't this get overwritten when upgrading DokuWiki? Shouldn't it go into plugins.required.php instead?
I'm not 100% sure as we all had some problems (i.e. were easily confused) figuring those things out while looking at the (new) extension manager...

Collaborator

selfthinker commented on conf/plugins.php in f9b8008 May 7, 2012

Won't this get overwritten when upgrading DokuWiki? Shouldn't it go into plugins.required.php instead?
I'm not 100% sure as we all had some problems (i.e. were easily confused) figuring those things out while looking at the (new) extension manager...

This comment has been minimized.

Show comment Hide comment
@sarnowski

sarnowski May 8, 2012

Contributor

I can just explain the intension as I'm not familiar with things like the extension manager, maybe @splitbrain should have a look.

To verify that the test suite works correctly, we have a "testing" plugin which adds nothing for normal users. The intension was to disable this plugin by default. There is no problem if someone overrides those in local/... whatever configuration for whatever reason. The whole point was to disable this for new, clean installations.

Contributor

sarnowski replied May 8, 2012

I can just explain the intension as I'm not familiar with things like the extension manager, maybe @splitbrain should have a look.

To verify that the test suite works correctly, we have a "testing" plugin which adds nothing for normal users. The intension was to disable this plugin by default. There is no problem if someone overrides those in local/... whatever configuration for whatever reason. The whole point was to disable this for new, clean installations.

@splitbrain

This comment has been minimized.

Show comment Hide comment
@splitbrain

splitbrain May 8, 2012

Owner

I thinks it's correct this way. Having it in plugins.required.php would make it impossible to change the state of this plugin through the plugin manager (which you might want to do for... well, I don't know ;-)).

Owner

splitbrain commented on f9b8008 May 8, 2012

I thinks it's correct this way. Having it in plugins.required.php would make it impossible to change the state of this plugin through the plugin manager (which you might want to do for... well, I don't know ;-)).

This comment has been minimized.

Show comment Hide comment
@sarnowski

sarnowski May 8, 2012

Contributor

The manager must be able to change its state to activate it for the corresponding test cases. (see _test/tests/test/plugins.test.php)

Contributor

sarnowski replied May 8, 2012

The manager must be able to change its state to activate it for the corresponding test cases. (see _test/tests/test/plugins.test.php)

This comment has been minimized.

Show comment Hide comment
@selfthinker

selfthinker May 9, 2012

Collaborator

Yes, I know what it's supposed to do (and it works). I forgot which are the local files and which are the core files. I remember that we had plugins.php earlier and that we removed it again because it didn't make any sense. But unfortunately I don't remember much more. If that is a file a user would change, it mustn't be in the repository, because an update would overwrite what a user put in there. (So, the other way around, not a user overwriting the file, but us overwriting the user's configuration.) Do we have some documentation for those plugin config files?

Even if this is not a local file (which I'm still not sure about), I still think it's better suited in plugins.required.php. To "make it impossible to change the state of this plugin" is the whole point of disabling this plugin, right? Why else is there a big fat red warning whenever you have enabled it? I think this is a perfect case for something a normal user should never be allowed to change (therefore it should go into plugins.required.php), but only an admin should do it only if there is a good reason for it (like the rest of the required plugins).

Collaborator

selfthinker replied May 9, 2012

Yes, I know what it's supposed to do (and it works). I forgot which are the local files and which are the core files. I remember that we had plugins.php earlier and that we removed it again because it didn't make any sense. But unfortunately I don't remember much more. If that is a file a user would change, it mustn't be in the repository, because an update would overwrite what a user put in there. (So, the other way around, not a user overwriting the file, but us overwriting the user's configuration.) Do we have some documentation for those plugin config files?

Even if this is not a local file (which I'm still not sure about), I still think it's better suited in plugins.required.php. To "make it impossible to change the state of this plugin" is the whole point of disabling this plugin, right? Why else is there a big fat red warning whenever you have enabled it? I think this is a perfect case for something a normal user should never be allowed to change (therefore it should go into plugins.required.php), but only an admin should do it only if there is a good reason for it (like the rest of the required plugins).

This comment has been minimized.

Show comment Hide comment
@selfthinker

selfthinker May 9, 2012

Collaborator

Here is the documention: http://www.dokuwiki.org/config#enabling_disabling_plugins which is missing the "default" file because @splitbrain added it just a short while ago here: 23725b9
I do remember we have decided to not have a default plugin config file. But as I said, unfortunately I don't remember the reason anymore. Maybe @HakanS, @michitux or @PiyushMishra will remember?

Collaborator

selfthinker replied May 9, 2012

Here is the documention: http://www.dokuwiki.org/config#enabling_disabling_plugins which is missing the "default" file because @splitbrain added it just a short while ago here: 23725b9
I do remember we have decided to not have a default plugin config file. But as I said, unfortunately I don't remember the reason anymore. Maybe @HakanS, @michitux or @PiyushMishra will remember?

This comment has been minimized.

Show comment Hide comment
@splitbrain

splitbrain May 9, 2012

Owner

We did not have a default file because all plugins are enabled by default ad we had no idea why we would ship a plugin that would be disabled by default.

Owner

splitbrain replied May 9, 2012

We did not have a default file because all plugins are enabled by default ad we had no idea why we would ship a plugin that would be disabled by default.

This comment has been minimized.

Show comment Hide comment
@sarnowski

sarnowski May 9, 2012

Contributor

until now :-p

Contributor

sarnowski replied May 9, 2012

until now :-p

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