Test Registry project


(Stas Bekman) #171

It’s a way, but it sounds a way more complicated. I think my proposed solution is much simpler. Except, probably compile a list of the module.py::tests that lack this_tests and print them all out at once at the very end of the test suite, rather than one by one.


(benedikt herudek) #172

ok, will do asap into a small PR


(Andrew) #173

Sorry I think I missed something. Can you remind me what’s still needed from the docs side?
Just need to include the test_api_db.json right?
Otherwise, show_test should be working right now


(benedikt herudek) #174

@ashaw I guess you just need to integrate it yes. ues make test-full to get the file test_api_db.json

here the latest PR- assuming @stas will merge it: https://github.com/fastai/fastai/pull/1696


(Stas Bekman) #175

Can you remind me what’s still needed from the docs side?

The integration into the API docs. so that besides [Source], there will be [Test] link that will show show_test info on click - i.e. users don’t need to type it. Yet, we don’t want it in plain docs by default - it’ll be too noisy and irrelevant to most - so a layer pop-up perhaps or an expansion? but any other way is fine too.

So the doc update will include:

make test-full   # generates the json file
tools/build-docs -f # uses the info from the json file

I will later make it into a script or Make target, so it’s one command


(benedikt herudek) #176

Existing tests to register with this_tests

run pytest -sv --testapireg to generate new report

@stas I won’t find time to continue with the current level of effort and suggest we define registering with this_tests as low hanging fruit. Iif there is sth special to be aware of when calling this_tests we open a separate thread, where we give detailed instructions how-to so the PR submission will be smooth.

I might find lets say 15 min per weekday, like one class - so after a while should be done then too but prefer some help :wink: Ideally imho this is available before fast.ai part II starts, maybe Jeremy could briefly demo it, so everyone knows this API exists and maybe even contributes tests.

Let me know your thoughts, how we can best finish this

@ashaw


Improving/Expanding Functional Tests
(benedikt herudek) #178

not as a link? Think a link in the regilar docs to the git function is helpful and not too noisy ? @ashaw


(Stas Bekman) #179

As a link, yes, so that we have [Test], next to [Source]. I meant not to dump the output of show_test unless [Test] is clicked on.


(Stas Bekman) #180

don’t worry, we will finish it - there is no rush.

But please remember that if you add this_tests, most of the time it should be live objects, not class.method arguments. You can move this_tests to any place in the test to get to the live object.

And another request - don’t submit a PR until the whole test module is finished - e.g. your last PR left last test out.

Thank you.


(benedikt herudek) #181

suggesting guidelines here: Improving/Expanding Functional Tests

It’s easy to forget, helpful to understand reasons - will lead to smooth PRs :wink:


(Stas Bekman) #182

Correct. And you’re in the best position to edit that wiki post, since you now understand what the guidelines are.

And if you are not sure - you can ask for clarification and then you will gain better understanding and know how to edit those :slight_smile:

Does it make sense?


(Stas Bekman) #183

I made the initial doc of this feature for developers:
https://docs.fast.ai/dev/test.html#test-registry

Please let me know if I missed anything.

Thank you.


(Stas Bekman) #184

@Benudek, after having the experience of managing a few of your PR submissions, I felt something wasn’t right and so I checked in with Jeremy to get clarifications about the code style requirements. Jeremy clarified that fastai tries to be as flexible as possible with supporting different coding styles, and so unless it’s listed in the style guide anything goes as long as it’s reasonable and makes common sense. You can see here my attempt to codify what I thought was the style in my post here.

So now I see that several times I was unnecessarily asking you to comply with the style rules that I thought were so, while they weren’t so in reality. I apologize for doing that. I was acting on my own story and not the reality.

Henceforth, I will let the future PR submissions keep whatever style they are in as long as they comply with the actual style guide, which is much less strict than how I like my code to be and then rewrite things if I feel they can be improved in readability and quality.


(Andrew) #185

Ahh thanks for the reminder. Totally forgot why the whole point of this feature :man_facepalming:

Couldn’t decide on the UI or what it should look like. Have any thoughts?
Modal seems a bit more clear, but it completely fails if you click the link from a notebook.

Default view:

Expanded card view:

Modal view:


(Stas Bekman) #186

Looking good, @ashaw!

My vote is for simplicity, and we can always improve it down the road. so my vote is for whatever works everywhere.

Moreover, while the layer pop-up is neat, it also obstructs the normal page, so I think the simple solution is just perfect as it works everywhere (right?) and it lets the user choose whether to look at the test info or look at other info near by.

Perhaps a collapse button to hide the expansion if it is no longer desired?


(benedikt herudek) #187

Anything you suggested Stas was better than what I proposed! Serious, no doubt,no worries at all :slight_smile: thx for the learning curve


(benedikt herudek) #188

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


(Stas Bekman) #189

Heh, except there is an even simpler way, have a look at the last incarnation. you just print the variable as *var, no need even for join there.

Anywhere you feel is a good place to compile such suggestions is good. except not in project index - its purpose is just that - index.


(Stas Bekman) #190

@ashaw, would it be possible to change the current click on [test] UI functionality?

Currently we have:

<a class="source_link" data-toggle="collapse" data-target="#DataBunch-pytest" style="float:right; padding-right:10px">[test]</a>

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.

For example:

  1. click on: https://docs.fast.ai/basic_train.html#mixup
  2. 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:

  1. not refocus on that anchor and just uncollapse the hidden text - i.e. clicking on [test] should not trigger browser location update
  2. 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.


(Stas Bekman) #191

What do we do in case where test_foo calls a helper function (e.g. wrapper) that calls this_tests? Currently it blows up badly.

For example tests/test_basic_train.py:

def test_memory(data):
    subtest_destroy_mem(data)
[...]    
def subtest_destroy_mem(data):
    this_tests(learn.destroy)

Fails in:

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?