Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: add regression test to check WFs are started properly #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

tests: add regression test to check WFs are started properly #37

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion tests/test_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
import json
import uuid

from mock import MagicMock, PropertyMock
from mock import MagicMock, PropertyMock, patch

import requests_mock
from six.moves.urllib.parse import urlparse
Expand Down Expand Up @@ -146,6 +146,40 @@ def test_tasks(app, db, halt_workflow, sample_records_uri):
assert job.status == JobStatus.ERROR


def test_submit_result_starts_a_workflow(
app,
db,
halt_workflow,
sample_records_uri,
sample_records
):
"""Test tasks."""
job_id = uuid.uuid4().hex # init random value
with app.app_context():
CrawlerJob.create(
job_id=job_id,
spider="Test",
workflow=halt_workflow.__name__,
logs=None,
results=None,
)
db.session.commit()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit is not required, pls remove it.
Also, changing the state of the db in a test is bad practice as it affects other tests.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: if you feel proactive you can try to remove the same statement in all other tests and see it was required for the test to pass. My guess is that in most cases, it is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that changing the DB state is a bad practice. Not cleaning it, instead, it is.
A proper tear-down function is defined in the config.py and run after each test, this ensures the DB consistency for each test. Also, not committing the session won't insert a CrawlerJob in the DB, making these tests fail because they read it from the DB.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure you need to commit, if you are working in the same session. But if you clean up explicitly, it's not too bad.


job = CrawlerJob.get_by_job(job_id)
assert job
assert str(job.status)
assert job.status == JobStatus.PENDING

with patch('inspire_crawler.tasks.start') as mock:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use autospec=True (more info here https://docs.python.org/3/library/unittest.mock.html#autospeccing), I am preparing a PR adding it for all patches in inspire-next.

submit_results(
job_id=job_id,
results_uri=sample_records_uri,
errors=None,
log_file="/foo/bar"
)
mock.apply_async.assert_called()
Copy link

@michamos michamos Apr 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... then you'll discover that assert_called doesn't exit in our version of mock: https://github.com/testing-cabal/mock/blob/master/mock/mock.py#L309-L317.
That seems to be wrong, I looked at the wrong place in that file: https://github.com/testing-cabal/mock/blob/master/mock/mock.py#L893.



def test_submit_results_with_results_data(app, db, halt_workflow,
sample_records_uri, sample_records):
"""Test submit_results passing the data as payload."""
Expand Down