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

enable using pytest-django #58

Merged
merged 1 commit into from
Oct 26, 2017
Merged

enable using pytest-django #58

merged 1 commit into from
Oct 26, 2017

Conversation

megies
Copy link
Collaborator

@megies megies commented Oct 26, 2017

supersedes and closes #51

@megies megies requested a review from krischer October 26, 2017 07:38
@krischer
Copy link
Owner

How about also using this in the CI?

@barsch
Copy link
Collaborator

barsch commented Oct 26, 2017

tl;dr: I really like to keep the number of dependencies low (this also applies for ObsPy)

I don't like those new dependencies on pytest, django-nose, django-pytest, django-request - I learned the hard way that relying on to many third-party dependencies will fail in the future as people tend to stop maintaining a project - e.g. django-plugins ^^

maybe I don't see the point but why do we need this dependencies which is not working with the default installation - does the test suite in CI not work without?

bear in mind that you can use for your local environment whatever you want - if you need fancy logging - go ahead extend your local environment - if you need a super-duper test runner please be my guest and do it in your setup - but don't polute the original code with your stuff ^^

@krischer
Copy link
Owner

Fair enough. I nowadays strongly prefer pytest to the normal the built-in unit testing framework as it is just a lot more expressive but I do agree that we don't need the additional dependency right now.

Speaking of django-plugins: Do you think we could just replace it with pkgutils plug-in system? I really don't want to maintain it long-term to be honest (and its already a couple django versions behind). It just takes a deep dive in django and I don't know it well enough to do it without a lot of effort.

@barsch
Copy link
Collaborator

barsch commented Oct 26, 2017

Do you think we could just replace it with pkgutils plug-in system?

I guess so - I would like to remove that - problem is that I'm at the moment very packed with work - so I don't see me this touching in the next weeks ... I could also look into django-plugins an bring it up-to-date ...

@krischer
Copy link
Owner

I could also look into django-plugins an bring it up-to-date ...

That would also be nice.

If I find some time I can also give replacing django-plugins a shot. But updating django-plugins would be a nightmare for me.

@barsch
Copy link
Collaborator

barsch commented Oct 26, 2017

I would prefer pkgutils

also while already hijacking this PR ^^ - how compatible to older Django version do we need to keep Jane? If time allows I would update Jane's core to django 1.11 as soon as possible ...

@krischer
Copy link
Owner

also while already hijacking this PR ^^ - how compatible to older Django version do we need to keep Jane? If time allows I would update Jane's core to django 1.11 as soon as possible ...

Not at all I think. Only supporting the latest django + python + obspy + X versions should be perfectly fine!

@megies
Copy link
Collaborator Author

megies commented Oct 26, 2017

maybe I don't see the point but why do we need this dependencies which is not working with the default installation - does the test suite in CI not work without?

This PR doesn't add any dependencies at all. I agree, the less dependencies the better. This PR simply enables using pytest as a drop in locally for running tests. Also this replaces the other PR about nose, so no need to worry.

Not at all I think. Only supporting the latest django + python + obspy + X versions should be perfectly fine!

My only concern is about maintaining existing production servers. I don't know enough about all the migration stuff and I think the docs so far don't cover updating, really. That aside, sure, using newer versions seems like a good idea, if updating is well documented.


Regarding the plugin stuff: Anything's fine with me, as long as I'm able to use multiple plugins for one document type (see krischer/django-plugins#11), this is kind of vital for my use cases.

@krischer krischer merged commit 7b7bf9e into master Oct 26, 2017
@krischer krischer deleted the enable_pytest branch October 26, 2017 19:06
@megies
Copy link
Collaborator Author

megies commented Oct 27, 2017

Thanks for merging, this helps debugging test fails locally..

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