think its great ! cool. And yes: using real names of instances makes absolutely sense because if names of classes changes, it does not break. It just slipped my mind really. I linked this section also into the testing wiki section here: Improving/Expanding Functional Tests
I was thinking in the general style guide we could add suggestions like
try to use join loop constructions where possible e.g. to generate strings:print(’\n’.join(str(missing_this_test) for missing_this_test in TestAPIRegistry.missing_this_tests))
Use f notation instead of + to build strings: missing_this_test = f"file: {filename} / test: {test_name}"
Was looking for a good place to add such small suggestions - maybe here? Dev Projects Index
So when I click on [test] the browser first focuses on the anchor, shifting the content up, then it opens the test entry which is quite further below. This is a very spastic behavior and I find it extremely difficult to use.
click on [test] next to the create_opt or dl entry below
and you will see what I mean. I had no idea what happened. I had to look for the test entry, which was very complex and intuitive. Let me know if you see what I mean.
Perhaps it should:
not refocus on that anchor and just uncollapse the hidden text - i.e. clicking on [test] should not trigger browser location update
move the [test] anchor where it actually gets opened and not at a distance (i.e. not next to [source], but after or next to the docstring?
Perhaps the collapsed section should go before the docstring? See the snapshot how test breaks the flow of the doc at the moment.
but I realize it probably would make things much complex if we want to inject the collapsed show_test output between the function definition and its docstring. Perhaps doing it before the function definition? not great either.
def get_parent_func(lineno, lines, ignore_missing=False):
"Find any lines where `elt` is called and return the parent test function"
for idx,l in enumerate(reversed(lines[:lineno])):
if re.match(f'\s*def test', l): return (lineno - idx), l # 1 based index for github
if re.match(f'\w+', l): break # top level indent - out of function scope
if ignore_missing: return None
> raise LookupError('Could not find parent function for line:', lineno, lines[:lineno])
E LookupError: ('Could not find parent function for line:', 159, ['"""\n', 'module: basic
[it dumps the whole test module here]
Ideas?
Should we just clean up that Exception not to dump the whole file and give a better error message that this_testscan be called only directly fromtest_foo` functions? This makes it somewhat harder to pass the live api, since the object might be only visible inside the helper function.
Or support nested functions so that inspect could reach up say a frame or two and see if the parent or grandparent are test_foo function?
Had to tweak the logic even more to support this_tests checks in skipped tests via decorator and dynamically from the test, but I think all bases have been covered.
I also introduced a special case of this_tests('skip') for situations where it’s not fastai API or it’s an accessor, which we currently don’t support.
All in all a bit of perl and some manual tweaking and we have now all existing tests covered: 233 test_this calls, 38 of which are skipped.
@ashaw, the docs api only contains callables, correct? i.e. there is no entry for things like Learner.data.
and https://docs.fast.ai/ now contains [test] for each test that we have. Please browse around and see if you detect any problems.
No tests found for mean_squared_error. To contribute a test please refer to [this guide](https://docs.fast.ai/dev/test.html) and [this discussion](https://forums.fast.ai/t/improving-expanding-functional-tests/32929).
So registering with this_tests is done? Otherwise wanted to put that on my agenda to work on it when I find some time (which might be a little slow at times with other obligations)
It could be too confusing in the test, where skip has a special meaning. Probably, should pick another short name instead, but I can’t think of a good word so that it communicates a complex thing.
Basically we are trying to say one of the two:
this tests something that is not a fastai API
this tests a property and it is not callable
Two very unrelated situations to be expressed in one intuitive word.
this_tests('none') is the only thing that comes to mind, but that’s not true either because the test is testing something.
Looking through synonyms: omit looks intuitive, and probably won’t be used as a real function anywhere.
update: ok, I think I sorted it out - I changed it to this_tests('na') na = not applicable, not available, etc. - covers all bases and is short.
Pushed some UI updates to master and rebuilt the docs.
Thanks for your suggestions! Followed as closely as I could and definitely looks much better. Send any more improvement ideas my way =)
Started on supporting property attributes learn.loss_func for show_doc and show_test. It works for now, but there’s a lot of edge cases. It’ll be an ongoing thing.
That’s much better, @ashaw! This definitely works for me UI-wise. Thank you!
Started on supporting property attributes learn.loss_func for show_doc and show_test. It works for now, but there’s a lot of edge cases. It’ll be an ongoing thing.
Hmm, I swear I remember seeing Jeremy calling show_doc in one of the classes and it didn’t look like he needed to import anything. If I grep show_doc now, it’s in none of the course notebooks. Do I mix it with some other function?
That’s why I thought that if show_doc gets imported with fastai.basics then to add show_test there.
Ah, found it, it was doc(func) and it has [test] built in already, so I think that covers it.
