Skip to content

New SemanticFieldInferenceFieldsIT test#154009

Open
kkharbas wants to merge 3 commits into
elastic:mainfrom
kkharbas:SemanticTextInferenceFieldsIT
Open

New SemanticFieldInferenceFieldsIT test#154009
kkharbas wants to merge 3 commits into
elastic:mainfrom
kkharbas:SemanticTextInferenceFieldsIT

Conversation

@kkharbas

Copy link
Copy Markdown
Contributor
  1. New SemanticFieldInferenceFieldsIT test for inference field inclusion//exclusion with semantic field.
  2. Extracts common logic from SemanticTextnferenceFieldsIT into abstract base class AbstractInferenceFieldsIT

Partly addresses #146592

@elasticsearchmachine elasticsearchmachine added v9.6.0 needs:triage Requires assignment of a team area label labels Jul 15, 2026
@kkharbas kkharbas added >test Issues or PRs that are addressing/adding tests Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch :Search Relevance/Search Catch all for Search Relevance and removed needs:triage Requires assignment of a team area label labels Jul 15, 2026
@elasticsearchmachine

Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-search-relevance (Team:Search Relevance)

@kkharbas
kkharbas requested a review from Mikep86 July 15, 2026 19:03

@Mikep86 Mikep86 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this is how we should generalize this test case. It adds a lot of duplication that is confusing and hard to maintain. Instead, I think we can run cases for both field types through excludeInferenceFieldsFromSourceTestCase. We just need to parameterize that across a few dimensions:

  • The inference endpoint(s) created
  • The field(s) created (1:1 with the inference endpoints)
  • The field values used during indexing
    • We can use SemanticTextFieldTests.randomSemanticInput to generate a random mix of values appropriate for the field type
  • The field(s) queried and asserted on

Most of the logic can live in AbstractInferenceFieldsIT, and the concrete implementations can be thin layers that define the abstract functions that implement these parameters.

@kkharbas

Copy link
Copy Markdown
Contributor Author

I don't think this is how we should generalize this test case. It adds a lot of duplication that is confusing and hard to maintain. Instead, I think we can run cases for both field types through excludeInferenceFieldsFromSourceTestCase. We just need to parameterize that across a few dimensions:

  • The inference endpoint(s) created

  • The field(s) created (1:1 with the inference endpoints)

  • The field values used during indexing

    • We can use SemanticTextFieldTests.randomSemanticInput to generate a random mix of values appropriate for the field type
  • The field(s) queried and asserted on

Most of the logic can live in AbstractInferenceFieldsIT, and the concrete implementations can be thin layers that define the abstract functions that implement these parameters.

Updated. Its in a better shape now.

@kkharbas
kkharbas requested a review from Mikep86 July 23, 2026 16:27

@Mikep86 Mikep86 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This isn't quite what I meant. We can generalize the logic further and provide more test coverage with less code duplication. I took a shot at it myself: #155004

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Search Relevance/Search Catch all for Search Relevance Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch >test Issues or PRs that are addressing/adding tests v9.6.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants