MST Test Suite integration #42
Conversation
|
Note to self, I should probably add something to the README about how to init the submodule |
arroba/tests/mst_test_suite.py
Outdated
| return car_header["roots"][0] | ||
|
|
||
| def test_diffs(self): | ||
| for testname, testcase in tqdm(self.diff_testcases.items()): |
There was a problem hiding this comment.
oh, and I used tqdm for a progress bar - not super necessary. I should either take it out or add it as an optional dependency
arroba/tests/mst_test_suite.py
Outdated
| reference_cid_set = set(x[0] for x in reference_blocks) # just look at the cids from the car | ||
|
|
||
| self.assertEqual(root_b, reference_root, f"{testname} inverse: new root") # fails occasionally | ||
| self.assertEqual(diff.new_cids, reference_cid_set, f"{testname} inverse: new cid set") # basically always fails, I think I'm doing something wrong |
There was a problem hiding this comment.
the gist of this test case is: take mst_a, apply the list of ops to it, and compare the result to mst_b. The list of CIDs I'm getting from null_diff(mst).new_cids looks completely different to what I expect, so I suspect I'm doing something wrong here (or maybe null_diff() is just very broken heh)
There was a problem hiding this comment.
Hah. Both are possible I guess! arroba's diff code is used in production, but definitely not as broadly as it could be, and not all parts, so big bugs are very possible.
There was a problem hiding this comment.
This is awesome! Really psyched for this test suite. Thank you so much for putting it together, and for this PR! Especially appreciate you seeing and using arroba-idiomatic parts like testutil.TestCase.
Minor nit, you'll probably want to use with self.subTest(...) for the individual test cases to get unittest to see and run and count them individually. Alternatively you can generate them programmatically, eg https://github.com/snarfed/granary/blob/c29dd723f8b82ca5874f2122de64b2e9cd2c1fd7/granary/tests/test_testdata.py#L144-L156 , but that's more complicated and less idiomatic.
I'll also mention making the test suite pip-installable just for the record here, even just from GitHub if not pypi. Submodule is ok, pip package is definitely nicer.
Regardless, this is exciting!
|
Oh and I like separating it out from |
|
ah yes, subTest is what I was looking for. Thanks for the feedback! Current stats: (some of these are plausibly bugs in the test cases themselves, since I generated them with my own code which has low test coverage itself!) |
|
Thanks! I'm happy to merge this whenever. Lots more to do, sure, but it's contained and harmless and a great start as is. Up to you though! |
|
Cool, feel free to merge it now. When I add new tests it should just be a case of updating the submodule, and if I add new test types I can PR those in too. I'll think about making it a pip-installable package in the future, but submodule definitely works for now. I just bumped the submodule version, no changes to the tests themselves but it includes my visualisation script. |
|
Thanks again, this is so great! |
|
[cracks knuckles] time to get to work |
The tests take ~30 seconds to run on my M1 pro macbook (with failing asserts commented out) - not too bad, but I anticipate adding a lot more tests to the suite.
Due to the slowness, I deliberately named the test file such that
python -m unittest discoverwon't find it. It can be manually invoked viapython -m unittest arroba.tests.mst_test_suite.The test suite itself is still in a state of flux, hence this is a draft PR for now.
Currently, it only tests basic MST diffing (detection of created, updated, deleted records, and new and deleted MST blocks)
Unfortunately the tests currently don't pass -
Diff.new_cidsis flakey (which you note in one of your existing diff tests). Everything else seems good though! (due to the symmetry of diffing, you could in theory compute the reverse-diff and takeDiff.removed_cidsto get a correct result - but presumably you want to fix it properly heh)