Closed wyn closed 1 year ago
Merging #2247 (fa126aa) into main (1f046dd) will increase coverage by
0.02%
. The diff coverage isn/a
.:exclamation: Current head fa126aa differs from pull request most recent head 97b2dc4. Consider uploading reports for the commit 97b2dc4 to get more accurate results
:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
@@ Coverage Diff @@
## main #2247 +/- ##
==========================================
+ Coverage 68.14% 68.16% +0.02%
==========================================
Files 137 137
Lines 16669 16669
==========================================
+ Hits 11359 11363 +4
+ Misses 5310 5306 -4
see 2 files with indirect coverage changes
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
You're right that Invalid_depth
only applies to inodes in irmin-pack
. I don't have full context on the original purpose for introducing this exception (@samoht perhaps you can weigh in), but my intuition is that it is to protect against wrong data that has been written to disk. If that is true, I see two paths for triggering the exception:
irmin-pack
store that has wrong data. This seems difficult since the current code creates correct data on disk.Pack_store.unsafe_find
for some inode. I'm not sure how difficult this will be, but these lines are where you might start to do something like this. Ultimately, I would expect these lines to raise the exception and be the entry point for tests.As for testing concrete case, I would look into using the concrete type to create a representation that has a node with the wrong depth and then call of_concrete
with your crafted representation. I don't see a need to add ?depth
to the function if you have a concrete representation with wrong data -- I would expect it to trigger the exception (and return the equivalent error) when calling of_concrete
.
(also, no changelog is needed for this kind of change so I added a label to skip the CI check)
@metanivek I had a go yesterday with your suggestions - hopefully I got the right idea?
I had to change the test Context module into a functor to allow to switch between the Inode and Inode_mock,
I wondered about doing the same thing for Inode_permutations_generator but settled on passing in the appropriate Inter.Val.index function instead - maybe it's cleaner to turn this into a functor too?
closes #1668
@metanivek thanks v much for the review - I've just pushed up those changes you suggested, for the second one I went with the first class module approach.
NOTE - I have had a go at this issue but am not sure its the correct approach hence I havent updated the CHANGES yet.
It looks like the Invalid_depth exception is raised in two places, on Inode's
Internal.of_concrete_exn
and also when doing afind
(or more specificallyunsafe_find
).of_concrete_exn (included in this PR): the Inode_intf.Internal.of_concrete type has been changed to include an optional
?depth=0
parameter. This allows to call it with the wrong depth which then surfaces theInvalid_depth
exception. My worry is that this is a bit too heavy handed and exposes the wrong API / type forof_concrete
? Also, if changed here should it be changed for otherof_concrete
funcs e.g. forPortable.Proof.of_concrete
?Inode.unsafe_find (not included in this PR): I was looking into this today and (inspired by
test/irmin-pack/test_inode.test_intermediate_inode_as_root
) I have been trying the strategy of usingInode.unsafe_find
at an incorrect depth in order to try and get the Invalid_depth exception to surface. The problem is that this exception is first converted into anError ...
type (c.f. here which is then swallowed inInode.integrity_check
) because I guess the values that are constructed in the test cases are such that these integrity checks pass.So I was wondering