Improve assertion test failures with bad zend.assertions value

When I have some time I'll compare the tests changed in this patch against the tests changed in #2916440 (which is now marked as related to this) - there's bound to be some overlap, if not a lot of overlap.

I want to get us to a point where we can freely run all tests with assertions on or off. Tests which check assertions will be skipped as seen in the patch for the related issue.

I've also submitted a request to the PHPUnit team to add an annotation that can be used to check zend.assertions (or any other ini setting) before running a test, and skipping the test if the required value isn't set.

EDIT: I've reviewed the patch - there is indeed an almost complete overlap between the patch here and the one in the issue I raised a few months ago.

No need to apologize over the overlap. Indeed, I feel guilty for not being a squeakier wheel on this to get the related patch included sooner. No one's to blame though - there's just so much to do that some things will sometimes go through cracks, but I did do a promote to crit on #2916440 to try to keep it from falling through the cracks again.

I'm not married to either one, and maybe the strict op of !== would be the most sound, but whichever is used should be used consistently.

if (ini_get('zend.assertions') == 0) {

I read the conversation but I did not get why this is also not skipping when zend.assertions = -1
Is the default for the php debian package for example.

I feel the best check would be
if (ini_get('zend.assertions') <= 0) {

I closed #2714595: Improve failures for tests requiring assert() when its not enabled as a duplicate of this one as they appeared to be approaching the same issue, and this one was more recent.

The patch no longer applies, so needs a reroll.

+++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
@@ -343,7 +343,12 @@ public function testTransformRootRelativeUrlsToAbsolute($html, $scheme_and_host,
+    if (ini_get('zend.assertions') <= 0) {
+      $this->markTestSkipped(
+        'Assertions are not enabled. Set zend.assertions to 1 to run this test.'
+      );
+    }
+    elseif (method_exists($this, 'expectException')) {
       $this->expectException(\AssertionError::class);

Can't this use the trait as well?

Re-rolled patch in #15 against 8.9.x

RTBC +1

+++ b/core/tests/Drupal/Tests/AssertHelperTrait.php
@@ -32,4 +32,24 @@ protected static function castSafeStrings($value) {
+    if (ini_get('zend.assertions') <= 0) {
+      $this->markTestSkipped(
+        'Assertions are not enabled. Set zend.assertions to 1 to run this test.'
+      );
+    }

It's not only about this setting it's also about assert.active. If that's set to 0 these test are going to fail too.

Also I'm not sure this is a good solution. Having yet another test helper for people have to learn exists feels the wrong way around.

Also I've got to ask but what is being proved by running core's unit tests in another environment? I'm missing the point and value to the community. Our unit tests are not API, they're not meant to prove any custom code works, and they're not meant to validate an environment.

The sole value of doing this is if you're working on an issue that changes one of these tests and it's failing for this reason for you locally. And for me that suggests that we should add:

    <env name="zend.assertion" value="1"/>
    <env name="assert.active" value="1"/>

to core/phpunit.xml.dist and document that these are required for running the core test suite.

I am facing a similar problem. after the apply #28 patch, test running successfully, thanks

Hi all. Getting back up to speed having spent the last couple years pulled into Java and JavaScript projects.

Before I got pulled away I submitted a pull request to the PHP Unit project which was included 2 years ago that significantly affects this project. As of PHP Unit 8.5 (at least, maybe older) individual tests can be annotated to run only when assertions are turned on, or turned off. This is the format:

/**
 * @requires setting zend.assertions 1
 */

Leveraging this annotation to the tests requiring it is cleaner than applying this patch.

Note also the patch I submitted to the PHP Unit project can be used against any PHP setting. I used display_errors in the unit test of the unit test in PHP Unit since that can be changed at runtime.

EDIT: For some reason @requires setting isn't in PHP Unit's documentation. I need to inquire with Mr. Bergman as to why - but most likely it's an oversight. In any event the implementing code in PHP Unit is present.

Ok, Here's a complete rebuild that leverages the annotation above. It should be possible to backport to 9.0.x if it is also using PHPUnit 8.5 or higher.

I have about 2 years worth of changes to get up to speed on so I didn't consider the ramifications of the code changes. I just took the code as it and applied the required setting annotation to the functions that appeared in the patch without modifying their code. I did carry over the use assert trait change from the patch as it is harmless if not actually needed.

As to the DX of writing runtime assertion checks - the only only time a runtime assertion should be tested by a unit test is if it part of a public API. Modules that aren't creating new API's shouldn't need to have any unit tests against their runtime assertions. So, while it does come up, it shouldn't come up often.

Anyway, I'm trying to get my current employer to use Drupal instead of making yet another proprietary in house CMS. Wish me luck - if I succeed I should have more time for core contribs.

This new implementation is now a task.

1. +++ b/core/tests/Drupal/Tests/UnitTestCase.php
   @@ -20,6 +20,7 @@
   +  use AssertHelperTrait;
   

   This is not needed - doesn't exist anymore.

2. The patch should also include

   +++ b/core/phpunit.xml.dist
   @@ -17,6 +17,8 @@
   +    <!-- Do our best to make sure assertions behave as expected. We can't change zend.assertions though. -->
   +    <ini name="assert.active" value="1"/>
   

   In the core/phpunit.xml.dist

3. RE the assert helper - we need less helpers not more. It's more for people to learn. PHPUnit changes fast so this increases maintenance burden. PHPUnit moved away for a single method for asserting the exception class and message for whatever reason. Adding it back in Drupal and only for a special case feels like a step in wrong direction.

Rather than have assert.active always be on, wouldn't it be better to set assert.active to the value of zend.assertions > 0 ?

This way units can be ran with assert in both modes if that is deemed necessary for any reason in the future.

That's what I mean - assert.active can be set at runtime, zend.assertions can't. So why don't we have our PHPUnit bootstrap lock assert.active to the value of zend.assertions > 0 and be done with it. That way we aren't tracking two very similar and somewhat confusing switches.

(Reminder, the difference between 0 and -1 is that 0 emulates how PHP 5 behaved, running any function call appearing as an argument to an assert statement even if assertions are turned off, which is not how assertions are really supposed to work. -1 skips the line and anything called by the line entirely - it doesn't even get compiled in).

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.