Skip to content
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

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

psalagnac
Copy link
Contributor

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

…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.
Copy link
Contributor

@epugh epugh left a 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());
Copy link
Contributor

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:

assertThat(cspZk.getClusterProperties(), Matchers.hasEntry("ext.foo", "bar"));

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much nicer!

@psalagnac psalagnac merged commit e105547 into apache:main Dec 10, 2024
3 checks passed
@psalagnac psalagnac deleted the solr-16390-test branch December 10, 2024 21:39
psalagnac added a commit that referenced this pull request Dec 10, 2024
…AllGood (#2901)

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.

(cherry picked from commit e105547)
@gerlowskija
Copy link
Contributor

Sorry all for the bug - thanks for fixing this @psalagnac (and for reviewing, David and Eric!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants