[WIP] Testing #31
[WIP] Testing #31
Conversation
For those who don't know what buildout is, it's 'just' a far superior alternative to virtual environments. I recommend setting up a If you want coverage information, just use:
Don't worry about
Just check out testing/tests/c_maths.py for an example! A few things to know:
|
|
Warning for Windows users: it won't work for you :). See #30 for patch suggestions, and hopefully some fixes will be merged soon! |
|
THIS IS EPIC! I'm quite excited to jump in on this, right now I plan to commit saturday for fixing this stuff. The above explanation and the need of specifying the doxygen output may hint at the underlying problem with Windows. When running Doxygen I'm explicitly changing working directories which may be what is breaking on windows? Expect more detailed feedback tomorrow, thanks again for putting so much time into this I really appreciate it!!! Assuming Windows stuff gets fixed, then tests will run on Windows right? AKA there's nothing about |
Yes, zc.buildout is just a tool used to generate an isolated virtual environment to run the tests. Once the script is generated it has no influence whatsoever on how the tests run. Today I've tried to tweak the tests to run with Python 2.7 (I'd like to add a tox config with python 3.6 and 2.7), and I ran into an issue: the fact you're defining |
|
I just got travis working with the tests. I'm gonna get more integrations setup on that sandbox repo and then just add them here. This branch should automatically start firing once i setup CI here. For the buildout config python version, is that necessary? |
It's not really necessary but I usually find it quite useful for quick testing. Fire |
testing/tests/c_maths.py
Outdated
| Tests on the c_maths project | ||
| """ | ||
|
|
||
| from testing.base import ExhaleTestCase |
There was a problem hiding this comment.
So for python 2 it's unable to import testing.base. I tried to import exhale just above it but the CI logs are inaccessible right now. I'm leaning toward ditching buildout, if I can't test py2 it's a no-go (since many people still use that on RTD).
There was a problem hiding this comment.
it has nothing to see with buildout really. buildout does not care if you're using python 2 or 3, it just generates a script and sets sys.path according to the packages it's told to include.
There was a problem hiding this comment.
afterthought: did you add the missing __init__.py files before testing with python 2?? Because without them python 2 is lost. (add one in the testing and one in testing/tests)
There was a problem hiding this comment.
yeah i added some __init__.py and various other things, the python2 importing now works!
testing/utils.py
Outdated
|
|
||
|
|
||
| @contextmanager | ||
| def cd(path): |
There was a problem hiding this comment.
All of this fancy stuff came from you right? Very swanky
There was a problem hiding this comment.
it must have come from somewhere in the first place, but it ended up in my toolbox as it's a very useful piece of code :)
There was a problem hiding this comment.
haha, just realised it was used in sphinx as well: from sphinx.util.osutil import cd
testing/utils.py
Outdated
| os.chdir(old_dir) | ||
|
|
||
|
|
||
| def deep_update(orig, override): |
There was a problem hiding this comment.
So the motivation for this method is because the configs are stored at the class-level?
There was a problem hiding this comment.
It would just be used to update a default configuration with a custom one. If the configuration values fitted in a simple dict, we could just use dict.update, but here we have one more level (with the exhale_args attribute).
It was just a quick way to do it, as I had this deep_update function in my toolbox, so feel free to change the way it's done if you find a better / clearer way!
|
Ok so in addition to I can say with absolute certainty that this would not have happened this quickly without your epic kickstart. Thank you again so much! P.S. I really like the idea of having C and CPP tests. That didn't even occur to me :) |
|
Regarding your issues with AppVeyor I would venture into saying that I have no problem if you want to ditch buildout, I just think it's a great tool that can help save time (and virtual environment space !) provided you get over the slightly unwelcoming issues. I use it for many things: run tests, generate documentation, create management or server scripts for Django apps, etc ... If you are interested in building Python apps with C/C++ modules, including testing and documentation in both languages, I have just put together a template project that should work out of the box (and that's the reason I got interested in exhale), you're welcome to give it a go and report any issue or suggest features: https://bitbucket.org/tkhyn/py_cpp. |
|
I'm looking a little more closely at what's up with |
|
Do you mind sharing the traceback of the issue you're getting regarding lxml installation on AppVeyor? And or the result of |
|
https://gist.github.com/svenevs/4b8de3a9c0561053ac66e1ba9242f8c8
P.S. I've got quite a few local changes, there's like 3-4 official (but different) ways to test a sphinx app. I'm still reading through the sphinx dev-list threads but it seems people prefer |
nose or py.test, honestly, does not change much and it's quite easy to swap from one to the other. My experience is that py.test is more flexible and is easier to tweak when you need to do something fancy at a later stage... |
No. buildout can install binaries. Installing lxml 4.2.1 works fine on windows provided setuptools 39.0.1 is available in the environment (it downloads lxml-4.2.1-cp27-cp27m-win_amd64.whl and installs it as pip would do it) ... The error you're seeing here (thanks for sharing the gist) typically happens when the version of setuptools is older than 38.x. |
Ok good to know 👍 So there's this way of doing things, what are your thoughts? import pytest
pytest_plugins = 'sphinx.testing.fixtures'
# test_docs_dir is path to a conf.py that contains breathe_projects, exhale_args etc
@pytest.mark.sphinx('html', testroot=test_docs_dir)
def test_project(app):# should be able to have (self, app): if in class
containmentFolder = app.config.exhale_args["containmentFolder"]
containmentFolder = os.path.abspath(os.path.join(app.srcdir, containmentFolder))
rootFileName = app.config.exhale_args["rootFileName"]
rootFileTitle = app.config.exhale_args["rootFileTitle"]
doxygenStripFromPath = app.config.exhale_args["doxygenStripFromPath"]
assert os.path.isfile(os.path.join(containmentFolder, rootFileName))The way this works is there needs to be a class ProjectTestCaseMetaclass(type):
def __new__(cls, name, bases, attrs):
# check for none and some stuff
# set some class level variables using existing ones
class ProjectTestCase(with_metaclass(ProjectTestCaseMetaclass, unittest.TestCase)):
test_project = None
test_root = None # set in metaclass
test_docs_dir = None # set in metaclass
test_stamp = None # set in metaclass
# vvv what I want to do but is not working
@pytest.mark.sphinx('html', testroot=test_docs_dir)
def test_project(app):# should be able to have (self, app): if in class
# test some common things like library_root.rst existing
# note: app.build() not necessary, exhale is already finished by this pointThe idea being with e.g. class CMathsProject(ProjectTestCase):
test_project = "c_maths"
# could also just set conf.py string or override values here too but let the metaclass generate itI don't know if all of that is possible, i need to sleep on it.
Ahhh. All I needed was to |
I've nothing wrong with that at all, despite it not being the approach I chose. My view was to actually try to avoid launching the full sphinx machinery in the tests as the only entry points in Basically I was assuming that sphinx is sufficiently tested to:
and that these points are none of The only real downside of the 'mock' approach I chose is that the tests will still pass in case One reason it could be useful to have the actual sphinx app at hand in the future is if features (for example using more event handlers than one or two) are added. But I'm not sure how likely it is to happen. I don't have any hard opinion on that really, it's just that my approach is to simplify and optimize things whenever I can, and actually run tests only on the things that actually need testing / aren't tested elsewhere to avoid introducing issues that are not mine/ours! It's basically about decoupling, and removing as many unknowns or variables as you can from an equation before solving it :). Same thing regarding a So, in the end, I'd say it's your call! Either you want to launch As a side note, I have some input on the way the Glad you made it work with (and sorry for the wall of text! did not have the courage to condense it more once written!) |
|
The issue is that the As far as the module level configs thing....god yes. One of the python 2 problems btw was you need to But yes in the end, not all test cases are going to be project based....because those all require a full execution with doxygen and the like. |
|
Ok, got the point about The only thing I'd be looking that, then, is how to pass custom configuration to the sphinx app pytest fixture in a very simple way (i.e. without having to create a I don't think the refactoring of the |
|
Yeah gimme a couple of minutes to redo the stuff that works. I've been working in a different repo because I knew getting the CI stuff setup was going to require a large number of commits. I'll just make a self contained file here with how their fixture stuff works, and then post back with where I think it needs to go. |
|
Cool, thanks. Just had a look, there is a very promising |
|
So yeah the problem is that I can't get def __new__(cls, name, bases, attrs):
# check things that need to be set in children...
# set test_docs_dir based on test_project name indexing into projects/{test_project_name}/docs
@pytest.mark.sphinx('html', testroot=test_docs_dir)
def project(app):
# run some common tests
attrs["test_exhale_project"] = projectYou can't bind it, e.g. if you add a I think the But at this point I can't figure out how to use I will figure out how to commit this here now because the changes to |
|
@tkhyn you should be able to |
|
Great, I'll have a closer look very soon kwargs to @pytest.mark.sphinx('html', testroot=test_docs_dir, confoverrides={'whatever': 'we_want'})The way to apply a decorator to a method in the metaclass could be something like (warning, untested code, may be flawed!!): def __new__(cls, name, bases, attrs):
def project(self, app):
# whatever you want project() do do
attrs['project'] = project
c = super(ExhaleTestCaseMetaclass, cls).__new__(cls, name, bases, attrs)
pytest.mark.sphinx('html', testroot=test_docs_dir, confoverrides={'whatever': 'we_want'})(c.project)
return cThe reason they are using generators is there is some setup and teardown / cleanup code. Anything above the |
|
OH SNAP. Yeah that looks like something that would work, instead of my nonsense...cool I'll take a look after dinner, I think I should be able to finalize this stuff tonight! |
|
yeah I'm not 100% sure about when the decorator should be called (i.e. before the super call or after it). Metaclass stuff can be a bit tricky ... So it looks like we could have a unique generic |
|
Gah whatever this is a waste of time, the decorators are nice but I cannot for the life of me figure out how to bind this thing. There's some discussion online about # necessary imports
from sphinx.testing.path import path
from sphinx.testing.util import SphinxTestApp
# in the metaclass
def setup_project(self):
# inspired by cd, just creates an empty conf.py
# the rest is handled via confoverrides :)
with conf_py(test_docs_dir):
app = SphinxTestApp(
buildername="html",
srcdir=path(test_docs_dir),
freshenv=False,
confoverrides=confoverrides,
status=None,
warning=None,
tags=None,
docutilsconf=None
)
assert isinstance(app.config.exhale_args, dict)
print("You did it, yay!")The only concern is what you mentioned earlier about setup / cleanup. Anything with respect to cleanup come to mind? |
|
All right, got it working. Check out the commit above (python 2 compatibility will come tomorrow in another commit). |
| flake8-docstrings | ||
| flake8-import-order | ||
| pep8-naming | ||
| flake8-colors |
There was a problem hiding this comment.
Not sure i want all of the errors these are spitting out to fail, some of them I fundamentally disagree with.
docs/testing.rst
Outdated
| .. todo:: | ||
|
|
||
| Finalize these docs. | ||
|
|
There was a problem hiding this comment.
@tkhyn any chance you would be willing to fill in these docs, describing what :class:pytest.fixture you have enabled and what they do?
I also want to change ExhaleTestCase::config to be confoverrides for consistency, but if you're willing to write these docs maybe you could change that too? WARNING: see hack for docs in testing/base.py, there are unfortunately two class definitions. One for the docs and one for the tests (otherwise the ExhaleTestCase docs were doing something really strange where it just said "alias of ExhaleTestCase" which linked to itself.
Locally I'm working on two unrelated components of the test framework
- Validation of the hierarchy (almost finished).
- Finalizing the CI stuff.
The docs for all the testing don't have to be perfect, I mostly just need them for myself so that after merging this I can peel off and make new project test cases as time permits.
There was a problem hiding this comment.
P.S. I am aware of the docs test failing right now, I know what the issue is but am awaiting a response before fixing.
corrected. I should have known better...
it doesn't for me, nor would i like it to.
to be honest, the things you are doing here are beyond my knowledge of the internals of python. for example, in languages like Java or C++ you should do the why is this a python bug and not something to do with sphinx (or autodoc)? the tests ran fine previously, it's just the documentation that was broken |
no problem, I agree it's quite confusing!
it removes it in the signature (it shows
I tracked it down by debugging deep into sphinx code and realised sphinx had nothing to do with it. |
|
And yes, I'll definitely attempt to come up with something understandable in the docs regarding the magic surrounding |
|
hehehe well i didn't see it either. thank you for fixing it proper :) thank you very much for being willing to document it too! I respect and fear metaclasses and decorators. i have a surface level understanding of them, but as witnessed by the super cool stuff you've added here i know which part of python i want to focus on learning more intimately next |
|
no worries As I'm writing the docs on |
|
In light of recent discussions, I will fix the singular docs consistency thing. My local changes are going to be a little challenging to merge into this, so please hold off committing until they are up here (I'll greenlight you again, but I think we're almost done with this!). Out of curiosity, what time zone are you in? |
testing/base.py
Outdated
| ############################################################################ | ||
| # Automatically create docs/{conf.py,index.rst} for the test project. # | ||
| ############################################################################ | ||
| def _with_docs_dir(self): |
There was a problem hiding this comment.
Previously before return default_confoverrides I was setting a class-level cls.config variable (hence the question earlier). Locally, with either scope="function" or scope="class" the testing/projects/{test_project}/docs/{conf.py,index.rst} are currently being created, but not on Travis for some reason. Locally they are not deleted though.
I had done cls = default_confoverrides and bound _with_docs_dir later because without it self.app.config is not available. So it seems there is a more glaring problem that I will investigate further.
- Glaring problem:
@confoverrideschangingcontainmentFolderorrootFileNamewill break that, hence the strict requirements onExhaleTestCase.copyThisProject. I don't think an elegant solution is needed, because the would-be test-case rendering site is going to assume that{containmentFolder} := test_projectand{rootFileTitle} := {test_project}_root.rst(as changed inmake_default_config).
The idea is in the future I will want to be able to actually save all of the generated reStructuredText and build out the testing website. I'm probably going to do it on a gh-pages branch (far far off in the future) because it will be very useful to actually set GENERATE_HTML = YES to have the ground truth (Doxygen) included.
There was a problem hiding this comment.
The way to change the sphinx root directory for the sphinx.testing.fixtures.app is to use pytest.mark.parametrize. Some digging will be needed to know where and with which arguments to call it.
testing/decorators.py
Outdated
|
|
||
| # If a given test case needs to run app.build(), make sure index.rst | ||
| # is available as well | ||
| cls_exhale_args = sphinx_kwargs["confoverrides"]["exhale_args"] |
There was a problem hiding this comment.
@tkhyn Ok so this is where my understanding breaks.
- The goal is automatically creating
testing/projects/{test_project}/docs/{conf.py,index.rst}.- Currently this is done with
scope=class, which is a problem whencontainmentFolderorrootFileNameare changed inconf.py. - Not a huge deal (I will document it more explicitly, see
testing.base.ExhaleTestCase.copyThisProject)?
- Currently this is done with
- I don't understand what's going on with the priority based
@pytest.mark.exhaleand thesphinx_kwargs. I've read the marking docs like three times, but don't understand where this priority actually comes from.
Basically, what are your thoughts on this? The reason for switching to project local docs/conf.py is because if you run the tests in parallel with a single TEST_DOCS_DIR/conf.py things can break. Creating it at the function scope may not solve this either though.
There was a problem hiding this comment.
I'd say the way to change the sphinx root directory used by sphinx.testing.fixtures.app is to use pytest.mark.parametrize for the "app_params" fixture (see sphinx.testing.fixtures). Some digging will be needed to know where and with which arguments to call it.
testing/base.py
Outdated
| The following are assumed: | ||
|
|
||
| 1. Every derived class of :class:`testing.base.ExhaleTestCase` calls this | ||
| method **exactly once**. |
There was a problem hiding this comment.
'exactly once' or 'at most once'? and actually why? If you call it twice it'll just override the files in the test results dir...
There are ways to enforce it by setting / checking a _project_copied attribute:
if hasattr(self, '_project_copied'):
return
[...]
self._project_copied = True
There was a problem hiding this comment.
So yeah I think I want to just remove this stuff for now, as a proof of concept it works fine but having learned more about marking I think the better approach is to define test methods on a subclass of ExhaleTestCase and mark them explicitly. Since this was all just a fantasy for what is to come in due time, it's better to just remove it for now.
This stuff will become useful if / when I decide to battle the template issues again. That's a long story though, and I've yet to decide if I actually want to go down that road (again).
testing/base.py
Outdated
|
|
||
| For (2), it is really just important that the path to | ||
| ``{containmentFolder}/{rootFileName}`` is the same as what is specified | ||
| by :func:`testing.base.make_default_config`. |
There was a problem hiding this comment.
Again, I am quite confident there are ways to enforce and/or mitigate this.
I feel it's overkill to prevent the use of @confoverrides just because it may change one value.
testing/base.py
Outdated
| # otherwise we need a ``test_project`` attribute | ||
| raise RuntimeError('ExhaleTestCase subclasses must define a "test_project" attribute') | ||
|
|
||
| # Run all test projects local to the project folder | ||
| testroot = os.path.join(TEST_PROJECTS_ROOT, test_project, "docs") | ||
| attrs["testroot"] = testroot |
There was a problem hiding this comment.
If you really think you're going to need parallel testing, then you need a per-test testroot. As tests for the same project will overwrite each other
testing/base.py
Outdated
| ############################################################################ | ||
| # Automatically create docs/{conf.py,index.rst} for the test project. # | ||
| ############################################################################ | ||
| def _with_docs_dir(self): |
There was a problem hiding this comment.
The way to change the sphinx root directory for the sphinx.testing.fixtures.app is to use pytest.mark.parametrize. Some digging will be needed to know where and with which arguments to call it.
tox.ini
Outdated
| # deps = | ||
| # {[testenv]deps} | ||
| # {[testenv]deps_py2} | ||
| # TODO [testenv:distribute] <- streamline bulding pypi releases |
There was a problem hiding this comment.
so you really want to ditch python 2.7 / 3.x testing? Why?
In my opinion it's quite important to make sure that the test suite completes on all target version of python
testing/decorators.py
Outdated
|
|
||
| # If a given test case needs to run app.build(), make sure index.rst | ||
| # is available as well | ||
| cls_exhale_args = sphinx_kwargs["confoverrides"]["exhale_args"] |
There was a problem hiding this comment.
I'd say the way to change the sphinx root directory used by sphinx.testing.fixtures.app is to use pytest.mark.parametrize for the "app_params" fixture (see sphinx.testing.fixtures). Some digging will be needed to know where and with which arguments to call it.
testing/decorators.py
Outdated
| ################################################################################ | ||
| # Automatically create docs/{conf.py,index.rst} for the test project. # | ||
| ################################################################################ | ||
| def _with_docs_dir(self): |
There was a problem hiding this comment.
I'd think it would be better to leave this code near the _set_app definition as it's a test case setup thing rather than closely related to how the configuration is determined.
There was a problem hiding this comment.
I think I've solved it once and for all! In conftest.py you can define these two key methods
def pytest_runtest_setup(item):
def pytest_runtest_teardown(item, nextitem):With some additional class level attribute saving and extra machinery it appears to all be working as expected. It got pretty ugly, so I'm going to clean that up tomorrow and then push ;)
KiwiZone :) Good evening! |
Too right! I'm in UTC-7, I figured you had to be somewhere else since normally when I'm working on this I'm the only one up. Thanks for the feedback, I'll do some digging right now! |
testing/decorators.py
Outdated
| """ | ||
| # If not explicitly overriden here, somehow the `func_to_sphinx_map` is getting | ||
| # populated with previous cls entries. | ||
| cls.func_to_sphinx_map = {} |
There was a problem hiding this comment.
@tkhyn what do you think of the new strategy (see testing/conftest.py's setup and teardown methods). The only thing I didn't understand is why I had to reset to {} for every time this method was called, but as far as I understand this is exactly what should be done.
The reason it was confusing was because the test methods defined in a separate class CMathsTestsNoRun were somehow getting in here if I don't reset... (e.g. set a trace in conftest.py/pytest_runtest_setup and look at item.cls.func_to_sphinx_map).
There was a problem hiding this comment.
I actually had started working on keeping the testroot stuff in ExhaleTestMetaclass.__new__ so that everything could stay at the same place and there would be no need for pytest setup and teardown hooks.
I finally made it work tonight after a little bit of effort and I think it's significantly cleaner. Check out https://github.com/tkhyn/exhale/blob/testing-fixture-only/testing/base.py#L135
Note: we can't merge _set_app and _rootdir because sphinx's app fixture is called in-between. It would make more sense to define _rootdir before _set_app by the way, but that's a detail
There was a problem hiding this comment.
WOW that's exactly what I tried to do but totally failed at! That commit has been added here with some other small changes / docs updates!
There was a problem hiding this comment.
yeah I actually gave up myself on my first attempt, I needed to sleep on it apparently. Everything started to become easier once I thought about adding app_params in the arguments and then overwriting srcdir in it. I think it might not even be necessary to yield testroot as the value is not used but it doesn't matter much ...
This could be made even cleaner by defining _set_app and _rootdir as partial functions out of the __new__ definition.
There was a problem hiding this comment.
TBH I'm very pleased with this. If you want to clean it up go for it, but I just want to build out one small CPP test case and get the class hierarchy tested (or at least a very basic version) and then put this down for a while. I need to fix the windows stuff and then table exhale for a while :S
There was a problem hiding this comment.
yeah nah it can stay like that, and once everything will be settle we can worry about making things super-clean 😀
|
Note: I had to force push a commit away that I made, sorry for that :/ |
|
GAH (notes to self before rage quitting on unfathomable regression):
Update damn i get it. It's the whole ReadTheDocs // Doxygen dilemma all over again.
|
|
@tkhyn WINDOWS IS FIXED!!!!!!!!!!!!!!! This is really close to being merged now (finally.......). I just need to cleanup the |
|
Ok I'm going to sleep on this and then do a full pass over the diff and fix up some small things like @tkhyn thank you very much for being willing to hash this out in such detail with me, this was a really cool experience for me. I think what we (well, mostly you...) arrived at here is quite wonderful, and will seriously help me get Exhale to where I want it to be. I don't expect you to go through the full thing, but was there any stuff floating in your mind that needs to be addressed here still? P.S. Sorry this went stale for a while, end-of-the-semester woes and all :/ |
Introduced `pytest` based framework
=================================================
- Extensive testing backend with metaclass and
fixture magic enabling easy creation of new
test projects and associated tests
- Not all hierarchical representations are
complete, see hierarchies docs
- Continuous Integration!
- Windows: AppVeyor
- Linux / OSX / docs / lint: Travis CI
- Coverage: codecov.io
Exhale Core
=================================================
- Exhale can now run on Windows!
- See exhale/deploy.py doxygen execution
- Fix pathing issues using normpath (see #30)
- Added more forceful doxygenStripFromPath logic
as needed for Travis
- Allow spaces in names for program listings
NOTE: union nesting is less broken than it was,
but still not valid.
Introduced `pytest` based framework
=================================================
- Extensive testing backend with metaclass and
fixture magic enabling easy creation of new
test projects and associated tests
- Not all hierarchical representations are
complete, see hierarchies docs
- Continuous Integration!
- Windows: AppVeyor
- Linux / OSX / docs / lint: Travis CI
- Coverage: codecov.io
Exhale Core
=================================================
- Exhale can now run on Windows!
- See exhale/deploy.py doxygen execution
- Fix pathing issues using normpath (see svenevs#30)
- Added more forceful doxygenStripFromPath logic
as needed for Travis
- Allow spaces in names for program listings
NOTE: union nesting is less broken than it was,
but still not valid.
|
Ugh so I can't explicitly mark it as merged, but really want this one to show up as Green rather than Red. So I'm gonna do some force pushing here to undo my local $ git checkout -b tkhyn-testing archive/tkhyn-testing-pr-31In case you ever need the Anyway, this is all being done because you totally deserve the "little green square" for this! |
Following #5, this is a WIP regarding adding tests to
exhale.The main idea is to use a mock sphinx application, which is a plain old object with corresponding attributes and with the configuration exhale needs for its setup.
More info in comments below, as the structure of the testing package may evolve.