Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Refactor stale-while-revalidate code in HttpCache, add a (first?) test for it#22043
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sleeping for 20 seconds in total in this test is unacceptable. Is it possible to sleep for less, or don't sleep at all?
javiereguiluzMar 17, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
With PHPUnitBridge clock mocking, time passes instantly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Doesn't@group time-sensitive / clock mocking solve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It does indeed. I only checked the method annotations (none). Indeedtime-sensitive is set on the test class 👍
mpdude commentedMar 21, 2017
@nicolas-grekas should I be concerned because of the failing AppVeyor builds? |
fabpot commentedMar 22, 2017
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Dunno why, but this test breaks the HttpKernel test suite on windows (see appveyorhttps://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/awill 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/awill make appveyor green.Commits-------3178f50 Fix failing HttpCache test on windows
Uh oh!
There was an error while loading.Please reload this page.
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.