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

check lca database error text #1415

Closed
ctb opened this issue Mar 28, 2021 · 12 comments · Fixed by #1445
Closed

check lca database error text #1415

ctb opened this issue Mar 28, 2021 · 12 comments · Fixed by #1445

Comments

@ctb
Copy link
Contributor

ctb commented Mar 28, 2021

In src/sourmash/lca/lca_db.py, the LCA_Database.load(...) class method checks to make sure that the filename is a directory, and if it is not, it raises a ValueError with the following string: "'{db_name}' is not a file and cannot be loaded as an LCA database". Currently this string is not checked for in the relevant tests.

Add a check for this string value in tests/test_lca.py functions test_databases_load_fail_on_dir and test_databases_load_fail_on_not_exist by looking at str(exc.value) after the pytest.raises block.

@keyabarve
Copy link
Contributor

@ctb Can I try working on this issue? I'm a little unclear on what exactly needs to be done.

@ctb
Copy link
Contributor Author

ctb commented Mar 30, 2021

go for it!

this is the key instruction:

Add a check for this string value in tests/test_lca.py functions test_databases_load_fail_on_dir and test_databases_load_fail_on_not_exist by looking at str(exc.value) after the pytest.raises block.

For an example of us doing something similar to this, see test_gather_error_no_sigs_traverse() in tests/test_sourmash.py - the assert statements at the bottom are basically what you want to do here, suitably modified.

@ctb
Copy link
Contributor Author

ctb commented Mar 30, 2021

note that you can run just the two tests you are working on by doing py.test tests -k test_databases_load_fail_on_dir.

@keyabarve
Copy link
Contributor

Will try, thanks!

@keyabarve
Copy link
Contributor

@ctb I can't seem to find the functions test_databases_load_fail_on_dir and test_databases_load_fail_on_not_exist in tests/test_lca.py. Do I need to create these functions?

@ctb
Copy link
Contributor Author

ctb commented Apr 1, 2021

argh, I think they're buried in some pull request, actually. I'll go find them and report back :)

@ctb
Copy link
Contributor Author

ctb commented Apr 2, 2021

ahh! they are here, #1406.

if you want to work on this, you can! You will need to start from branch refactor/db_load_multiindex instead of latest, and then set up the pull request into that branch (instead of latest).

Alternatively, you could wait until #1406 is merged :).

apologies for the confusion!

@ctb
Copy link
Contributor Author

ctb commented Apr 2, 2021

whups, well, #1406 is merged - so you should be able to update to latest and see the functions!

@keyabarve
Copy link
Contributor

Alright, thanks!

@keyabarve
Copy link
Contributor

@ctb Could you provide some clarification on what exactly needs to be done?

@ctb
Copy link
Contributor Author

ctb commented Apr 3, 2021

please see this comment and give it a try; once you have an open PR with some changes, I'd be happy to help further.

@keyabarve
Copy link
Contributor

Alright, I'll do that. Thanks!

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

Successfully merging a pull request may close this issue.

2 participants