rschmitt commented on PR #644: URL: https://github.com/apache/httpcomponents-core/pull/644#issuecomment-3977564053
Some early thoughts: 1. These tests add about 35 seconds of execution time and 10 seconds of wall-clock time on my machine. 2. Several of these test classes do not run by default in any Maven profile. 3. They did not uncover any new bugs. 4. They did not fail when I tried running them with my various connection pool fixes reverted. `assertCoreInvariants` doesn't assert that `getMaxTotal()` returns a non-negative value; the result of `lease` and the pool stats are apparently not compared to expected values; etc. 5. Running them in IntelliJ Profiler, it's clear from the timeline view that the tests spend most of their time sleeping. 6. The fuzzer tests are single-threaded, so they won't catch concurrency bugs. 7. There is no test coverage for `H2SharingConnPool`. A key issue here appears to be the 200ms connection request timeout in `ConnPoolContractFuzzer::nudge`. About 99% of the total test runtime is spent waiting for this timeout to fire. This has to be fixed in order to make progress on these tests. I'm pretty sure the `lease` method always returns a completed `Future` any time it's not at the max conns limit, so for single-threaded testing you can literally use a timeout of `0`. (I just tried it locally and it dropped the single-threaded test runtime from 30.4 seconds to 92 ms.) For multi-threaded testing things are more complicated, but we'll cross that bridge when we come to it. Another issue is the dependency on the system clock via the static JDK methods `Thread.sleep()` and `System.currentTimeMillis()`. Probably the very first thing I would do here is refactor the ConnPool classes for testability by replacing the calls to `System::currentTimeMillis` with calls to `java.time.Clock::millis`. We can then inject a test double and take direct and deterministic control over the clock for testing purposes. This will let us fix the sleeps in the _existing_ unit tests, before we add any new test coverage. If necessary, we can also add internal ConnPool test methods to support things like synchronization barriers, counters, callbacks, and whatever else we need in order to write fast, deterministic, exhaustive tests. But let's take this one step at a time; this should be done as a whole series of smaller PRs, rather than 1,200 lines all at once. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
