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

Refactor unit tests for vector functions. #48662

Merged
merged 3 commits into from
Oct 30, 2019

Conversation

jtibshirani
Copy link
Contributor

This PR performs the following changes:

  • Split ScoreScriptUtilsTests into DenseVectorFunctionTests and
    SparseVectorFunctionTests. This will make it easier to delete all sparse
    vector function tests once we remove support on 8.x.
  • As much as possible, break up the large test methods into individual tests
    for each vector function (cosineSimilarity, l2norm, etc.).

@jtibshirani jtibshirani added :Search/Search Search-related issues that do not fall into other categories >refactoring v8.0.0 v7.6.0 labels Oct 29, 2019
@elasticmachine
Copy link
Collaborator

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

}

public void testDenseVectorFunctions() {
for (Version indexVersion : Arrays.asList(Version.V_7_4_0, Version.CURRENT)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a loop here as we expect a change in every version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't planning to add more versions (unless we have more version-dependent changes). The loop is really just to test these two listed versions.

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

@jtibshirani Thank you, very nice refactoring. I like how the tests are so clearly organized, and very fast to understand. I will adopt your style :)

@jtibshirani
Copy link
Contributor Author

Thanks @mayya-sharipova for the review (and @idjevm for taking a look as well!)

@jtibshirani jtibshirani merged commit 83ef155 into elastic:master Oct 30, 2019
@jtibshirani jtibshirani deleted the vector-function-tests branch October 30, 2019 22:20
jtibshirani added a commit that referenced this pull request Oct 30, 2019
This PR performs the following changes:
* Split `ScoreScriptUtilsTests` into `DenseVectorFunctionTests` and
`SparseVectorFunctionTests`. This will make it easier to delete all sparse
vector function tests once we remove support on 8.x.
* As much as possible, break up the large test methods into individual tests
for each vector function (`cosineSimilarity`, `l2norm`, etc.).
@jtibshirani jtibshirani added :Search Relevance/Vectors Vector search and removed :Search/Search Search-related issues that do not fall into other categories labels Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants