Quite the contrary, it was your idea @Benudek, I just expanded on it.
Jeremy, thank you for acknowledging that this would be a worthy effort.
1 Like
I also wanted to elaborate why I am being anal about not accepting PRs that include things like:
tests/test_train.py:
""" file: tests/test_train.py """
and
def test_lr_find(learn):
"Tests API: lr_find"
This is directly because filenames and test function names are self-documenting most of the time, but the main indirect reason has to do with future changes. If someone decides to rename the filename, they are likely to miss the comment in the file that now lists incorrect filename, same goes for functions.
Any duplication of source is likely to lead to mistakes in the future, therefore a developer should always strive to have one source for anything, when itās possible and sensible. This is the main reason for refactoring, but it applies to documentation just as well, since documentation is the API equivalent for humans.
I hope this explanation was helpful.
3 Likes
yes on the file: it isnāt necessary and should go out. I also sometimes gateway code and have no issues really with s.o. being āpickyā - better than a messed up code base !
But this
def test_lr_find(learn):
"Tests API: lr_find"
is a suggestion to arrange for the mapping, that is this test-function tests API XYZ (so we can find it later on). I just have this pattern in one file yet, but if we want to map it seems to me we need sth like that?
Probably good we agree on some format, then when I touch a file can just add that.
I thought this was your intention, but letās save it for when there is a code to support that, otherwise other people looking to write new tests will find the style inconsistent and itād be unnecessarily confusing for something that is already quite complex.
And as the proposal says those will be actual code references and not strings/comments, since we need to make sure we map the right thing. So in the quoted example itād most likely be:
def test_lr_find(learn):
testapi(learn.lr_find)
Notice, how the argument to testapi() is an actual method reference (w/o ()). This way if internal classes get renamed, we never need to fix the tests.
That means that perhaps testapi call will come at the end of the test rather than beginning, if the tested api method is not available upon entry.
1 Like
I donāt find testapi an intuitive function name in this context:
def test_lr_find(learn):
testapi(learn.lr_find)
since the function testapi isnāt testing anything, it just registers things.
Perhaps:
def test_lr_find(learn):
test_register(learn.lr_find)
test_reg(learn.lr_find)
test_reg_api(learn.lr_find)
test_register_api(learn.lr_find)
or perhaps it shouldnāt start with test_ as those often cause problems when pytest detects them in normal code.
Perhaps an english like expression function name?
def test_lr_find(learn):
this_tests(learn.lr_find)
I like this_tests(funcA, funcB) as the most intuitive so far.
This is wide-open to any other suggestions from all.
not starting with ātestā is a good point - this_tests makes most sense to me and for now I just update above
Was also wondering on the name of the file where we place doctest and this_tests then and under which package to place e.g. fastai.utils or fastai.gen_docs?
I think meanwhile the cmd functionality in the notebook would not help too much, maybe if there would be some awesome visualisation of what a function does during runtime but even that has nothing to do with a cmd so taking it out.
Will update above design accordingly
1 Like
this is all docs, so some place under fastai.gen_docs - probably just stick it in one of the existing modules, we can move them later if need be.
I think meanwhile the cmd functionality in the notebook would not help too much, maybe if there would be some awesome visualisation of what a function does during runtime but even that has nothing to do with the dmc so taking it out.
yes, letās start with basic functionality and not worry about fancy widgets just yet.
Iād say just start with a simple dumping into a text map file. Once you have something in place, we will ask Andrew to see what the best format would be and if he could help us do the docs front end.
1 Like
yickes, discourse fails to handle concurrent wiki edits, got my changes wiped out by a later editor, so beware not to edit at the same time if you see someone else is editing the post already. got the wiped out part restored.
1 Like
Awesome idea! Iāll start prototyping doctest and check back to see if it works as intended
2 Likes
OK, I started working on the function this_test to register tests with what they test in a json file. Will take me a moment
Then would look at doc_test (unless someone else already picked it - what ever makes sense), thx a lot @ashaw!
1 Like
@stas submitted a pseudo PR (because it shouldnt get merged) to discuss this_tests design here: https://github.com/fastai/fastai/pull/1604.
Seems easier discussing code on github rather than here.
If you monitor git anyways, also closed PRs, and additional notify here to noisy, let me know 
Also looping @ashaw
1 Like
I think we can discuss it here just fine, perhaps another wiki post where any person can pose a question and another answer.
The easiest would have been to work together on the same branch, but Iām not sure how to arrange that since we donāt all have the same permissions.
To your initial prototype and questions you asked in the PR:
-
there should be no setup/teardown in each test module - this should happen only once at the level of pytest - so need to figure out the pytest starting/finishing hooks/callbacks. The only thing that goes into test modules is a single test_this() call per test. Itās possible that we will need to write a small pytest extension to make it happen. But perhaps there is a direct way to hook in via
tests/conftest.py. surely the loading can happen there, itās just the saving of the mapping file when the test suite is 100% is successful is the one to sort out.So if we create a custom extension then
pytest ...does nothing, but if we runpytest -doctestthen it saves the mappings, and since that would then be passed viamake testthis will do the trick.You understand why I suggestion to only ever update the mapping file after all tests were run successfully? Since if we donāt you need to make a much more complex logic to add/remove mapping file entries. If itās only done in one swoop, the whole mapping file is written at once, and never so only parts of it get modified. of course on the git level that will still be the case (only some of it will get modified), but there will be a trivial implementation. a. collect the entries from
test_this()b. dump them in a consistent sorted way into a mapping file - done! i.e. the mapping file is never updated, just completely re-written.Down the road perhaps we can make it more complex, but for now we donāt want to make this into a project, it should be as simple as it can be.
-
for now the activation should only happen during full pytest/make test run and not individual specific test modules run. So test_this() should do nothing if itās not a global run, i.e. return doing nothing. but for now it can just do the normal thing. see (1) for why.
But thinking more about it, the other approach is to not use an extension and to save each
test_this()call into the mapping file, and code all the complexity of updating the mapping file. I guess thatās a valid way too. I just thought that doing it occasionally and in one swoop will make it easier on developers where they donāt need to monitor yet another file to commit, especially since it wonāt be located under testsā¦ not sure. -
the code ideally should go with the docs gen, but for now itās ok under test/utils. You shouldnāt have problem importing from the main code base anything because
tests/conftest.pyre-arrangessys.pathto include../fastai2nd in the path. the reason it should go under docs gen is because thatās where the code using it will be - so it should be in one place. Also remember that the tests folder is not distributed to the users, so if someone looks itād be good for them to see the whole picture together. -
the map file should go somewhere under fastai/ where .py files are, so there is nothing to hardcode, as itās always tests/ā¦/fastai/somefile.json which is the one added to MANIFEST.in
Here is a quick prototype for you to work with that I built from a few SO posts:
add to tests/conftest.py:
@pytest.fixture(scope="session", autouse=True)
def start_doctest_collector(request):
request.addfinalizer(stop_doctest_collector)
print("\n***Initialize docset collector***")
def stop_doctest_collector():
print("\n*** Finalize docset collector***")
print(Helpers.x) # this is where the mapping file dump will happen.
class Helpers:
x = {}
@staticmethod
def this_tests(item):
Helpers.x[item] = 1
@pytest.fixture
def helpers():
return Helpers
and now write a test, say tests/test_x.py:
import pytest
def test_true(helpers):
helpers.this_tests(True)
assert True == True
def test_false(helpers):
helpers.this_tests(False)
assert False == False
Run:
=========================================================================== test session starts ============================================================================
platform linux -- Python 3.7.1, pytest-4.0.2, py-1.7.0, pluggy-0.8.0 -- /home/stas/anaconda3/envs/fastai/bin/python
cachedir: .pytest_cache
rootdir: /mnt/nvme1/fast.ai-1/br/fastai/master, inifile: setup.cfg
plugins: xdist-1.25.0, repeat-0.7.0, forked-0.2, cov-2.6.0, nbsmoke-0.2.7, ipynb-1.1.1.dev0
collected 2 items
tests/test_x.py::test_true
***Initialize docset collector***
PASSED
tests/test_x.py::test_false PASSED
*** Finalize docset collector***
{True: 1, False: 1}
Note that pytest buffers that initialize message, so it is called before all tests.
So now you have the pytest setup/teardown with a class attribute dict that collects the data, which you would save at the finalization stage.
So next get rid of the helpers fixture, that otherwise needs to be added to each test function (unwanted noise), so that it looks something like:
import pytest
from fastai.gen_doc.doctest import this_tests
def test_true():
this_tests(True)
assert True == True
This also looks potentially useful to tackle this task of avoiding a need for a fixture argument for each test: https://stackoverflow.com/a/36383912/9201239
there is also pytest cache that perhaps is easier to use than the class attribute, https://stackoverflow.com/a/50611422/9201239
and finally code test_this to the spec and we are pretty much done on the tests side.
the only other nuance to figure out is to run the collector only when pytest was given no specific test module to run (i.e. all tests are run). ok, here is how youād do it, basically peeking into argv to see if specific test modules are passed (will need to tweak if the order of args is different, currently itās inflexible but is good enough for a start)
@pytest.fixture(scope="session", autouse=True)
def start_doctest_collector(request):
print("\n***Initialize docset collector***")
print(sys.argv)
if 'test_' in sys.argv[-1]:
print("Specific tests are running, not collecting doctests")
else:
print("All tests are running")
request.addfinalizer(stop_doctest_collector)
perhaps there is an easier way, but at the first search I couldnāt find one.
If youāre stuck, first check out SO (stackoverflow) since most questions have been answered already there. but use google to search it - gives much better results. This is how I scraped the code above, as I havenāt done this kind of problem before - took a few minutes to put a few answers together.
1 Like
Yes, thatās fine. So will definitely do the google stackoverflow tour. Just thinking itās good to coordinate the PR in case you had specific solution in mind and avoid the PR would not go through.i
@stas, coded up (should have checked simply the pytest doc for sharing context between test classes!) along your comments. Only wasnāt sure, what you mean here:
do you mean doc_tests or to write a separate test for the functionality I will add?
Could make a PR for you to check, but I guess better I 1st look at doc_test and see how this all works in combination.
2 more remarks:
Using the namespace helper
to avoid copying imports in all test files later on. So, I suppose we will need to trigger installing this when submitting the PR also
The cache
looks good, but afaict we would need to pass around the request object in all test methods - lots of copy & code and class variable should just be fine.
thx for sharing your knowledge again, good learning curve here for me.
sorry, that was a typo - fixed now in the original. ātest_thisā
to avoid copying imports in all test files later on. So, I suppose we will need to trigger installing this when submitting the PR also
looks good, but afaict we would need to pass around the request object in all test methods - lots of copy & code and class variable should just be fine.
I meant that we donāt need to edit any tests other than adding a single line in each test of:
test_this(...)
and most likely we will have to do at the top of each test module:
from foo import test_this
I was trying to find a way how to make this function globally available in all test modules w/o needing to import it everywhere, but I couldnāt find a way to do that. Iām new to python so perhaps there is some hack that can do that. Itād be nice not to need to import that function everywhere. Python seems to be very non-hackable (for good reasons most of the time, but not always).
I hope that clarifies things.
1 Like
yes, cool - thx ! and the tip you shared does work without imports:
Just this line on a test class:
pytest.helpers.this_tests(Learner.lr_find)
After declaring it in conftest.py
@pytest.helpers.register
def this_tests(testedapi):
RegisterTestsperAPI.this_tests(testedapi)
and installing the package
So, thats quite useful as it avoids too much copying code!
seems more like you are pretty much ahead of the game in python to me I am rather new to python but did my fair share of programming ( but python so far is the most fun programming experience!)
yes, but as I said, we usually donāt want extra deps. I think the best approach is to port it so itās self-contained in the test suite (conftest.py), but otherwise itās an extra dependency which we typically try to avoid - dependencies often tend to break or disappear.
And iād shorten it to something shorter, pytest.g.this_test if itās sensible, may be not, open to suggestions. g for global?
1 Like