Change test harness default to Mongo and reduce integration test runtime #383
Conversation
Did you mean to commit these files?
|
agavra
left a comment
agavra
left a comment
There was a problem hiding this comment.
I'm a little confused about this PR, it seems like we're changing the default to run with Mongo but having all our integration tests specifically test Cassandra? I feel like they should be aligned to make sure we're testing our defaults
| public static final String STORAGE_BACKEND_TYPE_CONFIG = "responsive.storage.backend.type"; | ||
| private static final String STORAGE_BACKEND_TYPE_DOC = "The storage backend"; | ||
| private static final StorageBackend STORAGE_BACKEND_TYPE_DEFAULT = StorageBackend.CASSANDRA; | ||
| private static final StorageBackend STORAGE_BACKEND_TYPE_DEFAULT = StorageBackend.MONGO_DB; |
There was a problem hiding this comment.
oh I'm a little anxious about this, not sure whether or not existing customers set this explicitly or not. please check with them first!
I originally thought you were just going to change the default in the test configurations.
There was a problem hiding this comment.
ah sorry, good call. We should check with them though because I think we do want to shift to Mongo as the default (or do we?)
I'll extract this out into a separate PR that we can merge if/when we're ready. Does it make sense to change the default for the tests before we change the default itself?
| }; | ||
|
|
||
| private final CassandraClientFactory mockCassandryFactory = new CassandraClientFactory() { | ||
| private final CassandraClientFactory mockCassandraFactory = new CassandraClientFactory() { |
| final RemoteKVTable<?> table; | ||
| if (EXTENSION.backend == StorageBackend.CASSANDRA) { | ||
|
|
||
| if (type == KVSchema.FACT) { |
There was a problem hiding this comment.
it looks like we're removing the ability to test with cassandra and instead tying that to the KVSchema type? Am I reading that correctly?
There was a problem hiding this comment.
Just for this specific test (sorry I meant to leave a comment to clarify before you read this)
My thought was, since we still have at least one customer running Cassandra fact stores then it makes sense to keep some tests parametrized to run that. But no one is running Cassandra KV stores, we would recommend Mongo for that, so we might as well change this to run the KV schema variation against mongo.
Is the method of parametrizing confusing though? I can change it so that the storage backend is the parameter and we determine the schema type based on that.
|
|
||
| properties.put(RESPONSIVE_ORG_CONFIG, "responsive"); | ||
| properties.put(RESPONSIVE_ENV_CONFIG, "test"); | ||
| properties.put(ResponsiveConfig.STORAGE_BACKEND_TYPE_CONFIG, StorageBackend.CASSANDRA.name()); |
There was a problem hiding this comment.
I thought the point of this PR is to change our tests to run with Mongo?
There was a problem hiding this comment.
Some of them are Cassandra-specific or rely on a mocked Cassandra client to do something like fault injection, as is the case here. I went through and left a note on all the other tests that I hardcoded to use the Cassandra test harness
This specific test actually isn't using the test harness and only had to be updated because I changed the default. Since I'm backing that change out this test actually doesn't need to be updated yet anyways, and I'll back out this change too (so that we remember to update this test if/when we do change the actual default)
| public class TablePartitionerIntegrationTest { | ||
|
|
||
| @RegisterExtension | ||
| static ResponsiveExtension EXTENSION = new ResponsiveExtension(StorageBackend.CASSANDRA); |
There was a problem hiding this comment.
The table partitioner is an exclusively cassandra concept
| class CassandraFactTableIntegrationTest { | ||
|
|
||
| @RegisterExtension | ||
| static ResponsiveExtension EXTENSION = new ResponsiveExtension(StorageBackend.CASSANDRA); |
There was a problem hiding this comment.
Obviously this test is cassandra specific 😄 (ditto the one below)
| public class CommitBufferTest { | ||
|
|
||
| @RegisterExtension | ||
| static ResponsiveExtension EXTENSION = new ResponsiveExtension(StorageBackend.CASSANDRA); |
There was a problem hiding this comment.
We should eventually migrate this test to Mongo but that will be a pretty huge shift, so imo it makes sense to leave this one as Cassandra for now since it's pretty much hardcoded into the test setup
ableegoldman
changed the title
| public class GlobalStoreIntegrationTest { | ||
|
|
||
| @RegisterExtension | ||
| static ResponsiveExtension EXTENSION = new ResponsiveExtension(StorageBackend.CASSANDRA); |
There was a problem hiding this comment.
global stores are incompatible with Mongo(I guess we should fix this...?)
Changes the default storage backend type to Mongo in the ResponsiveExtension test harness
Also tries to reduce testing runtime in two ways: