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-13696 Simplify routed alias tests to avoid flakiness, improve debugging #2411

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gus-asf
Copy link
Contributor

@gus-asf gus-asf commented Feb 22, 2021

No description provided.

@gus-asf gus-asf requested a review from tflobbe February 22, 2021 00:34
@gus-asf
Copy link
Contributor Author

gus-asf commented Feb 22, 2021

@tflobbe Please review what I did to make enable the uploading of config sets within tests possible. When I tried to re-enable this test it was failing it was untrusted and no auth (changes for https://issues.apache.org/jira/browse/SOLR-14663 ) ... certainly open to other suggestions, but it didn't seem reasonable to be trying to set up authentication just for a test.

@gus-asf gus-asf requested a review from hossman February 22, 2021 00:47
@tflobbe
Copy link
Member

tflobbe commented Feb 22, 2021

Thanks for the ping @gus-asf. I’m on vacations without my computer. I can get back to this PR later this week If it’s still around.

@gus-asf
Copy link
Contributor Author

gus-asf commented Feb 22, 2021

@tflobbe later this week is fine :)

@tflobbe
Copy link
Member

tflobbe commented Feb 26, 2021

I don't think this will work well. Gradle runs multiple tests on the same JVM, static state will span multiple tests (so, depending on the order of tests, this could conflict with TestConfigSetsAPI or other classes testing auth or causing other inconsistencies).

How about replacing this code in RoutedAliasUpdateProcessorTest:

assertEquals(0, new ConfigSetAdminRequest.Create()
        .setConfigSetName(configName)
        .setBaseConfigSetName("_default")
        .process(getSolrClient()).getStatus());

With something like:

new ZkConfigManager(cluster.getZkClient()).copyConfigDir("_default", configName);

Would that work for you? You wouldn't be using the Configsets API, but I think that's OK, it's not what this class is trying to test?

@madrob
Copy link
Contributor

madrob commented Apr 15, 2021

it didn't seem reasonable to be trying to set up authentication just for a test.

I think this would be the correct approach. Are there other tests you can borrow authentication configs from?

@gus-asf
Copy link
Contributor Author

gus-asf commented Apr 15, 2021

Yeah sorry haven't got back to this. Work is quite busy. my recollection is that my initial thought was that the concern raised is not actually a problem since no tests run concurrently on the same vm, but.... I had not been going to say that until I spent some time verifying that...

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

Successfully merging this pull request may close these issues.

3 participants