AvailableConnectionManagerConcurrentTest is passing after changing it to use plain java coded stubs (no Mockito) which required some changes to Connection and PooledConnection (internal classes). The other ConcurrentTests are passing on the Mockito upgrade PR branch.
Locally, AvailableConnectionManagerConcurrentTest was taking 10+ minutes to run, while in the cloud it was causing IntegrationTest to hang and/or hit OOME. After changing it to use plain java coded stubs it passes locally in 11 seconds. The Cache.close() branch is unrelated. I mistook the 10+ minute run of AvailableConnectionManagerConcurrentTest to indicate that it the ConcurrentTest run a looong time even on develop (but they don't). The hang on that branch is also in IntegrationTest but it's caused by reentrant calls to Cache.close(). After Jake's PR is merged to develop, I'll rebase the Mockito upgrade on develop -- so far it looks good after these latest changes for AvailableConnectionManagerConcurrentTest. On Thu, Apr 16, 2020 at 4:24 PM Dan Smith <dsm...@pivotal.io> wrote: > I like the idea of a separate concurrency test suite. We have other tests > of concurrency that aren't using this runner that could also go there - > maybe we could establish some good conventions and figure out how to give > more CPU time to these tests. I'd actually like to see *more* tests of > concurrency at a lower level, so we can catch race conditions sooner. > > > Should I disable these tests? > > I don't think we should be marking any tests as ignored. Let's figure out > what is going on and make the appropriate fixes. > > Can you clarify why these tests are blocking your Cache.close PR? Also, > which tests are getting an OOME with the mockito PR? > > For a little clarity on the ConcurrentTestRunner, it just runs a test with > multiple threads some number of times. For example > > FilterProfileConcurrencyTest.serializationOfFilterProfileWithConcurrentUpdateShouldSucceed > is testing a bug some of our customers hit where serializing an object that > was being concurrently modified failed. I think there is definitely value > in having those sorts of tests given the type of code we have - a lot of > classes that are mutable and need to be thread safe. > > -Dan > > > On Thu, Apr 16, 2020 at 2:47 PM Jacob Barrett <jbarr...@pivotal.io> wrote: > > > > > > > > On Apr 16, 2020, at 2:16 PM, Kirk Lund <kl...@apache.org> wrote: > > > > > Anyone else up for moving them to new src sets? Unfortunately, this > alone > > > won't make the tests work with the newer versions of Mockito -- and > guess > > > what, they only OOME in the cloud (not locally). > > > > I could take a stab at it since I split up all the test types originally. > > I should be able to brush off those cobwebs. > > > > > Should I disable these tests? > > > > In an effort to get the Mockito stuff through, yes disable them. > > > > > Should I remove the use of ConcurrentTestRunner from these tests > > > (converting them to simple junit tests)? > > > > > > I spent some time studying ConcurrentTestRunner and I'm sorry, but I > > don't > > > really see the value in this runner, which is why I want to remove the > > use > > > of it and convert these tests to plain junit tests. > > > > They have less value as unit tests since there are already unit tests > that > > cover them. It does raise a good questions about the value and > > implementation of micro concurrent testing like this. I think isolating > > them away is a good step before starting this conversation. > > > > > > >
