Re: [PHP-DEV] run-tests SKIPIF caching can be problematic for third-party extensions
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 wrote: > On Thu, Sep 23, 2021 at 4:10 PM Nikita Popov wrote: > >> On Wed, Sep 15, 2021 at 7:23 PM Jeremy Mikola 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 SKIPI
Re: [PHP-DEV] run-tests SKIPIF caching can be problematic for third-party extensions
On Thu, Sep 23, 2021 at 4:10 PM Nikita Popov wrote: > On Wed, Sep 15, 2021 at 7:23 PM Jeremy Mikola 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
Re: [PHP-DEV] run-tests SKIPIF caching can be problematic for third-party extensions
On Wed, Sep 15, 2021 at 7:23 PM Jeremy Mikola 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? Regards, Nikita
Re: [PHP-DEV] run-tests SKIPIF caching can be problematic for third-party extensions
On Wed, Sep 15, 2021 at 1:59 PM Sara Golemon wrote: > > If the phpt file outputs "skip nocache" then that will skip without > caching on all versions, won't it? In 8.1 because it has the explicit > 'nocache' provided, and in older versions because they don't cache anyway. > > Am I missing something? > If I'm reading the most recent patch correctly [^1], the SKIPIF result is always cached if the output does not start with "nocache". If the output starts with "nocache" (ignoring whatever may follow), the entire result is replaced with an empty string. Given that, it seems like it's not possible to opt out of caching and skip a test. Perhaps that's by design if the cache is only intended to memoize SKIPIF blocks that do result in a test skip. But the question remains: how do we portably opt out of caching without skipping a test? In PHP 7.4 (and presumably earlier versions), run-tests.php appears to ignore unrecognized SKIPIF output [^2]; however, 8.0 is more strict and will flag unrecognized output as borked [^3]. I think Nikita alluded to that logic in his most recent patch [^1], since the same validation exists in 8.1; however, I don't think backwards compatibility (e.g. running the same test on both 8.0 and 8.1) was being considered there. [^1]: https://github.com/php/php-src/commit/0074a1d4e3a85d0d63118e7a30f4b7ed6da64695 [^2]: https://github.com/php/php-src/blob/php-7.4.23/run-tests.php#L2127 [^3]: https://github.com/php/php-src/blob/php-8.0.10/run-tests.php#L2200 -- jeremy mikola
Re: [PHP-DEV] run-tests SKIPIF caching can be problematic for third-party extensions
On Wed, Sep 15, 2021 at 12:22 PM Jeremy Mikola 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. > > If the phpt file outputs "skip nocache" then that will skip without caching on all versions, won't it? In 8.1 because it has the explicit 'nocache' provided, and in older versions because they don't cache anyway. Am I missing something? -Sara
[PHP-DEV] run-tests SKIPIF caching can be problematic for third-party extensions
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. [1]: https://github.com/php/php-src/pull/6681 [2]: https://github.com/php/php-src/pull/6726 [3]: https://github.com/php/php-src/commit/4d43cbe333690171753e9b8663df93d3762e02a8 [4]: https://github.com/php/php-src/commit/0074a1d4e3a85d0d63118e7a30f4b7ed6da64695 -- jeremy mikola
