Refactor stale-while-revalidate code in HttpCache, add a (first?) test for it #22043
Conversation
| * After 10s, the cached response has become stale. Yet, we're still within the "stale_while_revalidate" | ||
| * timeout so we may serve the stale response. | ||
| */ | ||
| sleep(10); |
There was a problem hiding this comment.
Sleeping for 20 seconds in total in this test is unacceptable. Is it possible to sleep for less, or don't sleep at all?
There was a problem hiding this comment.
With PHPUnitBridge clock mocking, time passes instantly.
There was a problem hiding this comment.
Doesn't @group time-sensitive / clock mocking solve this?
There was a problem hiding this comment.
It does indeed. I only checked the method annotations (none). Indeed time-sensitive is set on the test class 👍
|
@nicolas-grekas should I be concerned because of the failing AppVeyor builds? |
|
Thank you @mpdude. |
| $this->assertHttpKernelIsNotCalled(); | ||
| $this->assertEquals(503, $this->response->getStatusCode()); | ||
| $this->assertEquals('Old response', $this->response->getContent()); | ||
| } |
There was a problem hiding this comment.
Dunno why, but this test breaks the HttpKernel test suite on windows (see appveyor https://ci.appveyor.com/project/fabpot/symfony/build/1.0.20167#L841), removing the test fixes it.
This PR was merged into the 3.3-dev branch. Discussion ---------- Fix HttpCache test | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | #22043 (comment) | License | MIT | Doc PR | n/a will make appveyor green. Commits ------- 3178f50 Fix failing HttpCache test on windows
This PR was merged into the 3.3-dev branch. Discussion ---------- Fix HttpCache test | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | symfony/symfony#22043 (comment) | License | MIT | Doc PR | n/a will make appveyor green. Commits ------- 3178f50 Fix failing HttpCache test on windows
I came up with this while trying to hunt a production bug related to handling of stale cache entries under the condition of a busy backend (also see #22033).
It's just a refactoring to make the code more readable plus a new test.