-
Notifications
You must be signed in to change notification settings - Fork 59
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
Consume numNodes sent during integTestRemote #1085
Consume numNodes sent during integTestRemote #1085
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.9 #1085 +/- ##
============================================
- Coverage 76.25% 75.77% -0.49%
+ Complexity 1060 1057 -3
============================================
Files 141 141
Lines 4771 4771
Branches 546 546
============================================
- Hits 3638 3615 -23
- Misses 782 801 +19
- Partials 351 355 +4 |
build.gradle
Outdated
@@ -506,7 +506,7 @@ integTest { | |||
systemProperty "tests.cluster.${cluster.name}.http_hosts", "${-> allHttpSocketURI}" | |||
systemProperty "tests.cluster.${cluster.name}.transport_hosts", "${-> alltransportSocketURI}" | |||
systemProperty "tests.cluster.${cluster.name}.security_enabled", "${-> securityEnabled.toString()}" | |||
systemProperty "tests.cluster.${cluster.name}.total_nodes", "${-> _numNodes.toString()}" |
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.
Why do we need to change the property name for consume?
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.
I created a unified function to check if multiNodeConfiguration is present
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.
Let's keep the same key as before. isMultiNodeClusterConfiguration
can take a cluster name as argument.
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.
Done
Signed-off-by: monusingh-1 <msnghgw@amazon.com>
e3a4175
to
5b15905
Compare
Signed-off-by: monusingh-1 <msnghgw@amazon.com>
@@ -930,6 +930,9 @@ task integTestRemote (type: RestIntegTestTask) { | |||
systemProperty "tests.cluster.leaderCluster.security_enabled", System.getProperty("security_enabled") | |||
|
|||
nonInputProperties.systemProperty('tests.integTestRemote', "true") | |||
var numberOfNodes = findProperty('numNodes') as Integer | |||
systemProperty "tests.cluster.followCluster.total_nodes", "${-> numberOfNodes.toString()}" | |||
systemProperty "tests.cluster.leaderCluster.total_nodes", "${-> numberOfNodes.toString()}" | |||
systemProperty "build.dir", "${buildDir}" |
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.
For integTestRemote set total_nodes
if(totalLeaderNodes != null && totalFollowerNodes !=null && totalLeaderNodes < "2" && totalFollowerNodes < "2" ) { | ||
return false | ||
} | ||
return true |
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.
Why true
is the default behaviour here? If the system property is not getting stuck, we'll end up not skipping the tests and thus failing.
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.
If integ tests are run locally, we are always setting the property and same with integTest Remote
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.
then let's remove the null check
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.
and add an assertion instead
Signed-off-by: monusingh-1 <msnghgw@amazon.com>
Description
numNodes parameter was not getting consumed by the integTestRemote gradle task.
numNodes param is used to set
isMultiNodeClusterConfiguration
which is used to skip tests like reroute.This change allows numNodes param to be used.
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.