[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-wget] Patch: Ports test for restrict-filename to python
|
From: |
Elita Lobo |
|
Subject: |
Re: [Bug-wget] Patch: Ports test for restrict-filename to python |
|
Date: |
Tue, 17 Mar 2015 00:37:53 +0530 |
Hi,
I have made the changes in my patch as suggested above by Darshit and also
added test files to test 206 failure and -E -k feature of Wget. Kindly let
me know if I have missed anything and need to make any further
modifications.
Thanks and Regards,
Elita Lobo
On Mon, Mar 16, 2015 at 12:17 AM, Darshit Shah <address@hidden> wrote:
> Hi Elita,
>
> I'm putting my comments inline
>
> Subject: [PATCH] Ports test from old test suite to Python *
>> Test-restrict-lowercase.py: Tests --restrict-filename=lowercase feature *
>> Test-restrict-ascii.py: Tests --restrict-filename=ascii feature *
>> Test-restrict-uppercase.py: Tests --restrict-filename=uppercase feature
>> *Test-nonexisting-quiet.py: Tests Wget's quiet feature *
>> Test-start-pos.py:
>> Tests Wget's --start-pos feature * Test-start-pos--continue.py Tests
>> Wget's
>> --start-pos with -c feature * Makefile.am: Adds newly added testfiles to
>> test
>> suite
>>
>> That's one *long* commit message! Could you please take a look at the
> other commit messages we've written and write yours accordingly?
>
> --- /dev/null
>> +++ b/testenv/Test-nonexisting-quiet.py
>> @@ -0,0 +1,48 @@
>> +#!/usr/bin/env python3
>> +from sys import exit
>> +from test.http_test import HTTPTest
>> +from misc.wget_file import WgetFile
>> +
>> +"""
>> + Test for Wget nonexisting quiet. --quiet turns off Wget's output.
>> +"""
>> +TEST_NAME = "Test nonexisting quiet"
>> +################################ File Definitions
>> ############################################################
>> #################
>> +dummyfile = "This is a dummy file"
>> +
>> +A_File = WgetFile("dummy.html", dummyfile)
>> +
>> +WGET_OPTIONS = "--quiet"
>> +WGET_URLS = [["nonexistent"]]
>> +
>> +Files = [[A_File]]
>> +
>> +ExpectedReturnCode = 8
>> +ExpectedDownloadedFiles = []
>> +
>> +################### Pre and Post Test Hooks
>> ###############################
>> +pre_test = {
>> + "ServerFiles" : Files,
>> +}
>> +test_options = {
>> + "WgetCommands" :WGET_OPTIONS,
>> + "Urls" :WGET_URLS
>> +}
>> +post_test = {
>> + "ExpectedFiles" : ExpectedDownloadedFiles,
>> + "ExpectedRetcode" : ExpectedReturnCode
>> +}
>> +
>> +err = HTTPTest (
>> + name=TEST_NAME,
>> + pre_hook=pre_test,
>> + test_params=test_options,
>> + post_hook=post_test
>> +).begin ()
>> +
>> +exit(err)
>> +
>> +
>> +
>> +
>> +
>>
>
> I'm not sure what is the point of this test. A test for non-existent files
> I can understand, but the we;re not really testing the -q option here. How
> would you test for the quiet option?
>
>
> Also, trailing newline characters at the end of the file.
>
>> diff --git a/testenv/Test-restrict-ascii.py
>> b/testenv/Test-restrict-ascii.py
>> new file mode 100644
>> index 0000000..c974f68
>> --- /dev/null
>> +++ b/testenv/Test-restrict-ascii.py
>> @@ -0,0 +1,67 @@
>> +#!/usr/bin/env python3
>> +from sys import exit
>> +from test.http_test import HTTPTest
>> +from misc.wget_file import WgetFile
>> +
>> +"""
>> + This program tests that --restrict-file-names=ascii can be used to
>> + ensure that all high-valued bytes are escaped. The sample filename
>> was
>> + chosen because in former versions of Wget, one could either choose
>> not
>> + to escape any portion of the UTF-8 filename via
>> + --restrict-file-names=nocontrol (which would only be helpful if one
>> + was _on_ a UTF-8 system), or else Wget would escape _portions_ of
>> + characters, leaving irrelevant "latin1"-looking characters combined
>> + with percent-encoded "control" characters, instead of encoding all
>> the
>> + bytes of an entire non-ASCII UTF-8 character.
>> +"""
>> +
>> +TEST_NAME = "Restrict filename:ascii"
>> +
>> +# "gnosis" in UTF-8 greek
>> +gnosis = "%CE%B3%CE%BD%CF%89%CF%83%CE%B9%CF%82"
>> +#################################### File Definitions
>> #########################
>> +
>> +mainpage = """
>> +<html>
>> +<head>
>> + <title>Some Page Title</title>
>> +</head>
>> +<body>
>> + <p>
>> + Some text...
>> + </p>
>> +</body>
>> +</html>
>> +"""
>> +A_File = WgetFile(gnosis + ".html", mainpage)
>> +
>> +WGET_OPTIONS = "--restrict-file-names=ascii"
>> +WGET_URLS = [[gnosis + ".html"]]
>> +
>>
> If we're _always_ using `gnosis + ".html"`, why not add that string to the
> variable directly?
>
>> +Files = [[A_File]]
>> +
>> +ExpectedReturnCode = 0
>> +ExpectedDownloadedFiles = [A_File]
>> +
>> +######################### Pre and Post Test hooks
>> ############################
>> +pre_test = {
>> + "ServerFiles" :Files,
>> +}
>>
> Just nitpicking here, but the alignment is off. It's off in other places
> too, but I'll mention it only here. I realize that this test suite doesn't
> follow PEP8 at all, and I would like to fix that at some point. But till
> then it's best to stick to existing formatting style.
>
> +test_options = {
>> + "WgetCommands" :WGET_OPTIONS,
>> + "Urls" :WGET_URLS
>> +}
>> +post_test = {
>> + "ExpectedFiles" : ExpectedDownloadedFiles,
>> + "ExpectedRetcode" : ExpectedReturnCode
>> +}
>> +
>> +err = HTTPTest (
>> + name=TEST_NAME,
>> + pre_hook=pre_test,
>> + test_params=test_options,
>> + post_hook=post_test
>> +).begin ()
>> +
>> +exit(err)
>> +
>>
> <- snip ->
>
> diff --git a/testenv/Test-start-pos--continue.py
>> b/testenv/Test-start-pos--continue.py
>> new file mode 100644
>> index 0000000..dfc1a38
>> --- /dev/null
>> +++ b/testenv/Test-start-pos--continue.py
>> @@ -0,0 +1,55 @@
>> +#!/usr/bin/env python3
>> +from sys import exit
>> +from test.http_test import HTTPTest
>> +from misc.wget_file import WgetFile
>> +
>> +"""
>> + Tests if Wget correctly starts downloading bytes from the given
>> start position (--start-pos=OFFSET) in the file.
>> +"""
>> +TEST_NAME = "Test start-pos"
>> +################################ File Definitions
>> ############################################################
>> #################
>> +File1 = "12345678910"
>> +File2 = "This is an existing file"
>> +File3 = "2345678910"
>> +
>> +File1_rules = {
>> + "SendHeader" : {
>> + "Response" : 206
>> + } +}
>>
> What is this header needed for?
>
>> +
>> +A_File = WgetFile("somefile.txt", File1, rules=File1_rules)
>> +B_File = WgetFile("somefile.txt", File2)
>> +C_File = WgetFile("somefile.txt.1", File3)
>> +
>> +WGET_OPTIONS = "--start-pos=1 --continue --debug"
>>
> --debug options are always added to Wget. You don't need to specify it
> again.
>
> Could you also add a comment at the top of the file explaining how this
> test also tests that --continue and --start-pos don't work together?
>
> +"""
>> + Tests if Wget correctly starts downloading bytes from the given
>> start position (--start-pos=OFFSET) in the file.
>> +"""
>> +TEST_NAME = "Test start-pos"
>> +################################ File Definitions
>> ############################################################
>> #################
>> +File1 = "12345678910"
>> +File2 = "2345678910"
>> +
>> +File1_rules = {
>> + "SendHeader" : {
>> + "Response" : 206
>> + } +}
>> +
>> +A_File = WgetFile("File1", File1, rules=File1_rules)
>> +B_File = WgetFile("File1", File2)
>> +
>> +WGET_OPTIONS = "--start-pos=1"
>> +WGET_URLS = [["File1"]]
>> +
>> +Files = [[A_File]]
>> +
>> +ExpectedReturnCode = 0
>> +ExpectedDownloadedFiles = [B_File]
>> +
>>
> I think this test is only a subset of the previous one and hence a bit
> redundant. What do you think?
>
>
> Sorry if I come across as being a little too mean, but I'm a bit picky
> when it comes to well formatted commits. Zihang can tell more about that. :)
>
>
>
> --
> Thanking You,
> Darshit Shah
>
0001-testenv-Adds-new-testfiles-to-test-suite-which-tests.patch
Description: Text Data