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.

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

Conversation

ammirate
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Mar 15, 2018

Coverage Status

Coverage remained the same at 82.081% when pulling 0032b8b on ammirate:regression_test_crawl_api into 62bb227 on inspirehep:master.

logs=None,
results=None,
)
db.session.commit()

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.

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.

@ammirate ammirate changed the title tests: add test for checking wf are started using new hepcrawl API tests: add regression test to check WFs are started properly Apr 6, 2018
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.

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

@michamos michamos Apr 6, 2018

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.

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants