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 <satyamz...@gmail.com>
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.

--
Thanking You,
Darshit Shah

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.

pgpPmL2eIvzHu.pgp
Description: PGP signature