Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabled testing of APCU for all PHP versions when running with all extensions enabled #363

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

boenrobot
Copy link

@boenrobot boenrobot commented Aug 10, 2023

Enabled testing of APCU for all PHP versions when running with all extensions enabled;

Removed a few instances of erroneous doesNotPerformAssertions annotations.


With APC(u) enabled, there are two test failures:

1) Zend_Cache_TwoLevelsBackendTest::testSaveOverwritesIfFastIsFull
Failed asserting that false is true.

/home/runner/work/zf1-future/zf1-future/tests/Zend/Cache/TwoLevelsBackendTest.php:124
/home/runner/work/zf1-future/zf1-future/tests/resources/Runner.php:20
/home/runner/work/zf1-future/zf1-future/tests/Zend/AllTests.php:143
/home/runner/work/zf1-future/zf1-future/tests/Zend/AllTests.php:[267](https://github.com/boenrobot/zf1-future/actions/runs/5821371801/job/15783687811#step:14:268)
phpvfscomposer:///home/runner/work/zf1-future/zf1-future/vendor/phpunit/phpunit/phpunit:60

2) Zend_Cache_TwoLevelsBackendTest::testSaveReturnsTrueIfFastIsFullOnFirstSave
Failed asserting that null matches expected 90.

/home/runner/work/zf1-future/zf1-future/tests/Zend/Cache/TwoLevelsBackendTest.php:152
/home/runner/work/zf1-future/zf1-future/tests/resources/Runner.php:20
/home/runner/work/zf1-future/zf1-future/tests/Zend/AllTests.php:143
/home/runner/work/zf1-future/zf1-future/tests/Zend/AllTests.php:267
phpvfscomposer:///home/runner/work/zf1-future/zf1-future/vendor/phpunit/phpunit/phpunit:60

And honestly, I'm not entirely sure what's going on there, as I've never used two level cache. I wonder if I should mark those tests incomplete and we move on? Is this a legit bug that needs fixing? And if it's a legit bug, I'm not sure how to fix it.

@develart-projects develart-projects added the enhancement New feature or request label Aug 10, 2023
@develart-projects
Copy link
Collaborator

Actually merging that with 2 failing tests is not the best practice :)
Anyone to take a look in that?

@develart-projects develart-projects added the invalid This doesn't seem right label Aug 10, 2023
@boenrobot boenrobot marked this pull request as draft August 10, 2023 14:08
@boenrobot boenrobot force-pushed the enable-apcu-testing branch 2 times, most recently from d497b4c to 0045448 Compare August 11, 2023 07:27
@boenrobot
Copy link
Author

boenrobot commented Aug 11, 2023

Some more trial & error seems to give a bit of a hint with the test failures, though I'm still not sure how to fix the code or test (whichever one is actually broken...).

In PHP 8.1 and later, the unserialize() inside the two level cache's load returns false in the testSaveReturnsTrueIfFastIsFullOnFirstSave test, which in turn causes errors about array access on bool (rendering the test "errored"). I added a check for that which, if it fails, will do a remove of the key in both caches and return false in that case. When I do that, the outcome of the test in PHP 8.1 is now like in PHP 7.1 (which is what the results of the above were from).

Further adjusting the behavior of load() to remove only the fast cache on corruption and try the slow in that case (succeeding and potentially repopulating the fast cache if the slow succeeds) fixes the testSaveReturnsTrueIfFastIsFullOnFirstSave test in all PHP versions.

testSaveOverwritesIfFastIsFull remains failing, even with that change in behavior, and I'm still not sure how unserialize() ends up returning NULL in the first place. That should in theory only happen if the data originally serialized was NULL, but all calls to save() seem to save an array.

@boenrobot boenrobot force-pushed the enable-apcu-testing branch 8 times, most recently from e9cef3a to 354070c Compare August 11, 2023 14:58
@develart-projects develart-projects added this to the 1.23.1 milestone Aug 11, 2023
@boenrobot boenrobot force-pushed the enable-apcu-testing branch 4 times, most recently from 8689fe4 to b64a187 Compare August 11, 2023 16:01
@develart-projects develart-projects modified the milestones: 1.23.1, 1.23.2 Aug 15, 2023
@develart-projects
Copy link
Collaborator

Moving to 1.23.2, as we have some PRs to be released ASAP.

@boenrobot boenrobot force-pushed the enable-apcu-testing branch 6 times, most recently from 1fc208a to 5363678 Compare August 15, 2023 15:53
@boenrobot boenrobot force-pushed the enable-apcu-testing branch 15 times, most recently from 84e5a17 to 50d3926 Compare August 17, 2023 08:21
@boenrobot
Copy link
Author

OK, it seems the problem was in the test... The test uses a full mock, without specifying what to do on save and load, leading to the fast backend in the test returning NULL all the time, which in turn means it failed. Using partial mock that explicitly mocks only getFillingPercentage() fixes this.

Should I keep the debug messages in the code I wonder, since they were instrumental in detecting the problem? There is probably a negligible overhead in checking if a debug logger is configured...

@boenrobot boenrobot force-pushed the enable-apcu-testing branch 2 times, most recently from 0d23a31 to 01a80cb Compare August 17, 2023 10:28
In order to accomplish successful passing of tests in all supported PHP versions:
- Removed a few instances of erroneous doesNotPerformAssertions annotations.
- Changed the detection from "apc" to "apcu".
- Added protection for classes unserialization in TwoLevels cache and made it remove the ID from either cache if it was corrupted at rest.
- Fixed the testSaveOverwritesIfFastIsFull() method to only partially mock the fast cache. Specifically, mock only the getFillingPercentage() method, and still really execute the rest, rather than just return NULL, which in turn caused the test to fail.
- Added debug logs in TwoLevels cache, and warnings in File cache on errors.
@boenrobot boenrobot marked this pull request as ready for review August 17, 2023 12:36
@boenrobot boenrobot changed the title [WIP] Enabled testing of APCU for all PHP versions when running with all extensions enabled Enabled testing of APCU for all PHP versions when running with all extensions enabled Aug 17, 2023
@develart-projects
Copy link
Collaborator

@boenrobot question is, if this is somehow interfering with anything. I mean the debug messages. If not, we probably should keep them.

@develart-projects develart-projects added to be released PR exists or in master, but not released yet and removed invalid This doesn't seem right labels Aug 21, 2023
@develart-projects develart-projects modified the milestones: 1.23.2, 1.23.3 Aug 21, 2023
@boenrobot
Copy link
Author

@boenrobot question is, if this is somehow interfering with anything. I mean the debug messages. If not, we probably should keep them.

No. No interference, i.e. no change in behavior.

The only costs are

  1. The neglegible bit of performance penalty of checking if there is a logger, and in the event that there is, the writing of a few debug messages.
  2. Possibly more logging noise if you turn on debug logs or (in the case of the file cache) warning logs.

@develart-projects develart-projects merged commit 310ce0c into Shardj:master Aug 21, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request to be released PR exists or in master, but not released yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants