Test Registry project

[ashaw (Andrew)]

Just fixed. It should change the next time docs are built

[stas (Stas Bekman)]

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?

[ashaw (Andrew)]

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"

[Benudek (benedikt herudek)]

If we can do most off this dynamic it’s great, see also my remarks on surfacing the api usages on notebooks (other thread)

[stas (Stas Bekman)]

@ashaw

Makes sense. Pushed an update with the new ordering.
Let me know if you want to remove direct tests alltogether

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.

[ashaw (Andrew)]

@stas

We do have an issue with skipped tests

What if we make this_tests a decorator before @pytest.mark.skip. Would that solve the merging problem?

[Benudek (benedikt herudek)]

we are on a (different) solution path: https://github.com/fastai/fastai/pull/1805

[stas (Stas Bekman)]

What if we make this_tests a decorator before @pytest.mark.skip . Would that solve the merging problem?

No, because

1. some tests skip from inside the test.
2. this_tests needs to have access to objects which often don’t yet exist when entering the test function.

[stas (Stas Bekman)]

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 pytest run 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  => TestRegistry
  

  so now we only need to remember 2 things: this_tests and test registry

Otherwise, everything looks good - contributors already start using this_test, thanks to the alerting feature I’d imagine.

[Benudek (benedikt herudek)]

Great, thx. We need a Stas appreciation thread somewhere for sure!