Create Checks class#57
Conversation
jjgrainger
added
[Type] Enhancement
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger There are still a few things to address here, left some feedback.
| public function test_run_all_checks_throws_exception_if_not_prepared() { | ||
| $this->expectException( Exception::class ); | ||
|
|
||
| $this->checks->run_all_checks(); | ||
| } | ||
|
|
||
| public function test_run_single_check_throws_exception_if_not_prepared() { | ||
| $this->expectException( Exception::class ); | ||
|
|
||
| $this->checks->run_single_check( 'check' ); | ||
| } |
There was a problem hiding this comment.
Can you also add tests for these two methods when the environment is prepared? Right now we only test the error case, but it's arguably even more important to test the success case.
If you had those tests, it would also have shown the PHP error in the Checks class above.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger A few small things, almost good to go.
| * @throws Exception Thrown when check fails with critical error. | ||
| */ | ||
| public function run_checks( array $checks ) { | ||
| $result = new Check_Result( $this->check_context ); |
There was a problem hiding this comment.
@jjgrainger Do we needs to add use WordPress\Plugin_Check\Checker\Check_Result;?
There was a problem hiding this comment.
@mukeshpanchal27 it's not necessarily a requirement as it shares the same namespace, but I see no harm in explicitly defining this.
There was a problem hiding this comment.
Not really needed, I think we can avoid that in the future. But okay to keep for now.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
Great work, thanks @jjgrainger!
|
@jjgrainger Added comment to add function doc block, other than that LGTM!, Thanks! |
4 participants
Adds the
Checksclassget_checks()method once implemented. Check AC's will be updated to include this step.Closes #12