rschmitt commented on PR #653: URL: https://github.com/apache/httpcomponents-client/pull/653#issuecomment-2988627072
That's not _too_ different from what we're doing now, though, but I have a few observations: 1. We can't set all the other timeouts to one minute. In order to test lower-precedence timeouts, the higher-precedence timeouts have to be unset. Otherwise the `ResponseTimeout` will always win. 2. The specific values are much lower than the ones you suggested because I hate slow tests. I want them to succeed as fast as possible, and ideally to fail quickly as well in the event of a regression. Ideally I'd like to add an integration test for the _default_ socket timeout (which is one minute), but that would more than double the time it takes to run `./mvnw verify`. Hence, I haven't added such a test, even though I feel it's very important that the client in its default configuration never hang indefinitely when making a call. 3. One thing I like about the stricter upper bound (stricter than "less than the far longer timeouts we configured") is that it can detect other issues, such as changes in internal retry behavior, or the blocking `SSLSocket` close behavior on JDK11 that was detected by `TestTlsHandshakeTimeout`. I'd rather be able to detect these things and then decide whether we want to ignore them or not than ignore them automatically because our assertions are so lax. There's a balance here that has to be maintained over time: we want our tests to be _sensitive_, but not _flaky_. (Flaky tests should not be tolerated.) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org For additional commands, e-mail: dev-h...@hc.apache.org
