Re: [Bug-wget] [PATCH] Added wget http test for 503 Service unavailable

[Darshit Shah]

Hi Satyam,

Great work! And thanks for the contribution. However, I have a couple of queries 
about this test which I've mentioned in-line.


> From ce32f9ee17fcd9544a34cf9e3656ee7e10ea289d Mon Sep 17 00:00:00 2001
> From: Satyam Zode <address@hidden>
> Date: Sat, 14 Mar 2015 02:46:26 +0530
> Subject: [PATCH] Added wget http  test for 503 Service unavailable
>
> ---
> testenv/Test-503.py | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
> create mode 100644 testenv/Test-503.py
>
> diff --git a/testenv/Test-503.py b/testenv/Test-503.py
> new file mode 100644
> index 0000000..7f1c3c8
> --- /dev/null
> +++ b/testenv/Test-503.py
> @@ -0,0 +1,60 @@
> +#!/usr/bin/env python3
> +from sys import exit
> +from test.http_test import HTTPTest
> +from misc.wget_file import WgetFile
> +
> +"""
> +    This test ensures that Wget handles a 503 Service Unavailable response
> +    correctly.
> +"""
What exactly is the correct response to a 503 Service Unavailable? As far I 
understand, Wget currently simply treats it like any other fatal error and moves 
on.
> +TEST_NAME = "503 Service Unavailable"
> +############# File Definitions ###############################################
> +File1 = """All happy families are alike;
> +Each unhappy family is unhappy in its own way"""
> +File2 = "Anyone for chocochip cookies?"
> +
> +File1_rules = {
> +    "Response"          : 503
> +}
> +
> +A_File = WgetFile ("File1", File1, rules=File1_rules)
> +B_File = WgetFile ("File2", File2)
> +
Why are the two files required in this test? Is the second file required to 
catch any case?

> +Request_List = [
> +    [
> +        "GET /File1",
> +        "GET /File2",
> +    ]
> +]
> +
> +
> +WGET_OPTIONS = "--tries=2"
How does --tries=2 help us in testing this particular scenario. I would be 
averse to complicating any test un-necessarily. Tests should be minimal so that 
they test only few features as possible.

> +WGET_URLS = [["File1", "File2"]]
> +
> +Files = [[A_File, B_File]]
> +
> +ExpectedReturnCode = 8
Why not simply check that Wget did not attempt to download the same file again, 
returned the correct code and exit?

> +ExpectedDownloadedFiles = [B_File]

I would also like to quote RFC 7231 here about the 503 response code:
>   The 503 (Service Unavailable) status code indicates that the server
>   is currently unable to handle the request due to a temporary overload
>   or scheduled maintenance, which will likely be alleviated after some
>   delay.  The server MAY send a Retry-After header field
>   (Section 7.1.3) to suggest an appropriate amount of time for the
>   client to wait before retrying the request.

This is in response to Tim's questions about Wget's behavior with such 
responses. Based on RFC 7231, I think Wget should parse the Retry-After header 
and sleep for the specified number of seconds before retrying that file. If the 
Retry-After header is not available, we must treat it as a 500 Response which is 
a fatal error and skip attempting that file at all.

@Satyam: I'm mostly fine with your patch, apart from a couple of clarifications 
/ modifications as outlined above. However, if you're willing to, you could 
modify the test to actually send a Retry-After Header and test if Wget waits for 
the specified length of time before retrying. This test should fail and we will 
add it to the XFAIL_TESTS as something that we need to fix in Wget.
If you'd like to move on to something else, that is perfectly alright and we'll 
consider your current patch as valid test case.

--
Thanking You,
Darshit Shah

pgppNOTzJ_rZg.pgp
Description: PGP signature