added phpunit tests and found a bug #37
Conversation
|
Thanks for finding this bug, but rewritting all tests? Seems too much. I know that some of our specs arent really specs, rather some integration tests for the facades and we do have a plan to improve them, but peronally i wouldnt go that far and replace just all of them with phpunit. |
|
That's what I'm doing right now, but I didn't know that was possible in PHP before writing all these tests. I didn't throw away the specs, they are still there, they simply do the same right now. |
|
@Zae nice! Also according to PSR standards you should use 4 spaces instead of tab for indention as mentioned in Contributing file |
| @@ -21,7 +21,9 @@ | |||
| "symfony/yaml": "~2.3" | |||
| }, | |||
| "require-dev": { | |||
| "phpspec/phpspec": "2.0.*" | |||
| "phpspec/phpspec": "2.0.*", | |||
| "phpunit/phpunit": "^3.0|^4.0|^5.0", | |||
There was a problem hiding this comment.
Because this is only dev dependency I think we can use highest version ^5.0
|
Hey Norbert, I didn't think about the spaces, my company uses most of PSR-2 only with tabs, I'll change it along with the other suggestions you made. Do you want me to squash all the commits into one when i'm done? |
|
I did the changes and squashed all the test commits into one but left the one where I fixed the bug separate. I added namespaces, changed to spaces I changed the dev version requirement of phpunit, to 4.5 minimum, because that version includes prophecy for the first time and ^5.0 only supports >= php 5.6. |
| */ | ||
| public function preciseDifferenceDataProvider() | ||
| { | ||
| return array( |
There was a problem hiding this comment.
Maybe its a good idea to separate each test case per locale?
There was a problem hiding this comment.
I could, I thought it was easier and DRYer this way, simply add a new block to the dataprovider and you're good to go, the test code stays the same.
There was a problem hiding this comment.
Im not totally sure if we should split those test cases. IMO this way is easier for new ppl to add tests for other languages.
|
Did the changes, but github says there are conflicts... Should I rebase on top of master? |
|
Yes, please rebase against master |
|
@Zae thanks! I gonna take it from here, we still need to add tests for languages that we currently support. |
PHPUnit 4.5 is the first version that includes prophecy so we need at least that or the newest version
…e called statically, assuming $this from incompatible context
|
rebase complete! |
First I wanted to change the library to make it possible to call the methods non-statically, after doing so I could not figure out why phpspec was not throwing any errors as it should, so I rewrote all the specs in phpunit.
Then I found out PHP allows you to call static methods non-statically so all my work was for nothing, or so I thought!
phpunit found that the fromRoman method in the Number class was not static and so not allowed to be called statically.
So, this pull request adds a lot of phpunit tests that found a bug that phpspec somehow missed and fixes a very tiny bug!