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

Added inheritance filter to gemini plugin #223

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

moonso
Copy link
Collaborator

@moonso moonso commented Apr 27, 2016

No description provided.

@moonso
Copy link
Collaborator Author

moonso commented Apr 29, 2016

Finally works!! Please review @brainstorm @guillermo-carrasco @robinandeer

@@ -53,6 +53,12 @@ def gemini_path(request):
gemini_db = "tests/fixtures/HapMapFew.db"
return gemini_db

@pytest.fixture(scope='function')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this decorator tell you when that "big" GEMINI db is not there?

Would it make sense to be a bit smarter here and pull the S3 file when and if it's not present while running the testsuite? Could save some time for new devs/beginners before they figure it out when reading the docs...

Automation >> Docs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I added this then changed my mind but forgot to remove. Thanks for noting!
We should have a dedicated test database as issued in #225

@brainstorm
Copy link
Collaborator

Cool stuff Måns! Just went through the PR, seems fine to me except the HapMapMany test/comment I left... It would make sense to have an exception for pulling the data from S3:

When tests are running on TravisCI, do not pull those in.

Other than that, excellent job! ;)

@moonso
Copy link
Collaborator Author

moonso commented May 4, 2016

I have no idea why the tests fails, it's working on my local installation...
Guess it has to do with different gemini versions, can anyone help out @brainstorm @guillermo-carrasco @robinandeer ??

@moonso
Copy link
Collaborator Author

moonso commented May 4, 2016

@robinandeer would you like to have a last review of this?
The tests fails because of this arq5x/gemini#724 (comment), is it possible to get around in some way?
Please merge so I can do a release

 - Add a .coveragec file to control what code is excluded when checking
test coverage.

Modify this file if there is repeated types of code that should be
excluded from coverage testing.

- Remove coverage testing when using gemini.gim since it does not work.
@guillermo-carrasco
Copy link
Contributor

hmm I don't even get what's the problem exactly, the stacktrace is confusing :-/

@moonso
Copy link
Collaborator Author

moonso commented May 9, 2016

@guillermo-carrasco think that the problem has to do with gemini.gim uses eval, that in combination with pytest cov make some wierd things happen

@robinandeer
Copy link
Owner

robinandeer commented May 9, 2016

eval...

captain-picard-meme-dafuq

@robinandeer
Copy link
Owner

How are we standing on this?

Doesn't GEMINI do a test for this case or are they not using py.test?

I don't like the idea of merging in code that will make tests fail just bc having a "broken master branch" tag in the README is not a good thing for potential collaborators etc. to see

@moonso @brainstorm @guillermo-carrasco @henrikstranneheim ??

@brainstorm
Copy link
Collaborator

I had to deal with GEMINI's "testsuite" in PR arq5x/gemini#719... TL;DR: Not fun.

Best chance you guys get is pinging BrentP on this if you are really stuck, IMHO...

@robinandeer
Copy link
Owner

Yeah let's bring in BrentP and see what he has to say

Otherwise I guess we'll drop that unit test and/or work around it 😕

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.

4 participants