Skip to content

Conversation

@hariso
Copy link
Contributor

@hariso hariso commented Feb 7, 2018

This should make writing unit tests an easier task. It's basically the same as #27238.

Comment here is the motivation.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@hariso
Copy link
Contributor Author

hariso commented Feb 7, 2018

@elasticmachine Why are you spamming? :D

@javanna
Copy link
Member

javanna commented Feb 8, 2018

hi @hariso I am not a big fan of this change as the reason is only testing. #27238 had different reasons, like the need to add support for custom endpoints. I am not sure this is needed in IndicesClient but I see the testing problems that are caused by not being able to mock the class so I am considering getting it in. I see that despite the class is non-final though, still there is no way to use a custom subclass in RestHighLevelClient due to the way IndicesClient is created. Is this good enough to make your tests happy?

@hariso
Copy link
Contributor Author

hariso commented Feb 8, 2018

@javanna Thanks for feedback and new info which I just learnt! Actually, this is not enough to make the tests happy. The configuration I used for my tests was a bit hacky, so a non-final class with final method still doesn't work. I'll close this PR since it doesn't really solve the problem, and try finding another solution.

Thanks again!

@hariso hariso closed this Feb 8, 2018
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