run-tests SKIPIF caching can be problematic for third-party extensions View as Markdown

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

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