test_minidump_size_limit occasionally fails

[afranchuk]

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.

[gabrielesvelto]

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]

Yah that has been problematic in the past it's fine to relax it.

[afranchuk]

So to clarify, do we want to

1. 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, or
2. continue to instead assert that the minidump size is approximately the same as the prior run, but with a more tolerant setting of "approximately"?

[gabrielesvelto]

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_limit to 3/4 of normal_file_size and then assert that the minidump is indeed smaller than normal_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]

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...