fix: page.goto should resolve to response for self request (#1391) #1781
Conversation
|
@yujiosaka thank you for the patch! The only thing I don't understand is how come this is related to request interception. Is this just a timing issue? |
|
@aslushnikov If that's the case, I can add another test without request interception. |
It was right! I could repro the same problem even without request interception with this small html. Thus, I added a test for non request interception version. 37f0d67 Thank you for pointing it out! |
test/test.js
Outdated
| @@ -1397,6 +1402,13 @@ describe('Page', function() { | |||
| expect(requests.length).toBe(5); | |||
| expect(requests[2].resourceType()).toBe('document'); | |||
| }); | |||
| it('should work with self requesting page', async({page, server}) => { | |||
There was a problem hiding this comment.
Can you please drop this test? It doesn't cover anything new wrt to another one you've added here.
There was a problem hiding this comment.
Do you think so? I would like to hear more reasons.
With page.setRequestInterception(true) it goes different code path.
I thought that's the reason why we have such tests as redirect chain for both non interception and interception versions.
There was a problem hiding this comment.
With page.setRequestInterception(true) it goes different code path.
@yujiosaka from the point of view of Page class and page.goto method specifically, there's no difference in the codepath. Or am I missing something?
There was a problem hiding this comment.
Or am I missing something?
No! I understand your point, so I'll drop the test :)
… (puppeteer#1781) This patch fixes `page.goto` for websites that re-request document URL with javascript. Fixes puppeteer#1391.
I confirmed that the following URLs will start resolving responses by this fix:
I cannot repro for the following URLs:
But these may have been fixed already like #631 .
Fixes: #1391
@aslushnikov