Use adventage of newer PHPUnit sytax #11422
Conversation
keradus
force-pushed
the
fix_phpunit
branch
2 times, most recently
from
09b4762
to
2bc2a80
Compare
Codecov Report
@@ Coverage Diff @@
## master #11422 +/- ##
============================================
+ Coverage 93.11% 93.12% +0.01%
Complexity 13008 13008
============================================
Files 436 436
Lines 33697 33697
============================================
+ Hits 31377 31382 +5
+ Misses 2320 2315 -5
Continue to review full report at Codecov.
|
| * @return void | ||
| */ | ||
| public function testAuthenticateFailReChallenge() | ||
| { | ||
| $this->expectException(\Cake\Network\Exception\UnauthorizedException::class); | ||
| $this->expectExceptionCode(401); |
There was a problem hiding this comment.
Why is this notation an improvement? The annotation style is not deprecated and this type of change has the opportunity to create merge conflicts with the 3.next branch.
There was a problem hiding this comment.
it is best practice and suggested way of expecting expectations since almost 2 years:
https://thephp.cc/news/2016/02/questioning-phpunit-best-practices
(TLDR: code over annotation for better detection&refactoring, method call could be put directly before code that is expected to raise exception, not to whole codeblock, so better control of what is raising the exception)
There was a problem hiding this comment.
while merging into 3.next, you could keep 3.next version of code on each conflict and we could re-apply auto-changes in separated PR for 3.next. that's easy ;)
There was a problem hiding this comment.
method call could be put directly before code that is expected to raise exception
Thats when I would start putting it in code, and that needs to be done manually.
There was a problem hiding this comment.
that part of the benefit need to be achieved manually, yes. yet, keep in mind that a lot of test methods are already single-liners, so it's already achieved for them.
also, having real class usage in code instead of text (annotation) is still benefit.
There was a problem hiding this comment.
Maybe this could be done in/for the 3.next branch instead.
There was a problem hiding this comment.
I sent this PR to default branch, if I shall change target branch, let me know ;)
There was a problem hiding this comment.
I will get this merged into master/3.next over the next few days. Any conflicts will be a one time cost.
There was a problem hiding this comment.
i'm not fully get it. it's already targeting master. shall I change PR target branch to sth ? (then I could solve the conflicts saving your time!)
There was a problem hiding this comment.
What you have now is fine. I can take care of any conflicts that come up when I merge master -> 3.next.
| @@ -418,7 +418,7 @@ public function testPathDoesNotExist() | |||
| ]); | |||
|
|
|||
| Cache::read('Test', 'file_test'); | |||
| $this->assertTrue(file_exists($dir), 'Dir should exist.'); | |||
| $this->assertFileExists($dir, 'Dir should exist.'); | |||
There was a problem hiding this comment.
I would extract these changes as separate PR as they are without discussion.
Brought to you by PHP CS Fixer