Adding new tests using mozhttpd server #137

Nebelhom · 2013-08-22T20:48:22Z

Hi folks,

This PR is for discussion only. It is just the easiest way of sharing ideas.

I set up two very basic tests for the DirectoryParser and for the ReleaseScraper (-p win32 -v latest).

Note that there is a basic unittest class setting up mozhttpd and terminating it, so we just need to inherit from it.

downloadable_tests is the folder that mimicks the Mozilla Server, so we can just add downloadable content into it.

The things I would like to have your input in is:

Current binary is real and too big. What are alternatives to this?
Is the general set up ok like this or do you see obvious disadvantages?
What are meaningful tests?
Comments on code is always welcome.

Thanks. I'm looking forward to your input :)

Nebelhom · 2013-08-22T21:23:31Z

Note: The changes regarding whitespace were forced upon me by a pep8 pre-commit hook ;)

davehunt · 2013-08-27T09:15:15Z

tests/release_scraper/test_release_scraper.py

+        wdir = '/'.join([server_address, mhttpd.WDIR])
+        scraper = ReleaseScraper(os.getcwd(), 'latest', platform='win32',
+                                 base_url=wdir)
+        scraper.download()


Do we need to perform a download? We of course want to test that the download method works, but I don't think we'll need to do this for every test. Essentially we want to know that given a bunch of files/directories, the correct one is selected by the parser for download. We could even do this without mozhttpd.

hmmm... it's true that we could just have some tests in place that cover all the code of the general scraper.download() method and the rest just tests that the correct path is created...

Let me just think about what tests need to be run to cover all of this.

whimboo · 2013-08-28T17:19:19Z

It's a good start. But to get a better picture about mozhttpd I would like to see how you mimic the ftp.mo server for content, and enhance the DirectoryParser for that.

Nebelhom · 2013-08-29T09:02:31Z

tests/base_scraper/test_base_scraper.py

+# TODO:
+# Tests of base class:
+# - detect_platform
+# - show_matching_builds


I had trouble finding a good method of testing those. Any suggestions are welcome.

What exactly will you test here?

At the time of writing this, I was having looking to get good code coverage.

On review, it does not make much sense to test these two. detect_platform relies on mozinfo, so if there is a mistake it will be found there not in mozdownload and show_matching_builds is just a bunch of print outs, so nothing crucial.

So lets get those lines removed.

Nebelhom · 2013-08-29T09:10:11Z

@whimboo, @davehunt

I think this is enough for the start. The rest we could do in issues once this has all been generally overhauled by you two.

I used coverage to make sure the tests cover as much code as possible. If you are interested, I can send the output file to you. There are still some gaps (I couldn't catch that NotFoundException), but I find it a good start.

Thanks for looking over it!

Nebelhom · 2013-09-02T06:49:02Z

tests/directory_parser/test_directory_parser.py

+                                 version='latest',
+                                 platform='win32',
+                                 base_url=self.wdir)
+        parser1 = DirectoryParser(scraper.path)


In retrospect, I am not sure how clever that is when testing the DirectoryParser. This test will then also be affected by changes to the ReleaseScraper regex, as well.

Maybe I should pass in a regex manually. Any comments on that?

Why are you using the ReleaseScraper here? I don't think you need any scraper class it all. Just prepare a folder with a couple of entries and compare the listing via DirectoryParser with the list you get from the file system.

whimboo · 2013-09-02T22:41:53Z

Even with a lot of comments I think that this is a nice start. Personally I would have reduced it to the DirectoryParser without taking any scraper class in the patch. But it's your choice how deep you want to go here. It makes a follow-up class smaller. Also it gives us an idea about the usage of mozhttpd and if we can slim down the extra mozhttpd testing class. I will check that the next time.

Thanks for the work you already put in here, Johannes!

Nebelhom · 2013-09-03T21:57:31Z

@whimboo

I asked @wlach with regard to some of the mozhttpd questions you had as I didn't know and couldn't find a satisfactory answer in the documentation.

Delay in HTTP Response:

"I think you should be able to delay responses fairly easily by doing a subclass which does a quick sleep when static content is gotten."

Basic auth
"basic authentication should also be possible by parsing the headers inside a get method"

General comment to the above:
"neither of these things is completely trivial... maybe the best place to start looking is inside mozhttpd's source code: https://github.com/mozilla/mozbase/blob/master/mozhttpd/mozhttpd/mozhttpd.py"

So, in a word, I guess, we would have to improvise a little for that, but it should be possible.

whimboo · 2013-09-06T23:48:19Z

Delay in HTTP Response:
"I think you should be able to delay responses fairly easily by doing a subclass which does a quick sleep when static content is gotten."

Basic auth
"basic authentication should also be possible by parsing the headers inside a get method"

I think we should file both as follow-up work. For basic http auth we have issue #141, but for delaying the response we might have to wait once we have time to go into details, unless @wlach can give us further hints. I for myself will not have time the next couple of week. :S

whimboo · 2013-09-06T23:55:16Z

mozdownload/utils.py

+
+    m = hashlib.md5()
+    # rb necessary to run correctly in windows.
+    fopen = open(fpath, "rb")


Please use with so we don't loose the handle in case of an exception.

I did not use with as it only got brought in in python2.6. In 2.5 we would need to import the __future__ module. I remember a case, where a feature that came in with 2.6 caused a regression elsewhere.

Should I still use it?

@davehunt do we care about Python 2.5? I don't. Any tool we have sets Python 2.6 as lower limit.

Yeah, 2.6 should be the lowest we support, IMO.

whimboo · 2013-09-10T13:45:57Z

tests/base_scraper/test_base_scraper.py

+import mozhttpd
+import requests
+
+from mozdownload import *


As I have learned lately that cause issues with pyflake. Just do an ´import mozdownload´.

We should consider adding pyflake to our Travis CI job and pre-commit hooks.

I filed issue #157 for it.

whimboo · 2013-09-10T13:51:14Z

Beside the mentioned changes please do not forget to update the list of manual tests and remove those lines which are covered by the automation now.

Also this PR doesn't apply cleanly anymore at the moment.

test_directory_parser: replaced all Scraper instances with regex or strings test_release_scraper: Tests target attribute utils: added create_md5 function

Nebelhom · 2013-09-10T17:10:30Z

tests/mozhttpd_base_test.py

+
+    def setUp(self):
+        """Starts server that lists all files in the directory"""
+        self.logger = mozlog.getLogger(' ')


@whimboo @davehunt

This is a suggestion on how to solve the question on the print statement. If we need additional info, we just set the level to debug and it will be shown (still not pretty but shown).
I have forgotten to add mozlog to the ./runtests.sh script, so I shall just let it stand here for you guys to decide and if you agree on it, I shall add it, otherwise I will revert to the original again.

Sounds good to me, although we should pass a class name to the logger, otherwise it won't be clearly identifiable from the mozdownload logs entries.

Agreed with Dave. Lets add a name.

Nebelhom · 2013-09-10T17:13:58Z

@whimboo

There are no manual tests to be removed except one for the ReleaseScraper, but I would like to do that with issue #156, if you don't mind.

Also do not review the last commit, unless you agree with the changes highlighted in this comment

Nebelhom · 2013-09-11T15:00:09Z

@davehunt @whimboo

As including mozlog seemed to meet with approval, I updated the patch now. A quick review would be nice. Thanks.

whimboo · 2013-09-11T18:20:20Z

There are no manual tests to be removed except one for the ReleaseScraper, but I would like to do that with issue #156, if you don't mind.

Ok, would work for me then. Just missed a feedback from you about it from an already earlier comment.

whimboo · 2013-09-11T18:30:25Z

mozdownload/utils.py

+
+    m = hashlib.md5()
+    # rb necessary to run correctly in windows.
+    with open(path, "rb") as fopen:


Instead of ´fopen´ please call it ´f´. I could get the impression its a modifed open method. :)

whimboo · 2013-09-11T18:32:12Z

I think we are very close in getting it landed. Just fix the remaining items pointed out. Next time I will do a testrun on my own.

Nebelhom · 2013-09-11T20:16:44Z

@whimboo

I did the one change you suggested, so it should be all set to go. I tested it on my machine in Ubuntu and it ran without problems. Let me know if it throws any errors on yours.

Cheers!

P.S. Sorry about not responding to the manual tests comment. I must have missed it... or forgotten about it (blush)

whimboo · 2013-09-13T14:15:31Z

This has been merged as 2c6a9e6

Nebelhom · 2013-09-13T21:43:30Z

Awesome! :)

davehunt reviewed Aug 27, 2013
View reviewed changes

Nebelhom reviewed Aug 29, 2013
View reviewed changes

Nebelhom reviewed Sep 2, 2013
View reviewed changes

whimboo reviewed Sep 6, 2013
View reviewed changes

Nebelhom mentioned this pull request Sep 9, 2013

Write complete unittests for the ReleaseScraper class #156

Closed

whimboo reviewed Sep 10, 2013
View reviewed changes

Nebelhom added 10 commits September 10, 2013 18:48

Basic mozhttpd functionality

6cebb4d

Basic test for Release Scraper

59d2130

Exchanged exe file mimick

626995a

Tests for base Scraper class

b4e51fb

Tests for DirectoryParser class

42a4545

Completing DirectoryParser code coverage

74177b9

test_base_scraper: made code more concise and always used mozhttpd

2dcf55d

test_directory_parser: replaced all Scraper instances with regex or strings test_release_scraper: Tests target attribute utils: added create_md5 function

Removed test for TimeoutExceptions and minor changes

301d1b6

Cleaning up the code

076a399

Used mozlog and cleaned up code a little more

372bfba

Nebelhom reviewed Sep 10, 2013
View reviewed changes

added class name to mozlogger and added mozlogger to runtests.sh

fd1fabc

whimboo reviewed Sep 11, 2013
View reviewed changes

Changed variable name fopen to f in create_md5 function

f583c31

whimboo closed this Sep 13, 2013

Nebelhom deleted the mozhttpd_test branch September 28, 2013 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new tests using mozhttpd server #137

Adding new tests using mozhttpd server #137

Nebelhom commented Aug 22, 2013

Nebelhom commented Aug 22, 2013

davehunt Aug 27, 2013

Nebelhom Aug 27, 2013

whimboo commented Aug 28, 2013

Nebelhom Aug 29, 2013

whimboo Sep 2, 2013

Nebelhom Sep 3, 2013

whimboo Sep 6, 2013

Nebelhom commented Aug 29, 2013

Nebelhom Sep 2, 2013

whimboo Sep 2, 2013

whimboo commented Sep 2, 2013

Nebelhom commented Sep 3, 2013

whimboo commented Sep 6, 2013

whimboo Sep 6, 2013

Nebelhom Sep 7, 2013

whimboo Sep 10, 2013

davehunt Sep 10, 2013

whimboo Sep 10, 2013

davehunt Sep 10, 2013

whimboo Sep 11, 2013

whimboo commented Sep 10, 2013

Nebelhom Sep 10, 2013

davehunt Sep 10, 2013

whimboo Sep 11, 2013

Nebelhom commented Sep 10, 2013

Nebelhom commented Sep 11, 2013

whimboo commented Sep 11, 2013

whimboo Sep 11, 2013

whimboo commented Sep 11, 2013

Nebelhom commented Sep 11, 2013

whimboo commented Sep 13, 2013

Nebelhom commented Sep 13, 2013

Adding new tests using mozhttpd server #137

Adding new tests using mozhttpd server #137

Conversation

Nebelhom commented Aug 22, 2013

Nebelhom commented Aug 22, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whimboo commented Aug 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nebelhom commented Aug 29, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whimboo commented Sep 2, 2013

Nebelhom commented Sep 3, 2013

whimboo commented Sep 6, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whimboo commented Sep 10, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nebelhom commented Sep 10, 2013

Nebelhom commented Sep 11, 2013

whimboo commented Sep 11, 2013

Choose a reason for hiding this comment

whimboo commented Sep 11, 2013

Nebelhom commented Sep 11, 2013

whimboo commented Sep 13, 2013

Nebelhom commented Sep 13, 2013