Test Registry merge new with existing

[Benudek]

wrt https://forums.fast.ai/t/test-registry-project/38344/216

@stas00 If there is value & a way to make merge_with_old_registry even more compact (e.g. the loops, if then) happy to look at it.

For the rest, functionally: passed the local tests I was able to come up with ;-)

[stas00]

It looks good, but I don't have a way to test whether it works correctly or not, other than making a whole bunch of changes to tests, re-running them multiple-times, and hope I didn't miss anything - very time consuming and error-prone.

Can you please add a test that validates its functionality?

i.e.

1. extract the merging of 2 dicts functionality into its own function
2. write a new test that merges 2 dicts using that new function (simple case and edge cases) and validates that it does the right thing
3. use that function

Thank you.

p.s. that's how I'd have approached this task - first created a merging dict function, while writing a test that checks that I made no mistakes, then used it in the code.

[Benudek]

Stas, makes sense :-)

However - on 2nd look and 3rd look: it looks a little hairy to write tests for this. e.g. setting up data, but also I might need to rewrite the code a little (are you suggesting this with (1)?) to be able calling functions independently.

I wonder if we can be a little 'pragmatic' here it's a utility function and we have some more-core function without tests ;-) If it fails in 'production' as far as I can see it wouldn't be a 'biggie' and we could fix then based on the edge case detected, that right now I couldn't predict.

So, what I mainly did is below - and yes it's time-consuming ( I always re-ran all tests to try breaking things) and likely not covering all edge cases:

1. delete the file test_api_db.json
2. run pytest -sv --testapireg

NOW: after that the merge function will call

3a. open file in editor and take a random entry and overwrite the entry

_FROM "fastai.basic_data.DataBunch.create": [
{
"file": "tests/test_basic_data.py",
"line": 19,
"test": "test_DataBunch_Create"
},
{
"file": "tests/test_basic_data.py",
"line": 33,
"test": "test_DataBunch_no_valid_dl"
}
],

TO "fastai.basic_data.DataBunch.create": [
{
"file": "DUMMY",
"line": 19,
"test": "test_DataBunch_Create"
},
{
"file": "tests/test_basic_data.py",
"line": 33,
"test": "test_DataBunch_no_valid_dl"
}
],_

EXPECTATION: when rerunning

4a. run pytest -sv --testapireg

I want to see both: the DUMMY entry and tests/test_basic_data.py

I also changed other lines here with the same expectation and result.

This tests the if-part of the function

if func_fq_key in old_api_tests_map:
for new_entry in new_entries:
if new_entry not in old_api_tests_map[func_fq_key]:
old_api_tests_map[func_fq_key].append(new_entry)
else:
old_api_tests_map[func_fq_key] = new_entries

The else part is tested here

3b. open file in editor and take out all of "fastai.basic_data.DataBunch.create":
4b. run pytest -sv --testapireg

I expect to see the entire entry from above see FROM.

Where we see huge gaps here, we could run some more tests manually - could that help?

[stas00]

That's why I said, extract the functionality into a function that is totally unrelated to test registry:

def merge_dicts(old, new):
[...]
return merged

test:

old = { a: [1, 2, 3], b: [4, 5, 6])
new = { a: [11, 12, 13], c: [9])
expected = { a: [11, 12, 13], b: [4, 5, 6], c: [9])
merged = merge_dicts(old, new)
assert expected == merged

done.
this is untested. just showing what I meant.

of course adjust old/new to match our needs - i.e. mimic the same dict structure.

[Benudek]

understand, makes sense.

Unfortunately I wont get to this any time soon :-(

So need to close this for now.

[stas00]

No problem, I will sort it out.

I also wanted to communicate to you that I find it very difficult to work with you, @Benudek.

I trust you will find someone else with a better character compatibility to continue forward.

[stas00]

update: I implemented all this functionality including tests, so nothing else needs to be done.

[Benudek]

Hhm, no issues here at all and sorry to hear that. I am trying to give my constructive professional view (so I think that's fair, not intending making things difficult :-)) but certainly have a limited timebudget.

So I agree, we should not repeat this PR process in this setting.

Thx for the support here in any case and making this happen!