python-hospital / hospital

Tools to diagnose Python projects (supervision, monitoring).
Other
40 stars 8 forks source link

Support pytest (#50) #57

Closed ftobia closed 10 years ago

ftobia commented 10 years ago

Adds support for running healthchecks with py.test.

Also moves the tests out of the "hospital" package, so hospital won't have a pytest dependency.

This pull request does not include tests for this functionality.

benoitbryon commented 10 years ago

Adds support for running healthchecks with py.test

Thank you @ftobia for this pull-request! I'm doing the review ASAP.

Also moves the tests out of the "hospital" package, so hospital won't have a pytest dependency.

Moved these changes to #62, so that the changelog is more readable.

This pull request does not include tests for this functionality.

Do you have an idea about how to add tests? I currently see the following options:

  1. add "pytest" to requirements then run some pytest command in tox configuration file (modules in tests/ do not depend on pytest, but hospital's tox does).
  2. add "pytest" to requirements in tox configuration file, then add a test that run and checks pytest result (modules in tests/ depend on pytest).

If you have not written tests by your side yet, I think I will try to write them. What do you think? Note: the technique for py.test could be useful for nose too!

ftobia commented 10 years ago

I haven't written tests for the pytest code I wrote. I have some ideas of how to do it.

The easiest way is to import pytest and then use pytest.main() to simulate running py.test from the command line. It's easy to test end-to-end behavior this way.

Also there is an undocumented _pytester plugin included in pytest, for testing pytest plugins. This is useful if you want to test pieces that are smaller than end-to-end behavior.

I don't think you need to use two test runners (both nose and pytest). I think this corresponds with your option 2.

I made this pull request without tests because I didn't want to hold up a pull request any longer. Over the next few weeks I'm hoping to have the time to write (or flesh out) tests for pytest integration, but I can't commit to anything, and I figured even without tests the code could be helpful.

benoitbryon commented 10 years ago

@ftobia: do you think the --healthcheck option is really useful? I mean, isn't -m healthcheck enough? Perhaps having only -m healthcheck would be better because it is consistent with -m "not healthcheck". Whereas I do not know how to do some --not-healthcheck. Of course we could also add some --not-healthcheck option, but it also looks like a duplicate of -m, doesn't it?

That said, I am not used to pytest plugins, so I may miss a best practice.

benoitbryon commented 10 years ago

Note for later (when this feature is merged and released): register this plugin to http://pytest.org/latest/plugins_index/index.html

ftobia commented 10 years ago

I don't think something like a --healthcheck shortcut is a best practice. I thought it was a nice-to-have.

On one hand, I am sensitive to "there should be one, and preferably only one, obvious way to do it"; and this helper means extra code and extra tests. On the other hand, in places where this command will be used, potentially by non-Python people or non-pytest people, I think e.g. py.test tests/healthchecks --healthcheck is slightly better than py.test tests/healthchecks -m healthcheck on account of one fewer arcane command-line switch; and it is potentially nicer for newcomers to pytest and hospital.

I do think a helper like --healthcheck only makes sense in the common case, and that e.g. --not-healthcheck wouldn't be worth the effort and confusion.

I'm happy to go with whatever you decide is best.

benoitbryon commented 10 years ago

in places where this command will be used, potentially by non-Python people or non-pytest people, I think e.g. py.test tests/healthchecks --healthcheck is slightly better than py.test tests/healthchecks -m healthcheck on account of one fewer arcane command-line switch; and it is potentially nicer for newcomers to pytest and hospital.

I understand your point of view. But I do not have enough feedback from community to guess what option would be best... and have to make a biased choice :( Right now, my (blindfolded) guess (maybe wrong) would be: For newcomers to hospital and pytest who want hospital features, I would recommend trying hospital without pytest first. If users want pytest too, I suppose they really want to learn pytest too and they will not be afraid about learning the -m option. And users of pytest who want hospital features may appreciate using a builtin feature of pytest (markers).

I do think a helper like --healthcheck only makes sense in the common case, and that e.g. --not-healthcheck wouldn't be worth the effort and confusion.

I think running tests without capturing healthchecks is a common and important use case, because healthchecks can be captured by test runners. It is a side effect of writing healthchecks as special kind of tests. So when you introduce healthchecks, you may like to exclude healthchecks from your (unit|integration|functional) test suite. At least, I do it on the projects where I use hospital. Right now, I do not have much feedback about other's usage.

In doubt, I'd tend to keep the feature as simple as possible for a first release, i.e. "-m healthcheck is fine" and "-m 'not healthcheck' is a common use case". Then let's see if we get some feedback about it once it is released. Also, feel free to submit another pull-request for this --healthcheck option! I may merge it later ;)

Note: I'm working on a talk about hospital for DjangoCon EU... hoping we get some feedback there!

ftobia commented 10 years ago

Sounds good to me.

Cool to hear you're giving a talk on hospital at DjangoCon EU! I'll check it out when it gets posted online.