-
Notifications
You must be signed in to change notification settings - Fork 875
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor integration tests to support multiple C* distributions #1958
base: 4.x
Are you sure you want to change the base?
Changes from 2 commits
2da861d
c58cad0
1ec720f
8ad5567
a054e8d
b08c01a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -598,7 +598,7 @@ public void should_allow_use_of_dsl_graph_binary() throws Exception { | |
@Test | ||
public void should_return_correct_results_when_bulked() { | ||
Assumptions.assumeThat( | ||
ccmRule().getCcmBridge().getDseVersion().get().compareTo(Version.parse("5.1.2")) > 0) | ||
ccmRule().getCcmBridge().getDistributionVersion().compareTo(Version.parse("5.1.2")) > 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a change in logic. Before we were getting the DSE version specifically and comparing it to the min version we want to support for this test. Here we're getting a "distribution version" (which could be anything) and checking to see if it exceeds the min version. We should somehow be validating that we're dealing with a DSE version when we get it. Note that GraphTraversalRemoteITBase above does exactly the same check... but it makes sure we're dealing with a DSE distribution before we continue on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for catching this. I do not know how I overlooked it. Fixed. |
||
.isTrue(); | ||
|
||
GraphResultSet rs = | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like we're just moving the problem around a bit. We haven't decreased the verbosity... we've just moved it around a bit.
There are a lot of instances of things that use this pattern:
The removal of the Optional types here (i.e. the removal of CcmBridge.getDseVersion() below) means that we have to remember to do both of the checks above manually. If we were to make better use of the Optional type, however, we could do the whole thing in a single check. For example, the statements above can also be expressed as:
In this case if getDseVersion() returns an empty Optional (because we aren't dealing with DSE) the orElse() will return false. Otherwise we do the integer compare and return true or false based on it's results. This approach allows us to encapsulate all the checks we want into a single line of logic. I'd argue that's more desirable in a test framework like this where you want to make sure your base assumptions are respected before moving on to the actual tests.