DavidSpickett wrote: > The test should check that changing the timeout setting does have an effect.
My most general reading of the test name `test_http_timeout` is that it tests that there is a timeout at all, and setting the timeout to 1 second is purely to make the test faster. However, reading https://github.com/llvm/llvm-project/pull/192061 I see that the intent is to make it configurable so yes it is implied that this test case checks that it is configurable. So that was the intent, I agree. I do not think it actually achieves this though because going by my theory, a timeout of 1 second can become 2, 3, 4, or 4.9999999 seconds depending on process scheduling. And the test does not test the default timeout value, so it's not comparing one to the other. This is going to sound flippant but I'm not trying to disparage @weliveindetail here, it's simply very difficult to do this in a comprehensive way that isn't prone to flakes. If we really wanted to check that: * There is a timeout applied. * That timeout can be changed. * When the timeout is changed, the point at which we timeout changes with it. * The points at which we timeout are close to the actual value of the timeout setting that is being used. We would need to: * Run this test with plenty of spare resources, to make it 99.9% likely that the client checks the timeout often enough. * Run with timeout A several times to get some bounds on the actual time we close the connection and do all the lldb wrapper code involved in reporting it. * Run with timeout B (where B is an order of magnitude less or more than A) and repeat the same thing. * Assert that there was a significant difference between the average timeout point using A and using B. * Assert that A and B's actual point of timeout was within some bounds of A and B. What those bounds are would have to be hand tuned somehow or derived from measurements of a non-timeout scenario so we know how long lldb code generally takes to run, or a guess. Another guess that we could get wrong, though the more lightly loaded the machine is, the more chance we have to get it right. The first step we simply cannot do with llvm-lit as it stands. We do not have a mechanism to say this test is special and needs the machine to be quiet. See also https://github.com/llvm/llvm-project/issues/209874 where we are also expecting determinism from a non-deterministic scheduler. So until such a solution exists, I think what I'm proposing here is a good stopgap. If someone wants to make the test really embody the original intent, fine, but I'm not going to do that, and my alternative is disabling the test which means it will never get run (unless someone adds ELF support for symstore, at which point it's likely to flake on Linux too). > What about setting the wait to 40s or 50s? That could still hit the previous > error, but it should be much less likely and arguably a bug. True, but it's still too likely for my liking. I've dealt with other tests where I added minutes long timeouts and operating systems still managed to delay it enough. You just end up chasing higher and higher timeouts and I don't think it's good practice. And even if it's 0.01% flakes, someone has to look at the flake and decide if it's real. Well, if we're all good upstanding engineers that is. What's more likely to happen is it gets disabled again and we're back to zero coverage. I appreciate you putting forward the other solution but I just don't think it'll lead to anything but a worse scenario than what I'm proposing here. https://github.com/llvm/llvm-project/pull/221196 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
