Case sensitive keys for key value store #90
Conversation
sriram-viswanathan
force-pushed
the
case-sensitive-keys
branch
from
c53f60b to
903fe00
Compare
sriram-viswanathan
force-pushed
the
case-sensitive-keys
branch
from
903fe00 to
9e28fab
Compare
There was a problem hiding this comment.
Overall, this looks pretty good. The tests could use a bit more tightening and cleaning up, but that's about it.
One thing that could help the tests would be to compress them all down into just 2 tests that have variations set via a where block. Something like this:
def "getRowKey #respects case for #param when CASE_SENSITIVE_KEYS_ENABLED is #flagState"() {
setup:
// Do flag setting and capture existing state
expect:
DimStoreKeyUtils.getRowKey(rowName, rowValue) == expectedValue
cleanup:
// Restore flag to captured state
where:
flagState | param | rowName | rowValue | expectedValue
false | rowValue | "foo" | "FOO" | "foo_foo_row_key"
true | rowName | "FOO" | "foo" | "FOO_foo_row_key"
def respects = flagState ? "respects" : "does not respect"
}| DimensionStoreKeyUtils.getColumnKey("FOO") == "foo_column_key" | ||
| } | ||
|
|
||
| def "Test getRowKey() with CASE_SENSITIVE_KEYS_ENABLED on"() { |
There was a problem hiding this comment.
Ideally, test names reflect something about the behavior they are testing / asserting. With that in mind, could we update the test names in this file to be a bit more informative about what condition / behavior they are asserting exists?
For this particular case, it might be something like getRowKey retains the case of the rowName when CASE_SENSITIVE_KEYS_ENABLED is off
| BardFeatureFlag.CASE_SENSITIVE_KEYS.setOn(true) | ||
|
|
||
| expect: | ||
| DimensionStoreKeyUtils.getRowKey("FOO","1") == "FOO_1_row_key" |
There was a problem hiding this comment.
We should add tests like this where the rowValue parameter is a string, just to cover that base as well.
|
|
||
| def "Test getRowKey() with CASE_SENSITIVE_KEYS_ENABLED off"() { | ||
| expect: | ||
| DimensionStoreKeyUtils.getRowKey("FOO","1") == "foo_1_row_key" |
There was a problem hiding this comment.
It would be good to explicitly set the flag to off to ensure that we're testing with the condition we want to test, rather than relying on default config. Not a requirement, but it would be nice.
| DimensionStoreKeyUtils.getColumnKey("FOO") == "FOO_column_key" | ||
|
|
||
| cleanup: | ||
| BardFeatureFlag.CASE_SENSITIVE_KEYS.setOn(false) |
There was a problem hiding this comment.
It would be good to capture the initial state of the flag in the setup block, and then re-set to that initial state in the cleanup block, rather than just force back to false
Note: Applies to the other places we're flipping the flag as well.
|
👍 |
Fix for #89