Skip to content

Commit

Permalink
Fix request counter assertion
Browse files Browse the repository at this point in the history
  • Loading branch information
@sholladay
sholladay committed Feb 1, 2020
1 parent e1f27f6 commit 2808b0f
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions test/browser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ test('retry with body', withPage, async (t, page) => {
server.put('/test', async (request, response) => {
requestCount++;
await pBody(request);
response.sendStatus(408);
response.sendStatus(502);
});

await page.goto(server.url);
Expand All @@ -269,7 +269,7 @@ test('retry with body', withPage, async (t, page) => {
}).text();
return request.catch(error_ => error_.toString());
}, server.url);
t.is(error, 'HTTPError: Request Timeout');
t.is(error, 'HTTPError: Bad Gateway');
t.is(requestCount, 2);

await server.close();
Expand Down

10 comments on commit 2808b0f

@DanielRuf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should'nt it work with 408 too?

https://github.com/sindresorhus/ky/blob/master/readme.md#retry

@sholladay
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielRuf yes, you would think... and I believe it will work in production, but apparently not in our tests for some reason.

Feel free to keep debugging this. See my comment here: #231 (comment)

@szmarczak
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sholladay What about response.statusMessage = 'Request Timeout';?

@sholladay
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szmarczak I'm not sure I understand your comment. Are you suggesting that I try to switch it back to use a 408 status code (with that message)? I don't see why that would fix it.

@szmarczak
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I haven't looked at the error, thought it wasn't providing the HTTP status message. I'll test it locally.

@szmarczak
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sholladay Locally I get the same error. I'm trying to fix it now.

@sholladay
Copy link
Collaborator Author

@sholladay sholladay commented on 2808b0f Feb 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for trying @szmarczak. It's a hard one... I recommend running Puppeteer in headful mode by launching with headless: false.

@szmarczak
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sholladay Seems like Google Chrome is doing retries on its own: #233

@sholladay
Copy link
Collaborator Author

@sholladay sholladay commented on 2808b0f Feb 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. But it's weird that I couldn't reproduce it in normal Chrome without Puppeteer.

@szmarczak
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried non-Puppeteer yet. Let me see...

Please sign in to comment.