Any thoughts on naming Direct tests?
Or should I just put them all under the Other tests where x is used umbrella
Meh for ‘Direct tests’, it’s ambiguous - the first group are direct tests too.
How about we start with an explicit ‘Other …’ and then improve down the road if someone finds a word to encapsulate this long phrase.
Moreover this is just a sample of those tests, e.g. fit_one_cycle is used probably in 50+ tests. So perhaps not to mislead again we use:
Some other tests where x is used
I know I made it even longer, but a lawyer would be proud of the choice! 
1 Like
The cool thing about the test registry is its unintentional features - I already found several tests that were testing the same thing in slightly different ways, so I merged them and as a result discovered a bug in fastai and a bug in our registry. Awesome!
1 Like
Nice!! Useful already
To clarify, we should combine both Direct tests: and Related tests: into:
Some other tests where x is used:
Right?
yeeah, I like it and click around on the test button too. awesome work @ashaw!
Mostly as it serves a double purpose: testing is for testing and can serve as documentation.
I do wonder, if there will be more PRs on testing - crossing fingers!
1 Like
Oh, sorry, I thought you were suggesting ‘Direct tests’ as a better name - I misunderstood you.
What are ‘direct tests’?
Looking at DataBunch - these are tests called on the object of the class that one may want to test, right?
Yeah none of it makes sense. I’ll just loop all of them into other.
Direct tests:
Looks to see if any def test_xxx: names contain the function name
test_DataBunch_xxx: -> matches DataBunch
test_DataBunch_oneitem -> matches DataBunch and DataBunch.oneitem
Related tests:
any time the function is called in the body
1 Like
since function names can be random and worse “misnamed” probably yes the best is to lump them into “other”.
4 posts were merged into an existing topic: Feasibility Study - Notebook API Demo Registry to link into fast.ai doc
Just fixed. It should change the next time docs are built
1 Like
As I mentioned earlier, I’m not sure direct tests will always produce correct results and at times will send a user to a totally unrelated test, just because a test name happened to include a string that happened to be a function name. So I question whether those should be included at all.
So I’d say at the very least put the related first, and direct last to increase relevance. Currently, it uses set, so the order will be lost and the following code from the last commit won’t do that:
other_tests = [get_links(t) for t in (direct+related)]
other_tests = list(set(other_tests) - set(db_matches))
My suggestion is that since we do manual curation, let’s stick to showing less, but correct things, than more of maybe things, especially since one of the main purposes is to ask for more tests. So showing more isn’t necessarily a good thing, but this is not the issue.
As I’m writing this I’m realizing that direct tests shouldn’t be there in first place, since if we say “Other tests where x is used” we have no idea whether this is so for direct tests. If you can validate that the API is indeed used in test’s code and not as a substring or a comment, then it’s a good fit for “others”, but if it’s not, then I’d say it has little to no value.
But perhaps I’m missing something, what value do you think direct tests could provide over manually curated tests plus other tests based on their API?
1 Like
Makes sense. Pushed an update with the new ordering.
Let me know if you want to remove direct tests alltogether
The idea behind “direct tests” is the hope that you won’t have to manually enter “this_tests” and it will show up automatically - assuming the function is named right.
Something like: def test_DataBunch_create_and_oneitem - would show up for DataBunch, DataBunch.create and DataBunch.oneitem, but not for DataLoader.Create.
If it’s done correctly, we could remove all the 'this_tests"
1 Like
If we can do most off this dynamic it’s great, see also my remarks on surfacing the api usages on notebooks (other thread)
Yes, please.
The idea behind “direct tests” is the hope that you won’t have to manually enter “this_tests” and it will show up automatically - assuming the function is named right.
Something like: def test_DataBunch_create_and_oneitem - would show up for DataBunch,
DataBunch.create and DataBunch.oneitem, but not for DataLoader.Create.
If it’s done correctly, we could remove all the 'this_tests"
Absolutely not. This will not work with identical method names from different classes. Plus many tests have multiple this_tests calls and multiple arguments, and often the test name has little to do with the API it tests, this happens when a certain scenario is tested and not just an API. Therefore, let’s not mix functionalities, so that tests can be named intuitively and not interfere with test registry needs.
And currently we require each test to have this_tests (albeit non-fatal). I personally don’t mind adding those calls, which helps me to get to know new tests that others write, if they didn’t do it already.
1 Like
What if we make this_tests a decorator before @pytest.mark.skip. Would that solve the merging problem?
What if we make
this_testsa decorator before@pytest.mark.skip. Would that solve the merging problem?
No, because
- some tests skip from inside the test.
- this_tests needs to have access to objects which often don’t yet exist when entering the test function.
1 Like
A couple of updates:
-
merge_registries has been implemented with tests. This now allows us to run the registry update any time, and no more flags or conditions are needed. I’ve removed all conditions, so any
pytestrun w/ a single or multiple tests will update the registry. I will be monitoring if it creates any unnecessary burden for devs.Every so often we should nuke the old registry, to get rid of any stale info.
-
I did a bunch of renames for consistency and ease of remembering, the main ones are:
test_api_db.json => test_registry.json TestAPIRegistry => TestRegistryso now we only need to remember 2 things:
this_testsand test registry
Otherwise, everything looks good - contributors already start using this_test, thanks to the alerting feature I’d imagine.
2 Likes
Great, thx. We need a Stas appreciation thread somewhere for sure!
1 Like