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

More tests for align.py #477

Merged
merged 1 commit into from
Mar 30, 2020
Merged

More tests for align.py #477

merged 1 commit into from
Mar 30, 2020

Conversation

groutr
Copy link
Contributor

@groutr groutr commented Mar 25, 2020

Description of proposed changes

Increase test coverage for align.py.

This PR also adds an __init__.py to the tests directory so that the augur doesn't have to be installed to run the tests. This may, or may not, be desired. Open to feedback.

Related issue(s)

Related to #476

@elebow
Copy link
Contributor

elebow commented Mar 26, 2020

I'm not sure it's necessary to have a __init__.py in tests/, regardless of whether augur is available in the path.

I don't have augur in my path (or installed on my system at all), and the tests run correctly through pytest. Installing augur with pip doesn't seem to affect that.

from augur import align

import pytest

cur_dir = pathlib.Path(__file__+'/..').resolve()
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be safe to assume that the current working directory during test execution is the root of the project, so test fixtures can be referred to by simple strings like "test/data/align/test_aligned_sequences.fasta".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I like that better.

@groutr
Copy link
Contributor Author

groutr commented Mar 26, 2020

@elebow I'm going to write it off as a quirk in my original dev environment. I created a new one and can't seem to reproduce the issue I was having.
(see comment below)

Reverting the __init__.py commit.

@groutr
Copy link
Contributor Author

groutr commented Mar 26, 2020

@elebow That comment was premature. Removing __init__.py triggered the issue I was having. Without it, pytest fails with an import error for augur.

~/projects/augur align_tests*
(augur-dev) ❯ pytest tests/
========================================================================= test session starts =========================================================================
platform linux -- Python 3.7.6, pytest-5.4.1, py-1.8.1, pluggy-0.12.0
rootdir: /home/grout/projects/augur
plugins: cov-2.8.1
collected 16 items / 1 error / 15 selected                                                                                                                            

=============================================================================== ERRORS ================================================================================
________________________________________________________________ ERROR collecting tests/test_align.py _________________________________________________________________
ImportError while importing test module '/home/grout/projects/augur/tests/test_align.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/test_align.py:8: in <module>
    from augur import align
E   ModuleNotFoundError: No module named 'augur'

@elebow
Copy link
Contributor

elebow commented Mar 27, 2020

@groutr See https://docs.pytest.org/en/latest/usage.html#calling-pytest-through-python-m-pytest. Pytest behaves differently depending on whether it is invoked as pytest or python -m pytest—The second one automatically adds the current directory to the path.

I personally prefer to use a helper script, like run_tests.sh.

@groutr
Copy link
Contributor Author

groutr commented Mar 27, 2020

@elebow, oops, I didn't see the run_tests.sh. Force of habit had me calling pytests directly.

Copy link
Contributor

@huddlej huddlej left a comment

Choose a reason for hiding this comment

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

Thank you, @groutr! These tests look good to me.

Would you mind rebasing these 5 commits into a single commit with a clear description of the tests you've added? That will make the final merge cleaner on our end.

@groutr
Copy link
Contributor Author

groutr commented Mar 29, 2020

@huddlej, I'll do this first thing Monday morning.

@groutr groutr requested a review from huddlej March 30, 2020 16:09
@huddlej huddlej merged commit 62c91ba into nextstrain:master Mar 30, 2020
@groutr groutr deleted the align_tests branch March 30, 2020 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants