[WIP] Focused tests #194
[WIP] Focused tests #194
Conversation
2 similar comments
There was a problem hiding this comment.
Overall looks good! Left a couple comments/questions.
Definitely feeling the growing parameter list in constructors and Context methods, but I think it is appropriate for where we are now. Maybe an item on the punchlist for 2.0 to refactor towards a config object or some other mechanism to not get locked in like that in the future.
I'll have to try it, but I think this approach would work just fine with concurrency too.
| it('should return true even if nested tests are not focused', function() { | ||
| $test = new Test("test", function() {}); | ||
| $this->suite->addTest($test); | ||
| assert($this->suite->getFocused(), "suite should not be focused"); |
There was a problem hiding this comment.
Aren't we asserting here that the suite should be focused?
| */ | ||
| public function getFocused() | ||
| { | ||
| foreach ($this->tests as $test) { |
There was a problem hiding this comment.
What is the reason for crawling tests first? Wouldn't it be more efficient to check $this->focused first? It does require the explicit check for true, but would that be preferable?
if ($this->focused === true) {
return true;
}
// check tests
return false;There was a problem hiding this comment.
Yeah, that's true, probably a worthwhile change.
| * | ||
| * @return bool | ||
| */ | ||
| public function getFocused(); |
There was a problem hiding this comment.
Would this method read better as isFocused?
There was a problem hiding this comment.
Definitely, but that would also be inconsistent with getPending(). In light of that, what do you think I should call it?
There was a problem hiding this comment.
I remember there being some reasoning to getPending() . I think I remember it having something to do with pending being one of three states instead of just true/false.
I think the original thought was that null means a pending state does not exist, where true/false are explicit pending states. Hence Suite.php has the following:
protected function runTest(TestInterface $test, TestResult $result)
{
if ($this->getPending() !== null) {
$test->setPending($this->getPending());
}
$test->setEventEmitter($this->eventEmitter);
$test->run($result);
}Looking at it now, I can't remember why the explicit setting was favored....
There was a problem hiding this comment.
Yeah, that makes some degree of sense. Since the focused state doesn't use null, it can probably be isFocused() then. I'll update that in a sec.
| @@ -20,7 +20,7 @@ function describe($description, callable $fn) | |||
| */ | |||
| function context($description, callable $fn) | |||
| { | |||
| describe($description, $fn); | |||
| Context::getInstance()->addSuite($description, $fn); | |||
There was a problem hiding this comment.
Are we just favoring explicitness here? I always kind of viewed context and all of its forms as just an alias for describe. Why not just have it delegate to the describe function?
There was a problem hiding this comment.
Yeah, I probably shouldn't have changed that here. My logic was "less method calls = more faster", especially for PHP versions < 7. In any case, it belongs in its own PR.
| @@ -111,4 +127,23 @@ protected function runTest(TestInterface $test, TestResult $result) | |||
| $test->setEventEmitter($this->eventEmitter); | |||
| $test->run($result); | |||
| } | |||
|
|
|||
| private function focusedTests() | |||
There was a problem hiding this comment.
Couple thoughts on this:
I don't think I ever published any real coding standards, so I am open to being shot down on this :) - but for this project (and generally in the PHP realm for me) - I tended more towards the get language for things returning something. It is a little more verbose, but I think it fits the language and the rest of the project a little better. Something like getFocusedTests. In JavaScript and other functional languages I use, I like to ditch some of that verbosity. Thoughts?
Second, I'm not sure highlighting focused here is the best solution. It is currently our only need so again I'm probably wrong :). It kind of seems like we are applying a transformation of state to the test tree before we run things, and this seems like something that might fit well into an array_filter type scenario:
// or maybe getTestsToRun?
protected function getFilteredTests()
{
$tests = array_filter($this->tests, function (TestInterface $test) {
return $test->getFocused();
});
return empty($tests) ? $this->tests : $tests;
}Basically, if the state of the things to run has changed we know we can return that changed set. Otherwise return the original. The language might be a bit more flexible if we add similar functionality later on, and it also eliminates the need for the additional $hasFocusedTests variable. Thoughts?
There was a problem hiding this comment.
Yeah, I have no problem with any of that. I tend not to use the "get" prefix in my own stuff, but I know Peridot does tend to use it, so I think I just did it out of habit.
Having a more generic name for the method makes total sense, as does the filter approach. It also becomes more relevant if we change that method from private to protected, as it could then be interpreted as being part of Peridot's API, so having a future-proof name is a good idea.
|
Thanks for the review man, I'll probably be able to get your suggestions in today sometime. |
1 similar comment
|
@ezzatron what is your stance on squashing commits? I tend to lean more towards cleaning up commit messages addressing review feedback, but I am open to changing that mindset if you have a preference Thanks again man! |
|
I tend to squash once all feedback has been addressed. I think I covered all your review points, so I can do that now if that works for you? |
ezzatron
force-pushed
the
focused-specs
branch
from
0ad7e19 to
1e320a1
Compare
1 similar comment
This is a work in progress, to implement #185