Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117013
MIME-Version: 1.0
References: <CANzZr6kvQP26317=mnHbP49L+y6_PQp=d+uG=dKQTMe4JF+k5g@mail.gmail.com>
 <CAF+90c8fPuMw+s1y_RGr6DjiDvch2LKFwJiviJctaBMo29jKuQ@mail.gmail.com> <CAF+90c80VSPyjLhCM03=gScw8V5yfBtsMnFTH8VEByTmQQT-cg@mail.gmail.com>
In-Reply-To: <CAF+90c80VSPyjLhCM03=gScw8V5yfBtsMnFTH8VEByTmQQT-cg@mail.gmail.com>
Date: Fri, 11 Feb 2022 10:26:29 -0500
Message-ID: <CANzZr6nP4Y671BfH8sdt4ZxFMBqQZXR3iNMkCc90qwzHtC6uyA@mail.gmail.com>
To: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000871da305d7bfb143"
Subject: Re: [PHP-DEV] run-tests SKIPIF caching can be problematic for
 third-party extensions
From: jmikola@gmail.com (Jeremy Mikola)

--000000000000871da305d7bfb143
Content-Type: text/plain; charset="UTF-8"

The reply below is from Sept 24, 2021. I just realized that I inadvertently
responded to Nikita dierctly and never shared to the mailing list.  Doing
so now to close the loop on some open questions and provide more context
for a PR I recently opened (https://github.com/php/php-src/pull/8076).

Since this last message, I'll note that a mechanism to disable SKIPIF
caching entirely was ultimately merged in
https://github.com/php/php-src/commit/9fc6be297fc775c659d5e04b7a4a591bc8577f8a
.

----

I'm not sure I understand the setup you're using. The "nocache" tag that
> mysqli uses covers the situation where part of the test setup is done in
> the SKIPIF section -- because checking whether the test should be skipped
> requires doing the same setup as the actual test. If the test does get
> skipped, it's still fine to cache that.
>
> Now, it sounds like your situation is different from that, and you
> actually have SKIPIF sections where first one test should be skipped, but
> then a later test with identical SKIPIF shouldn't be skipped. Is that
> correct? What changes between the time these two tests run?
>

To avoid code duplication across SKIPIF blocks, we have several helper
functions (
https://github.com/mongodb/mongo-php-driver/blob/master/tests/utils/skipif.php).
In addition to environmental checks (e.g. server, PHP version), we have one
function that ensures the collection (table equivalent) under test is
empty. Based on the mysqli tests I saw, those SKIPIF blocks are quite large
and contain literal references to the schema, which leads to them being
cached independently. In our case, the functions called from our SKIPIF
blocks often rely on default values for arguments (e.g. COLLECTION_NAME).
The definitions for these constants are derived from a filename (
https://github.com/mongodb/mongo-php-driver/blob/master/tests/utils/basic.inc),
which ensures each test uses a different collection. Unfortunately, this
also means many of our SKIPIF blocks end up having identical code and thus
share the same cache entry.

https://github.com/mongodb/mongo-php-driver/blob/master/tests/cursor/bug0671-001.phpt
is an example of a common SKIPIF block, which ensures that the server is
accessible and that the collection under test is empty (i.e. that we can
successfully drop it).

I think I was able to work around our issue with
https://github.com/mongodb/mongo-php-driver/commit/ce6b11d475fb1ff19afed29bcf5b89c200f82440,
but echoing "nocache\n" after a successful drop -- and only for PHP 8.1+ to
avoid borks on earlier versions); however, this approach only allows us to
opt out of caching for non-skips. Hypothetically, if we encounter an error
dropping the collection for tests/cursor/bug0671-001.phpt and that test is
skipped, it's not desirable for run-tests.php to immediately skip the
subsequent bug0732-001.phpt test simply because the code in its SKIPIF
block is identical. The risk here is that our test suite skips more tests
than expected or are necessary.


> Independently of that question (which is about whether "nocache" is
> sufficient if we ignore cross-version compatibility issue) I do agree that
> an option to disable the skip cache entirely would make sense. Would
> https://github.com/php/php-src/pull/7510 work for you?
>

Being able to completely opt-out of SKIPIF caching seems generally useful.
I'll take a look later today and provide some feedback on the PR. Thanks!

-- 
jeremy mikola

>
On Thu, Sep 23, 2021 at 10:23 AM Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Thu, Sep 23, 2021 at 4:10 PM Nikita Popov <nikita.ppv@gmail.com> wrote:
>
>> On Wed, Sep 15, 2021 at 7:23 PM Jeremy Mikola <jmikola@gmail.com> wrote:
>>
>>> I just discovered that run-tests.php was changed to cache SKIPIF
>>> evaluation
>>> since 8.1.0beta3[1]. I believe ext-mongodb ran into the same issue as
>>> mysqli[2], as we use SKIPIF to check that database contents are clean
>>> going
>>> into a test. The present solution in core is to check SKIPIF output for
>>> "nocache" [3,4] and allow individual tests to opt out of caching;
>>> however,
>>> I'm worried that won't be portable for third-party extensions, where we
>>> test with run-tests.php for each version of PHP that we support.
>>>
>>> Is there still time to consider an alternative solution? If "nocache"
>>> needs
>>> to remain in place for mysqli, perhaps 8.1.0's run-tests.php can be
>>> enhanced to consult an environment variable (for maximum BC and playing
>>> nice with `make test`) that disables caching entirely.
>>>
>>
>> I'm not sure I understand the setup you're using. The "nocache" tag that
>> mysqli uses covers the situation where part of the test setup is done in
>> the SKIPIF section -- because checking whether the test should be skipped
>> requires doing the same setup as the actual test. If the test does get
>> skipped, it's still fine to cache that.
>>
>> Now, it sounds like your situation is different from that, and you
>> actually have SKIPIF sections where first one test should be skipped, but
>> then a later test with identical SKIPIF shouldn't be skipped. Is that
>> correct? What changes between the time these two tests run?
>>
>
> Independently of that question (which is about whether "nocache" is
> sufficient if we ignore cross-version compatibility issue) I do agree that
> an option to disable the skip cache entirely would make sense. Would
> https://github.com/php/php-src/pull/7510 work for you?
>
> Regards,
> Nikita
>


-- 
jeremy mikola

--000000000000871da305d7bfb143--