Ah you are right! it should be an array. So maybe an array of metadata?
fastai.train.lr_find’: [{ “file”: “fastai-doctest/tests/test_train.py”, “line”: 3 }, { “file”: “fastai-doctest/tests/test_train.py”, “line”: 8 }]
1 Like
Idea is we have an array with tests as members per API, see below a test (incorrectly registered but good for a test)
> "{\n
> \"fastai.basic_train.Learner.fit\":
> [\n
> [\n \"test: test_lr_find\",\n \"line: 16\",\n \"file: test_train.py\"\n ],\n
> [\n \"line: 29\",\n \"test: test_fit\",\n \"file: test_train.py\"\n ]\n
> ],\n
> \"fastai.train.fit_one_cycle\":
> [\n
> [\n \"line: 37\",\n \"test: test_fit_one_cycle\",\n \"file: test_train.py\ "\n ]\n
> ]
> \n}"
Little late in europe, offline now 
Ok after reading the feedback you guys had - I rebuilt skeleton tests to try and make it as easy as possible for people to write tests. Now you can create tests completely from a notebook.
The idea is similar to what we do with documentation notebooks. Lots of edge cases to handle, but I do think it makes writing tests less painful
1 Like
Hmm I guess I still don’t understand why we don’t do:
> "{\n
> \"fastai.basic_train.Learner.fit\":
> [\n
> {\n \"test\": \"test_lr_find\",\n \"line\":\" 16\",\n \"file\": \"test_train.py\"\n },\n
> {\n \"line\": \"29\",\n \"test\": \"test_fit\",\n \"file\": \"test_train.py\"\n }\n
> ],\n
> \"fastai.train.fit_one_cycle\":
> [\n
> {\n \"line\": \"37\",\n \"test\": \"test_fit_one_cycle\",\n \"file\": \"test_train.py\ "\n }\n
> ]
> \n}"
Not a big deal though. I can just as easily parse the string on my side
1 Like
IMHO, tests in notebooks is not the best idea, unless they are done on top of the normal tests suite (i.e. the features are already unit tested). We need a reliable test suite to run when making a release, and so really let’s stick to having the tests as part of the test suite. And once we have the luxury to be able to add extra tests in nbs, then it’s great.
Unless, of course, we integrate the notebook tests into the test suite, e.g. how I did it here:
https://github.com/stas00/ipyexperiments/tree/master/tests
by using this pytest extension:
pip install git+https://github.com/stas00/pytest-ipynb.git
but it needs a lot of work to be like the normal test suite (skipping, collecting, etc.). It’s ok though for the simple needs of that module. It’s a fork of an abandoned extension, and it’s really old and needs to be ported to the latest tools. I just added a few fixes to make it run the notebooks as tests, since I couldn’t find anything else to use.
And yes, we currently do run nb tests via nbval, but it’s not quite like a test suite - it only tells whether things are not completely broken, but no way to measure anything.
To be clear, these aren’t meant to be tests in notebooks. The tests are written back to the test suite.
The notebook I shared is just a demo of the process, but we should remove it.
We are just using it to search for existing tests and to write new ones.
Once you are done, save_test exports the function you wrote to the associated tests/test_module.py file - fastai/tests/test_data_block.py
After this, you can just delete the notebook, as the tests are saved to the test suite.
If you scroll down to the end, you can see the function is appended
I want to make sure we are on the same page. Totally fine if it still doesn’t sound helpful.
1 Like
Now I understand your point
and actually, I build up the dictionary as you suggest in doctest and the debugs show I get the {{ notation:
list_test = [{'file: '+ basename(pathfilename), 'test: ' + test_function_name , 'line: ' + str(lineno_parentfunc)}]
print('\n############ list_test =' + str(list_test))
fq_apiname = full_name_with_qualname(testedapi)
print('\n############ fq_apiname =' + str(fq_apiname))
if(fq_apiname in RegisterTestsperAPI.apiTestsMap):
RegisterTestsperAPI.apiTestsMap[fq_apiname] = RegisterTestsperAPI.apiTestsMap[fq_apiname] + list_test
else:
RegisterTestsperAPI.apiTestsMap[fq_apiname] = list_test
print('\n############ RegisterTestsperAPI.apiTestsMap[fq_apiname] =' + str(RegisterTestsperAPI.apiTestsMap[fq_apiname]))
Can see in conftest the json.dump method changes it from [{ to [[. Not sure if that is python standard or I could impact that - maybe also that default function has an impact but I cant quite figure out how.
conftest: https://github.com/Benudek/fastaidoctestexchange/blob/master/conftest.py
def set_default(obj):
if isinstance(obj, set):
return list(obj)
raise TypeError
def stop_doctest_collector():
fastai_dir = abspath(join(dirname( __file__ ), '..', 'fastai'))
print('################ RegisterTestsperAPI.apiTestsMap: ' + str(RegisterTestsperAPI.apiTestsMap))
with open(fastai_dir + '/TestAPIRegister.json', 'w') as f:
json.dump(obj=RegisterTestsperAPI.apiTestsMap,fp=f, indent = 4, sort_keys= True, default=set_default )
At least, revisiting the code I generate a better readable version now
TestAPIRegister.json: https://github.com/Benudek/fastaidoctestexchange/blob/master/TestAPIRegister.json
Happy for pointers, otherwise I understood we can handle the array [[?
this is fantastic! really cool !
Exactly my thinking: if one can surface tests as much as possible in notebooks - hopefully, some will contribute. And writing tests in notebooks would make it easier! Great stuff!
I actually wonder, if one can automate the PR process a little because for some it can be a ‘put-off’ : like we pull down the code for that test and make a branch before we start. After the test was added, they get a simple command to push it and get redirected to github, where they can see the test results. We already have this nice python script to create a branch and might be able instrumenting it here.
This could be a great tool for studygroups e.g.: task is to add tests for functionality xyz and we could classify it soon as ‘sometimes rather low hanging fruit’ in the how to contribute area. @PierreO
But we could handle this as a separate PR, right? So 1st we deliver this doc_test stuff and then another PR to facilate using a notebook to write tests.
thumbs up @ashaw this is great !
Thank you for clarifying, now that I read the sample notebook I can clearly see what you were proposing.
I think your idea is really really cool, but I’d love to see it actually working and resulting in PRs with new tests. The problem is not to write a skeleton but to know what to test and to know what the good and the bad results are.
I personally find it a way easier to copy an existing test and adjusting it to my needs. I would have a very hard time making sense from a skeleton from your sample that gives me:
instance = ItemList(items: Iterator, path: Union[pathlib.Path, str] = '.', label_cls: Callable = None, xtra: Any = None, processor: Union[fastai.data_block.PreProcessor, Collection[fastai.data_block.PreProcessor]] = None, x: 'ItemList' = None, ignore_empty: bool = False)
For the same reason I find that I don’t look up the fastai doc API, since I can’t make any sense of these entries. Those are written for computers and are not made for human consumption. It’s possible to eventually understand what such a declaration says, but I instead read tutorials, search forums, grep code, notebooks and tests to understand what the function does.
But, please, please, don’t let my grudges discourage you from innovating and creating amazing things, we are all different and different things work for different people.
I really don’t care if we have 20 different ways, guides, autogenerators, magic potions, and what not as long as the result is that we have a solid test suite and we can make a confident new release at any point in time and not need to let new code to sit and wait for brave early adopters to do the validation instead of having a test suite to do that.
For example we had to recall the last relese because our test suite missed an important regression.
1 Like
Hmm yeah that’s weird that it’s converting it.
Can we try one more thing?
Change this line to:
list_test = [{'file': basename(pathfilename), 'test': test_function_name , 'line': str(lineno_parentfunc)}]
If that doesn’t work, we can definitely handle the current format. Looking good!
1 Like
Yeah makes sense!
I’ll put this in a separate PR. Then I can visit the SF study group as you mentioned. It’d be good to get some feedback on whether writing tests in notebook is helpful at all and whether the PR process is another barrier.
1 Like
this works ! cool, thx !
Anything needed else from me? Some debugs need to go and that parent func call should reference your package
that’d be perfect, indeed ! thx
Haha yeah that generated skeleton was pretty unsightly. I just updated it so that only the required args are shown and added more examples at the very bottom.
I very much agree that the skeletons aren’t helpful at all. I think the benefit is solely for those who use fastai on a remote server. Writing, testing functions, and saving from notebooks is personally easier for noobs like me =)
2 Likes
I think everything looks good! I’ll start integrating with the json file you spit out.
Should we keep it separate for now? I like how your code doesn’t have any dependencies on fastai
1 Like
@ashaw separate is fine.
here my part: https://github.com/fastai/fastai/pull/1634
when the pr is in can go through some tests and register them. this could be a low hanging fruit for others to get involved.
Also, saw this to do from Stas: and remember to add test_api_db.json into MANIFEST.in when the integration is complete. Thanks.
think that matters and is worth trying. the entire MOOC is about making things easy & fast, so why wouldnt this apply here too and make testing a success
And also I recommend testing live methods and not hardcoded classes:
Instead of:
def test_fit_one_cycle(learn):
this_tests(Learner.fit_one_cycle)
this:
def test_fit_one_cycle(learn):
this_tests(learn.fit_one_cycle)
so the function should be able to derive the class.method automatically.
That way you know your testing is always correct.
As I explained who knows how many posts back, if you use Learner.fit_one_cycle, the codebase could be modified to move it to a different class, and now you have a bug in this_tests() since it’s registering the wrong thing and there will be no way to detect this fault.
That’s why I said this_tests() could be called anywhere in the test, as it might need to wait to have the right objects to pass on to this function.
Does it make sense?
And of course the current implementation won’t work for accessors that aren’t methods and there we will have to use an actual string instead - e.g. how do you do test_this(learn.data)?
So please hold your horses on updating any other tests until we agree on this little difference to save editing efforts. Thanks.
1 Like
Sorry, I didn’t pay close attention, but there were many inconsistencies, so I had to refactor those post commit.
Please try to be consistent with your code - thank you!
Another important change: I added fastai/test_api_db.json until this becomes ready to be used, and then it needs to be committed.
Also please, please, make sure that file doesn’t change if the test suite doesn’t change. i.e. it should always generate an identical output (sorted, et. al) if the inputs haven’t changed. Thank you!
1 Like