-
Notifications
You must be signed in to change notification settings - Fork 675
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
SOLR-16390: fix flaky test ClusterPropsAPITest.testClusterPropertyOpsAllGood #2901
Conversation
…AllGood The test fails when SSL in enabled, because of testing framework that set 'urlScheme' cluster property. This change updates the test to ignore existing cluster properties.
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.
LGTM.
Nice find, and it seems a bit of a shame that we have to have that logic there ;-).
assertEquals( | ||
testClusterProperty, | ||
(String) ((List<?>) getObjectByPath(o, true, "clusterProperties")).get(0)); | ||
assertEquals(initCount + 1, ((List<?>) getObjectByPath(o, true, "clusterProperties")).size()); |
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.
IMO this test shouldn't bother checking the size; it need only check that the entry it's expecting is there or is not there. Hamcrest test utils helps. I did this a day or two ago in this test:
solr/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java
Line 165 in 2d3a8d9
assertThat(cspZk.getClusterProperties(), Matchers.hasEntry("ext.foo", "bar")); |
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.
Good point. I'm updating the test to check the property exists instead of counting.
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.
much nicer!
Sorry all for the bug - thanks for fixing this @psalagnac (and for reviewing, David and Eric!) |
https://issues.apache.org/jira/browse/SOLR-16390
(linking to the Jira that introduced the test)
Description
The test fails when SSL is enabled, because of testing framework that set 'urlScheme' cluster property.
I was looking at the failure in another PR, and it seems this test fails quite often since #2788. I don't think it is tracked somewhere else.
Solution
This change updates the test to ignore existing cluster properties.
Tests
n/a