Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PhpUnitBridge] Make ClockMock match namespaces that begin with Tests\\#18726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
teohhanhui commentedMay 9, 2016
| $count =0; | ||
| $ns =preg_replace('/(^|\\\\)Tests\\\\/','$1',$mockedNs[0],1,$count); | ||
| if (1 ===$count) { | ||
| $mockedNs[] =$ns; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
imho we should simply additionally check if the namespace starts withTests\:
if (false !==strpos($class,'\\Tests\\')) {// ...}elseif (0 ===strpos('Tests\\')) {$mockedNs[] =substr($class,6);}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Using regex seems more extensible to me, even though it might not be as readable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd favor readability here, thus@xabbuh proposal
teohhanhui commentedMay 27, 2016
@nicolas-grekas: Addressed@xabbuh's comment. |
| $testNs =substr($class,0,strrpos($class,'\\')); | ||
| $mockedNs =array($testNs); | ||
| if (false !== ($pos =strpos($testNs,'\\Tests\\'))) { | ||
| $mockedNs[] =substr_replace($testNs,'\\',$pos,7); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this changes the semantic a bit (only the first\Tests\ is replaced now). Let's be safe and keep the code asis for this first part...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wouldn't that actually have been a bug? Or was that the intended behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not a bug (nor really planned but we shouldn't change now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
4b00e01 to310eaa5Compareteohhanhui commentedMay 27, 2016
I believe Travis failure is unrelated. |
| $ns =str_replace('\\Tests\\','\\',$class); | ||
| $mockedNs[] =substr($ns,0,strrpos($ns,'\\')); | ||
| }elseif (0 ===strpos($class,'Tests\\')) { | ||
| $mockedNs[] =substr($class,6,strrpos($class,'\\')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Must be:
$mockedNs[] =substr($class,6,strrpos($class,'\\') -6);
teohhanhui commentedMay 30, 2016
Status: Needs Review |
xabbuh commentedMay 30, 2016
👍 Status: Reviewed |
nicolas-grekas commentedMay 30, 2016
feature on 3.2 isn't it? |
teohhanhui commentedMay 30, 2016
If so, then we need to address this issue in symfony-standard, because it does not work as expected out of the box. |
nicolas-grekas commentedMay 30, 2016
there is no deps bewteen the bridge and sfstd, so no need to change, just use the newest bridge to run the tests |
teohhanhui commentedMay 30, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
That means bumping the version of the PhpUnit Bridge in symfony-standard 2.8+? |
nicolas-grekas commentedMay 30, 2016
Thank you@teohhanhui. |
…egin with Tests\\ (teohhanhui)This PR was submitted for the 2.8 branch but it was merged into the 3.2-dev branch instead (closes#18726).Discussion----------[PhpUnitBridge] Make ClockMock match namespaces that begin with Tests\\| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes?| New feature? | no| BC breaks? | no?| Deprecations? | no| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR |symfony/symfony-docs#6546See symfony/symfony-standard#969Commits-------2630c13 [PhpUnitBridge] Make ClockMock match namespaces that begin with Tests\\
Uh oh!
There was an error while loading.Please reload this page.
See symfony/symfony-standard#969