Pytest and vcrpy to improve sh.hubstorage tests #32

vshlapakov · 2016-10-18T14:18:41Z

Please, review.

codecov-io · 2016-10-18T14:25:08Z

Current coverage is 47.87% (diff: 100%)

Merging #32 into master will not change coverage

@@             master        #32   diff @@
==========================================
  Files            14         14          
  Lines          1178       1178          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            564        564          
  Misses          614        614          
  Partials          0          0

Powered by Codecov. Last update fe134f0...29ec8f3

vshlapakov · 2016-10-19T14:14:35Z

@chekunkov All the hubstorage tests were converted to pytest, and works perfectly with/without VCR.py. Last step is to commit all the locally generated cassetes and enable tests for sh.hs back. But these cassetes take a lot of space, about 6.5MB, so after the commit it will be very hard to review the changes itself; so I propose to review the changes, fix and merge it, and add the cassetes & enable the tests in other PR.
PS I've converted the tests to pytest, but almost not refactored it or changed its content - just made sure that tests are passing. We could take a deeper look into it here, or in the other PR.

vshlapakov · 2016-10-26T12:06:00Z

@chekunkov Last changes:

reworked existing fixtures, used vcr decorator to have per-session cassettes
found a culprit of high size of the cassettes (test_project.py:test_samples test), changed it a bit, now the cassette takes 200KB instead of 2+MB
gzip serializer for vcr cassettes
py.test option --update-cassettes to run tests with real services and update cassettes

The listed things helped to reduce cassettes size to ~800KB, wonder if it's ok. Most cassettes are already small enough, but probably we could win some space by rewriting tests with largest cassettes.

220K    cassetes/test_project/test_samples.gz
68K     cassetes/test_system/test_succeed_with_close_reason.gz
64K     cassetes/test_system/test_succeed_without_close_reason.gz
64K     cassetes/test_system/test_scraper_failure.gz
24K     cassetes/test_jobq/test_summary_countstart.gz
12K     cassetes/test_project/test_bulkdata.gz

PS Not all the test modules add new jobs, so initially I added clean-jobs fixture (with remove_all_job->yield->remove_all_jobs) only to certain modules with enabled auto-use (to avoid adding the fixture to each test function header). But it leaded to code duplication (4 same fixtures) and saved only about 100KB, so I dropped it in the last commit.

chekunkov · 2016-10-27T11:12:38Z

tests/hubstorage/conftest.py

+        "--update-cassettes", action="store_true", default=False,
+        help="make tests with real services instead of vcr cassettes")
+
+import shutil


Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants. Imports should be grouped in the following order: standard library imports related third party imports local application/library specific imports You should put a blank line between each group of imports.

Oh, I missed this one, sorry!

chekunkov · 2016-10-27T11:26:49Z

tests/hubstorage/conftest.py

+    with my_vcr.use_cassette(cassette_name):
+        remove_all_jobs(hsproject)
+        yield
+        remove_all_jobs(hsproject)


I think there's no need to remove all jobs after the test case, it's only needed to prepare environment for starting test case

chekunkov · 2016-10-27T11:35:22Z

tests/hubstorage/conftest.py

+        request.function.__name__
+    )
+    with my_vcr.use_cassette(cassette_name):
+        remove_all_jobs(hsproject)


Maybe we can reduce size of the cassettes even more by excluding this setup if we are using cassettes? It's needed only when you run tests against live servers and brings no value while using requests recorded in the cassettes. E.g. we can do this cleanup only if config.option.update_cassettes:

chekunkov · 2016-10-27T11:38:45Z

tests/hubstorage/conftest.py

+def pytest_addoption(parser):
+    parser.addoption(
+        "--update-cassettes", action="store_true", default=False,
+        help="make tests with real services instead of vcr cassettes")


What about adding another option that doesn't drop existing cassettes, just optionally ignores them? E.g. to check tests with real services, but not override existing cassettes without the need

chekunkov · 2016-10-27T11:49:49Z

tests/hubstorage/test_collections.py

+        {'foo': 42},
+        {'_key': []},
+        {'_key': 'large_test', 'value': 'x' * 1024 ** 2},
+    ]


is it possible to use @pytest.mark.parametrize instead?

chekunkov · 2016-10-27T11:56:56Z

Don't forget to add vcrpy to requirements-test.txt

chekunkov · 2016-10-28T11:40:18Z

tests/hubstorage/conftest.py

+        # - before_record returning None means skipping all the requests
+        global my_vcr
+        my_vcr.record_mode = 'all'
+        my_vcr.before_record_request = lambda request: None


chekunkov · 2016-10-28T11:44:11Z

tests/hubstorage/conftest.py

+    collection = get_test_collection(hsproject)
+    clean_collection(collection)
+    yield collection
+    clean_collection(collection)


cleaning after the test is not necessary

chekunkov · 2016-10-28T11:44:41Z

tests/hubstorage/conftest.py

+    remove_all_jobs(hsproject)
+    yield
+    remove_all_jobs(hsproject)
+    unset_testbotgroup(hsproject)


remove_all_jobs and unset_testbotgroup are not necessary after the test

chekunkov · 2016-10-28T11:47:17Z

tests/hubstorage/conftest.py

+    return str(hsproject.ids.spider(TEST_SPIDER_NAME, create=1))
+
+
+@my_vcr.use_cassette()


doubt that you need vcr here... get_test_collection doesn't produce requests, cleanup makes sense only when it's running with real services, like in https://github.com/scrapinghub/python-scrapinghub/pull/32/files#diff-5a347e09fdac148d5d13fac9f567fc37R121

chekunkov · 2016-10-28T11:49:23Z

tests/hubstorage/conftest.py

+@pytest.fixture(autouse=True, scope='session')
+def setup_session(hsclient, hsproject, hscollection):
+    set_testbotgroup(hsproject)
+    remove_all_jobs(hsproject)


do set_testbotgroup and remove_all_jobs make sense when running from vcrpy cassettes? if not - good candidate for vcr removal and condition like in https://github.com/scrapinghub/python-scrapinghub/pull/32/files#diff-5a347e09fdac148d5d13fac9f567fc37R121

chekunkov · 2016-10-28T11:52:41Z

tests/hubstorage/conftest.py

+@my_vcr.use_cassette()
+@pytest.fixture(scope='session')
+def hsspiderid(hsproject):
+    return str(hsproject.ids.spider(TEST_SPIDER_NAME, create=1))


same as https://github.com/scrapinghub/python-scrapinghub/pull/32/files#r85515796

ok, after some thinking - let's keep it, minor thing and we cannot guarantee the same spider id in dev environment

chekunkov · 2016-10-28T12:01:35Z

tests/hubstorage/conftest.py

+    jobq = hsproject.jobq
+    for queuename in ('pending', 'running', 'finished'):
+        info = {'summary': [None]}  # poor-guy do...while
+        while info['summary']:


btw, this is not Pythonic :P

while True: info = jobq.summary(queuename) if not info['summary']: break # ...

Oh, sure! I just moved the function as-is and didn't notice it, thanks

chekunkov · 2016-10-28T16:45:57Z

tests/hubstorage/conftest.py

+        help="test with real services rewriting existing vcr cassettes")
+    parser.addoption(
+        "--ignore-cassettes", action="store_true", default=False,
+        help="test with real services skipping existing vcr cassettes")


btw, this has to be added to the tests/conftest.py, otherwise it doesn't work with pytest tests

…on starts

otherwise `pytest --ignore-cassettes tests` doesn't work

vshlapakov self-assigned this Oct 18, 2016

vshlapakov force-pushed the sc786 branch 2 times, most recently from b002127 to c058106 Compare October 19, 2016 12:46

vshlapakov added 10 commits October 19, 2016 17:04

Initial tests for sh.hs.jobq

14510ec

Add tests for sh.hs.project

e23ebf0

Add tests for sh.hs.activity/batchuploader

856ddfb

Add tests for sh.hs.client/collections

6367cc8

Add tests for sh.hs.frontier/jobsmeta

4541ecd

Add tests for sh.hs.system

0d76f36

Add tests for sh.hs.retry

b1e5239

Use default vcrpy requests matching

bbd4687

Clean conftest for sh.hs tests

63f0b8c

Minor PEP8 fixes

0602bba

vshlapakov force-pushed the sc786 branch from 281a879 to 0602bba Compare October 19, 2016 14:05

OlgaCh assigned chekunkov Oct 19, 2016

vshlapakov force-pushed the sc786 branch from 6371fb3 to 2a7910f Compare October 26, 2016 11:34

vshlapakov changed the title ~~[WIP] Pytest and vcrpy to improve sh.hubstorage tests~~ Pytest and vcrpy to improve sh.hubstorage tests Oct 26, 2016

chekunkov suggested changes Oct 27, 2016

View reviewed changes

chekunkov force-pushed the sc786 branch from 4adfeb4 to 81d982c Compare October 28, 2016 11:17

chekunkov suggested changes Oct 28, 2016

View reviewed changes

chekunkov reviewed Oct 28, 2016

View reviewed changes

chekunkov force-pushed the sc786 branch from 253db75 to 29ec8f3 Compare October 28, 2016 22:49

chekunkov approved these changes Oct 28, 2016

View reviewed changes

Cassette for session setup, clean test bg logic

332454f

vshlapakov and others added 7 commits October 31, 2016 10:51

Enable cassettes with gzip serialization

4a842dc

Add pytest option to update cassettes

5f3bebe

Clean changes, improve option to update cassettes

9e29f62

fix test_connection_init_fail_wo_apikey test - cleanup environment

d6d98d2

Rm some redundant clean procedures, fix tests

fc28d8a

fix test failure when test collection doesn't exist before test sessi…

59fe99c

…on starts

move test options configuration to tests root

0d19b04

otherwise `pytest --ignore-cassettes tests` doesn't work

vshlapakov force-pushed the sc786 branch from 29ec8f3 to 0d19b04 Compare October 31, 2016 07:52

vshlapakov merged commit 8185341 into master Oct 31, 2016

vshlapakov deleted the sc786 branch October 31, 2016 08:00

		return str(hsproject.ids.spider(TEST_SPIDER_NAME, create=1))


		@my_vcr.use_cassette()

Pytest and vcrpy to improve sh.hubstorage tests #32

Pytest and vcrpy to improve sh.hubstorage tests #32

Uh oh!

Conversation

vshlapakov commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 47.87% (diff: 100%)

Uh oh!

vshlapakov commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshlapakov commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chekunkov commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chekunkov Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chekunkov Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vshlapakov commented Oct 18, 2016 •

edited

Loading

codecov-io commented Oct 18, 2016 •

edited

Loading

vshlapakov commented Oct 19, 2016 •

edited

Loading

vshlapakov commented Oct 26, 2016 •

edited

Loading

chekunkov Oct 28, 2016 •

edited

Loading

chekunkov Oct 28, 2016 •

edited

Loading