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

Replace script unit tests with integration tests #60027

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Jul 21, 2020

This replaces the fancy unit tests that we were doing to test our
painless integration with an extension to our existing integration
tests. The integration tests are "more real", shorter, and are a nice
step down the road of dropping our dependency on the painless plugin in
favor of a dependency only on it's SPI.

This replaces the fancy unit tests that we were doing to test our
painless integration with an extension to our existing integration
tests. The integration tests are "more real", shorter, and are a nice
step down the road of dropping our dependency on the painless plugin in
favor of a dependency only on it's SPI.
@nik9000 nik9000 added the :Search/Search Search-related issues that do not fall into other categories label Jul 21, 2020
@nik9000 nik9000 requested a review from javanna July 21, 2020 22:30
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jul 21, 2020
@javanna javanna mentioned this pull request Jul 21, 2020
30 tasks
@javanna
Copy link
Member

javanna commented Jul 22, 2020

I see where this comes from, but this raises a question for me: "are we overusing yaml tests?" They are nice to read and nice to have but ideally we should have more java tests and possibly unit tests. I understand why testing scripting is hard from a unit test and yaml tests are real and give you confidence that stuff works, though I don't like that we rely on yaml tests so much.

@nik9000
Copy link
Member Author

nik9000 commented Jul 22, 2020

Personally I'm ok with more yml tests, but I can see where you are coming from. If I were to cut back on the yml tests I'd remove most of the query tests and rely on the tests we have in the mapped field type. I think integration tests are pretty compelling for testing these cases so we can check the out how we integrate with painless without the unit tests depending on painless. Right now I think we are the only project who's unit tests depend on painless. I figure we should follow along. The only reason I added the unit test dependency on painless in the first place is because we couldn't make a mapper.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM

@javanna
Copy link
Member

javanna commented Jul 22, 2020

I think it may make sense to have less yaml tests for queries, as they duplicate unit tests for them.

@nik9000 nik9000 merged commit 9d72854 into elastic:feature/runtime_fields Jul 22, 2020
@nik9000
Copy link
Member Author

nik9000 commented Jul 22, 2020

Thanks @javanna ! I'll open a PR to drop a few yml tests for queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants