Repository navigation
test_minidump_size_limit occasionally fails #69
Description
The test fails sporadically on my local system (due to small differences). Based on the comments in the test, this isn't anything new.
I'm wondering why the logic/assertion here isn't simply assert!(meta.len() <= minidump_size_limit). As far as I can tell that's what is logically being tested, and there isn't really any way to guarantee (without significantly more effort, I expect) that the minidumps are the same size (with an error tolerance). Alternatively, I don't see much harm in increasing the error tolerance such that it's much less likely to fail.
Activity
gabrielesvelto commentedon Jan 9, 2023
Yes, I think that relaxing the assertion would be the way to go here. Enforcing a hard limit on the minidump size is too much of an effort for little value, this is really a soft limit.
Jake-Shadle commentedon Jan 9, 2023
Yah that has been problematic in the past it's fine to relax it.
afranchuk commentedon Jan 9, 2023
So to clarify, do we want to
assert!(meta.len() <= minidump_size_limit)- this is fairly permissive, given that it's allowing the minidump an extra 2MB over the prior run, and realistically wouldn't sporadically fail, orgabrielesvelto commentedon Jan 10, 2023
If we want to test the size limit then I'd say to go for 1. but maybe with a smaller value than the previous run? Say we set
minidump_size_limitto 3/4 ofnormal_file_sizeand then assert that the minidump is indeed smaller thannormal_file_size? Since it's a soft limit we just need to check that it's being enforced, not that it's yielding exact results.afranchuk commentedon Jan 10, 2023
So we do test that. The unit test both tests that a larger limit won't be triggered and a smaller limit will. This issue is specifically with regard to the larger limit case (which makes it all the more frustrating that it sporadically fails, since logically it's a simpler case 😄). I will adjust the larger limit case accordingly. Now that I think more about it, I suppose the test assertions were doing what they were doing in some attempt to show that the limit wasn't triggered. But I think that's fairly difficult to check without explicit signalling. So in that regard, removing this whole case would probably be fine too...