Run the test suite with assertions off

[greg0ire]

Q	A
Type	improvement
BC Break	no
Fixed issues	n/a

Summary

Enabling assertions to get more coverage was a bad idea, because these
are supposed to be disabled in production, which means you could have
code working in the CI, and not working in production. We do not want
that.

See #4359

[morozov]

Enabling assertions to get more coverage was a bad idea, because these are supposed to be disabled in production, which means you could have code working in the CI, and not working in production. We do not want that.

If not have assertions on CI and not have them in production, then where are they supposed to be enabled? We didn't enable them to increase code coverage (although this is how we learned that they are disabled), we enabled them to have them executed on CI.

[greg0ire]

You're right I'm attempting to fix the issue with a wrong solution. Maybe the right solution would be to somehow find a way to limit calls to assert() to simple expressions that do not change anything.

[simonwelsh]

If an INI setting can change what code gets run (and the use of assert() at all causes this) then CI should run with that setting on and with it off

[greg0ire]

@simonwelsh I considered it, but if you want to use that to catch the bug we had with PG, then you will have to run it for the Pg job, so following that logic you might end up with twice as many CI jobs, and we already have ~ 50.

[morozov]

@greg0ire I believe this should be solved at the static analysis level. Maybe Psalm implements this at a higher level than 3 which we're currently at, or it could be an improvement request (UPD: vimeo/psalm#4392).

The idea is that if there is an assertion around a non-pure expression, it's a bug. As simple as that.

[greg0ire]

I had the same idea but didn't connect it to the concept of pure expressions. Fully agree, this is the right solution, and I'm glad we live in a time where static analysis in PHP is advanced enough to make this a realistic feature request.