Test Registry project

stas · February 6, 2019, 7:37pm

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.

stas · February 6, 2019, 10:11pm

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.

Benudek · February 6, 2019, 10:17pm

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.

stas · February 6, 2019, 10:50pm

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.

stas · February 6, 2019, 11:24pm

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.

Benudek · February 6, 2019, 11:38pm

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

stas · February 6, 2019, 11:44pm

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.

stas · February 7, 2019, 12:57am

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.

ashaw · February 7, 2019, 11:22pm

Awesome idea! I’ll start prototyping doctest and check back to see if it works as intended

Benudek · February 8, 2019, 5:04am

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!

Benudek · February 10, 2019, 9:08am

@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

stas · February 10, 2019, 4:13pm

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 run pytest -doctest then it saves the mappings, and since that would then be passed via make test this 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.py re-arranges sys.path to include ../fastai 2nd 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

stas · February 10, 2019, 5:10pm

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.

Benudek · February 10, 2019, 10:41pm

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 · February 15, 2019, 8:12pm

8 posts were merged into an existing topic: Improving/Expanding Functional Tests

Benudek · February 11, 2019, 6:33pm

@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.

stas · February 11, 2019, 6:40pm

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.

Benudek · February 11, 2019, 6:46pm

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!

Benudek · February 11, 2019, 6:48pm

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!)

stas · February 11, 2019, 6:56pm

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?