While building Sunny, I’ve been vaguely checking my code coverage and checking PHPstan and seeing what I’m “forced” to write to get to 100% coverage or to make stan happy. I’ve finally encountered an instance where these tools encourage me to break my patterns: techenby.com/posts/first-... - ThreadSky

techenby.com • 72 days ago

While building Sunny, I’ve been vaguely checking my code coverage and checking PHPstan and seeing what I’m “forced” to write to get to 100% coverage or to make stan happy. I’ve finally encountered an instance where these tools encourage me to break my patterns:

https://techenby.com/posts/first-friction-with-coverage-and-phpstan

Comments

techenby.com•72 days ago

What do you think? Please read through the post and let me know if you think the test I wrote is useless.

jclermont.com•72 days ago

I don’t think it’s useless. Livewire treats a void return as null currently, but that could change in the future. It’s an easy test to write to be explicit about both paths.

jclermont.com•72 days ago

I enjoyed the write up and the thought process to adopting these tools. It’s always good to analyze if a tool is bringing value or just being a cranky boss. Love the critical thinking!

andrewfeeney.au•71 days ago

Ditto to both comments. You could argue about the value of the null tests, but it’s absolutely a valid test case. It’s testing a logical branch which could absolutely fail, not a language feature. If the intention is to be strict about 100% test coverage then this logical path needs testing.

andrewfeeney.au•71 days ago

It’s worth considering that the test case isn’t just protecting you against the current logic, which a cursory glance would tell you is correct, but against future changes. When you have this kind of careful coverage you can refactor knowing that your tests will catch the edge cases.

andrewfeeney.au•71 days ago

Another thing I would personally do here is reverse the conditional and make the null path a guard clause which returns early, with the “happy path” at the bottom of the function at the lowest indentation level. This particular case is fairly trivial but I’ve found this a useful discipline

jwhulette.bsky.social•72 days ago

No need for the if statement just return the call with ? On each piece to check if null. If it is it will just return null.

techenby.com•72 days ago

That's true! I usually hesitate to do that just because I personally find that less readable but it is a good tool for the tool box!

projektgopher.com•72 days ago

I think I half agree and half disagree. I feel like it depends on how your view handles this. Simply testing that it defaults to null or void does feel pointless to me (though good point Joel about how LW might treat void in the future). What I would test is what happens in the view layer if null

techenby.com•72 days ago

Yeah I think testing what happens in the view would be much more helpful! I wonder if that will also count for coverage! 🤞🏼

projektgopher.com•72 days ago

I'm pretty sure that coverage uses xdebug instead of simple stan. My gut says that since it will hit that branch during actual execution that it'll count

techenby.com•72 days ago

You were right! I also needed to change an `if` in the view to use the url instead of the form field's value. But testing if the preview image shows up or not is a much better test IMO than if a value is null.

acairns.co.uk•71 days ago

I think the warning from static analysis helped here.

The return type of `?string` means `null` or `string`. Not returning anything from the method would be a `void` type and give a type error.

techenby.com•71 days ago

I do wish string|void would work. Why do I need to be explicit if not having the return null is valid PHP and works in the browser and tests? Using static analysis and types everywhere is still relatively new to me, and I don't like “return null” in my code 😅

matthewdaly.co.uk•72 days ago

I'm more of a Psalm user, but I tried it in the online version of that.

It showed a similar issue around a missing return, but I was able to resolve it by setting the return type to string|void. I imagine that should work in PHPStan too.

techenby.com•72 days ago

Ooh! I like that a lot better, I'll give that a try :)

techenby.com•72 days ago

I tried this out, and unfortunately, in PHP, void can only be used as a standalone type.

matthewdaly.co.uk•72 days ago

Hmm, that's a shame - I'm surprised that the Psalm online thing didn't catch that as an issue.

Wondered if it's possible to essentially override the return type in a docblock using tool specific return types, but doesn't seem possible. Using docblocks only is an option but not a great one.

Comments

Posting Rules

Reply