tests: adds WSP functional test for local package #107

spirosdelviniotis · 2017-05-03T15:50:39Z

Adds WSP functional test for local package path.
Re-factored existing WSP functional test (add setup-teardown fixtures).
Re-factored utils.ftp_list_files to utils.list_files for re-usability.
Fixed WSP local package crawling mechanism.

Closes #106

Signed-off-by: Spiros Delviniotis [email protected]

david-caro

Nice

david-caro · 2017-05-04T15:39:19Z

hepcrawl/spiders/wsp_spider.py

@@ -89,16 +89,24 @@ def __init__(self, package_path=None, ftp_folder="WSP", ftp_host=None, ftp_netrc
    def start_requests(self):
        """List selected folder on remote FTP and yield new zip files."""
        if self.package_path:
-            yield Request(self.package_path, callback=self.handle_package_file)
+            dummy, new_files = local_list_files(


In python, for non-used variables like this dummy it's 'convention' to use just _.

david-caro · 2017-05-04T15:41:19Z

hepcrawl/spiders/wsp_spider.py

+                self.target_folder
+            )
+
+            for _file in new_files:


Instead of _file and new_files, it's better to use file_path and new_files_paths so you know it's a string representing the file path, and not a file object (that is usually clear when you write the code yourself, but for someone else it might become very confusing when debugging).

david-caro · 2017-05-04T15:42:44Z

hepcrawl/utils.py

+
+
+def local_list_files(local_folder, target_folder):
+    """List files from given package folder to target folder."""


I don't understand what this function is supposed to do by the comment, can you rephrase? What does it mean that the files 'are listed from a dir to another dir'?

I know it was not clear before either, but better not add more confusion ;)

david-caro · 2017-05-04T15:46:50Z

hepcrawl/utils.py

+    return list_files(local_folder, target_folder, files)
+
+
+def list_files(remote_folder, target_folder, files):


Maybe this should be called list_missing_files instead?
The files parameter would be better called file_names.
It feels to me that this is doing too many things at the same time... can you think of a way to split getting the list of missing files, with adding the paths to the files? (so there's no need to return both all_files and missing_files, making it easier to understand, test and use)

Actually, the all_files parameter does not seem to be used anywhere... we can remove it, then there's no need for a dummy variable anymore.

david-caro · 2017-05-04T15:53:40Z

tests/functional/WSP/test_wsp.py

+    cleaner(package_location + 'IDAQPv20i01-03160015-1510863')
+
+
+def cleaner(path='/tmp/WSP/'):


This is a bad name for a function, cleaner gives the impression to be an object, that you call on (it's an entity that has meaning by itself) while this is actually a function, maybe rename to something like clean_dir?

david-caro · 2017-05-04T15:57:47Z

Can you also try to separate them in individual commits? (it's a bit painful to do now, so next time try to create a lot of commits instead, that can be squashed easily later ;) )

david-caro · 2017-05-04T16:03:00Z

btw. ran locally the functional tests with this pr, and got error (checking if it's my issue):

____________________________________________________ test_wsp_ftp _____________________________________________________

set_up_ftp_environment = {'CRAWLER_ARGUMENTS': {'ftp_host': 'ftp_server', 'ftp_netrc': '/code/tests/functional/WSP/fixtures/ftp_server/.netrc'}, 'CRAWLER_HOST_URL': 'http://scrapyd:6800', 'CRAWLER_PROJECT': 'hepcrawl'}
expected_results = [{'abstracts': [{'source': 'WSP', 'value': 'Abstract L\xe9vy bla-bla bla blaaa blaa bla blaaa blaa, bla blaaa blaa. Bla b..., City, City_code 123456, C. R. Country_1'}], 'full_name': 'author_surname_2, author_name_2'}], 'citeable': True, ...}]

    def test_wsp_ftp(set_up_ftp_environment, expected_results):
        crawler = get_crawler_instance(set_up_ftp_environment.get('CRAWLER_HOST_URL'))
    
        # The test must wait until the docker environment is up (takes about 5 seconds).
        sleep(5)
    
        results = CeleryMonitor.do_crawl(
            app=app,
            monitor_timeout=5,
            monitor_iter_limit=100,
            crawler_instance=crawler,
            project=set_up_ftp_environment.get('CRAWLER_PROJECT'),
            spider='WSP',
            settings={},
>           **set_up_ftp_environment.get('CRAWLER_ARGUMENTS')
        )

tests/functional/WSP/test_wsp.py:110: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
hepcrawl/testlib/celery_monitor.py:81: in do_crawl
    **crawler_arguments
/hepcrawl_venv/lib/python2.7/site-packages/scrapyd_api/wrapper.py:184: in schedule
    json = self.client.post(url, data=data)
/hepcrawl_venv/lib/python2.7/site-packages/requests/sessions.py:535: in post
    return self.request('POST', url, data=data, json=json, **kwargs)
/hepcrawl_venv/lib/python2.7/site-packages/scrapyd_api/client.py:38: in request
    return self._handle_response(response)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <scrapyd_api.client.Client object at 0x4c685d0>, response = <Response [200]>

    def _handle_response(self, response):
        """
            Handles the response received from Scrapyd.
            """
        if not response.ok:
            raise ScrapydResponseError(
                "Scrapyd returned a {0} error: {1}".format(
                    response.status_code,
                    response.text))
    
        try:
            json = response.json()
        except ValueError:
            raise ScrapydResponseError("Scrapyd returned an invalid JSON "
                                       "response: {0}".format(response.text))
        if json['status'] == 'ok':
            json.pop('status')
            return json
        elif json['status'] == 'error':
>           raise ScrapydResponseError(json['message'])
E           ScrapydResponseError: spider 'WSP' not found

david-caro · 2017-05-10T14:18:38Z

hepcrawl/utils.py

+
+def list_missing_files(remote_folder, target_folder, file_names):
+    missing_files = []
+    for filename in file_names:


use the same style for filename and file_names

david-caro · 2017-05-10T14:19:53Z

tests/functional/WSP/test_wsp.py

    netrc_location = os.path.join(os.path.dirname(
        os.path.realpath(__file__)),
-        'fixtures/ftp_server/.netrc'
+        os.path.join('fixtures', 'ftp_server', '.netrc')
    )


The indent here is messed up

david-caro · 2017-05-10T14:22:33Z

tests/functional/WSP/test_wsp.py

+    }
+
+    clean_dir()
+    clean_dir(package_location + 'IDAQPv20i01-03160015-1510863')


Isn't this missing some os.path.join?

for _, dirs, files in next(os.walk(package_location)): for dir_name in dirs: clean_dir(dir_name) for file_name in files: os.unlink(file_name)

david-caro · 2017-05-10T14:22:41Z

tests/functional/WSP/test_wsp.py

+def set_up_local_environment():
+    package_location = os.path.join(os.path.dirname(
+        os.path.realpath(__file__)),
+        'fixtures/ftp_server/WSP/'


david-caro · 2017-05-10T14:31:28Z

tests/functional/WSP/test_wsp.py

+    )
+
+    assert [override_generated_fields(result) for result in results] == \
+           [override_generated_fields(expected) for expected in expected_results]


Save the lists in temp variables, then just compare them.

david-caro · 2017-05-10T14:31:45Z

tests/functional/WSP/test_wsp.py

        spider='WSP',
        settings={},
-        **set_up_environment.get('CRAWLER_ARGUMENTS')
+        **set_up_local_environment.get('CRAWLER_ARGUMENTS')
    )

    gottern_results = [override_generated_fields(result) for result in results]


s/gottern/gotten/

david-caro · 2017-05-10T16:44:39Z

tests/functional/WSP/test_wsp.py

+        for dir_name in dirs:
+            clean_dir(os.path.join(package_location, dir_name))
+        for file_name in files:
+            if '.zip' not in file_name:


not file_name.endswith('.zip')

david-caro · 2017-05-10T16:45:31Z

tests/functional/WSP/test_wsp.py

+
+
+def remove_generated_files(package_location):
+    for _, dirs, files in os.walk(package_location):


instead of a for, you can just use the first result:

_, dirs, files = next(os.walk(...)) for dir_name in dirs: ...

david-caro · 2017-05-10T16:47:36Z

hepcrawl/utils.py

+
+
+def local_list_files(local_folder, target_folder):
+    """List files from given local path to target folder."""


This comment does not make much sense.

david-caro

Minor thing and we merge right away 👍

david-caro · 2017-05-10T18:52:54Z

hepcrawl/utils.py

-                missing_files.append(source_file)
-            all_files.append(source_file)
-    return all_files, missing_files
+        file_names = host.listdir(host.curdir + '/' + server_folder)


os.path.join

* Adds WSP functional test for local package path. * Refactored existing WSP functional test (add setup-teardown fixtures). * Refactored `utils.ftp_list_files` to `utils.list_files` for reusability. * Fixed WSP local package crawling mechanism. Closes inspirehep#106 Signed-off-by: Spiros Delviniotis <[email protected]>

spirosdelviniotis · 2017-05-11T07:25:44Z

@david-caro Ready! 💃

spirosdelviniotis added Status: WIP and removed Status: WIP labels May 3, 2017

spirosdelviniotis self-assigned this May 3, 2017

david-caro suggested changes May 4, 2017

View reviewed changes

david-caro added Status: WIP and removed Status: Ready for review labels May 4, 2017

spirosdelviniotis force-pushed the hepcrawl_wsp_local_package_test branch 3 times, most recently from 81edfed to 8c9fab2 Compare May 10, 2017 14:05

david-caro suggested changes May 10, 2017

View reviewed changes

spirosdelviniotis force-pushed the hepcrawl_wsp_local_package_test branch from fe57c0c to 3824bc9 Compare May 10, 2017 16:32

david-caro suggested changes May 10, 2017

View reviewed changes

spirosdelviniotis force-pushed the hepcrawl_wsp_local_package_test branch from 3824bc9 to 1303c08 Compare May 10, 2017 17:17

spirosdelviniotis added Status: Ready for review and removed Status: WIP labels May 10, 2017

david-caro suggested changes May 10, 2017

View reviewed changes

spirosdelviniotis force-pushed the hepcrawl_wsp_local_package_test branch from 1303c08 to 94af814 Compare May 11, 2017 07:20

david-caro approved these changes May 11, 2017

View reviewed changes

david-caro merged commit 0c0fdf1 into inspirehep:master May 11, 2017

david-caro removed the Status: Ready for review label May 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: adds WSP functional test for local package #107

tests: adds WSP functional test for local package #107

spirosdelviniotis commented May 3, 2017

david-caro left a comment

david-caro May 4, 2017

david-caro May 4, 2017

david-caro May 4, 2017

david-caro May 4, 2017

david-caro May 4, 2017

david-caro May 4, 2017

david-caro May 4, 2017

david-caro commented May 4, 2017

david-caro commented May 4, 2017

david-caro May 10, 2017

david-caro May 10, 2017

david-caro May 10, 2017

david-caro May 10, 2017

david-caro May 10, 2017

david-caro May 10, 2017

david-caro May 10, 2017

david-caro May 10, 2017

david-caro May 10, 2017

david-caro May 10, 2017

david-caro left a comment

david-caro May 10, 2017

spirosdelviniotis commented May 11, 2017 •

edited

Loading



		def local_list_files(local_folder, target_folder):
		"""List files from given package folder to target folder."""

		return list_files(local_folder, target_folder, files)


		def list_files(remote_folder, target_folder, files):

		cleaner(package_location + 'IDAQPv20i01-03160015-1510863')


		def cleaner(path='/tmp/WSP/'):



		def remove_generated_files(package_location):
		for _, dirs, files in os.walk(package_location):



		def local_list_files(local_folder, target_folder):
		"""List files from given local path to target folder."""

tests: adds WSP functional test for local package #107

tests: adds WSP functional test for local package #107

Conversation

spirosdelviniotis commented May 3, 2017

david-caro left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-caro commented May 4, 2017

david-caro commented May 4, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-caro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spirosdelviniotis commented May 11, 2017 • edited Loading

spirosdelviniotis commented May 11, 2017 •

edited

Loading