Refactor integ tests that access model index #1423

jmazanec15 · 2024-01-26T17:50:56Z

Description

Refactors integration tests that directly access the model system index. End users should not be directly accessing the model system index. It is supposed to be an implementation detail. We have written restful integration tests that directly access the model system index in order to initialize the cluster state. However, we should not do this because users should not be able to interact with it through restful APIs. In fact, in some cluster configurations, it should not be possible.

That being said, some of this implementation detail leaks out through the interface. For instance, in k-NN stats we have a stat that is the model system index status. So, in order to test this, we do need direct access to the system index. Similarly, for search, we execute the search against the system index and directly return the results. This is probably a bug - but we still need to test it. So, for these cases, we check if the index exists.

Additionally, I deleted the test RestDeleteModelHandlerIT.testDeleteTrainingModel. This test had an inherit race condition that model that was got trained did not complete until a few lines later. This will lead to flakiness, so I deleted it. This functionality is also already unit tested in ModelDaoTests.

Along with this, I did some test renaming to make test logic more clear using test*_when*_then* convention.

Issues Resolved

#1365 , #888

Check List

Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Refactors integration tests that directly access the model system index. End users should not be directly accessing the model system index. It is supposed to be an implementation detail. We have written restful integration tests that directly access the model system index in order to initialize the cluster state. However, we should not do this because users should not be able to interact with it through restful APIs That being said, some of this implementation detail leaks out into the interface. For instance, in k-NN stats we have a stat that is the model system index status. So, in order to test this, we do need direct access to the system index. Similarly, for search, we execute the search against the system index and directly return the results. This is probably a bug - but we still need to test it. Signed-off-by: John Mazanec <[email protected]>

codecov · 2024-01-26T18:08:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (47728ce) 84.92% compared to head (8902317) 84.80%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1423      +/-   ##
============================================
- Coverage     84.92%   84.80%   -0.12%     
+ Complexity     1274     1273       -1     
============================================
  Files           167      167              
  Lines          5186     5186              
  Branches        491      491              
============================================
- Hits           4404     4398       -6     
- Misses          573      579       +6     
  Partials        209      209

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jmazanec15 · 2024-01-26T18:31:38Z

Failure related to #1413

ryanbogan · 2024-01-26T19:28:34Z

In a lot of the tests, the only change is removing the createModelSystemIndex. For my understanding, was this method not necessary at all for the successful completion of these tests?

src/test/java/org/opensearch/knn/plugin/action/RestDeleteModelHandlerIT.java

src/test/java/org/opensearch/knn/plugin/action/RestGetModelHandlerIT.java

src/test/java/org/opensearch/knn/plugin/action/RestSearchModelHandlerIT.java

heemin32 · 2024-01-26T19:30:27Z

src/test/java/org/opensearch/knn/plugin/action/RestSearchModelHandlerIT.java

-    public void testNoModelExists() throws Exception {
-        createModelSystemIndex();
+    public void testSearch_whenNoModelExists_thenReturnEmptyResults() throws Exception {
+        // Currently, if the model index exists, we will return empty hits. If it does not exist, we will


Can we create an issue to fix this?

Sure, #1425

…HandlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

…dlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

…HandlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

…dlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

…HandlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

jmazanec15 · 2024-01-26T19:54:11Z

In a lot of the tests, the only change is removing the createModelSystemIndex. For my understanding, was this method not necessary at all for the successful completion of these tests?

Currently, for training, if the model does not exist, then it is supposed to create the system index. So, for many tests that were already doing the training, it didnt do anything

ryanbogan

LGTM!

Signed-off-by: John Mazanec <[email protected]>

opensearch-trigger-bot · 2024-01-26T21:38:59Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1423-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 2b963b4ae2ae06f0108e3c80e7b9d8785b9033b3
# Push it to GitHub
git push --set-upstream origin backport/backport-1423-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1423-to-2.x.

Refactors integration tests that directly access the model system index. End users should not be directly accessing the model system index. It is supposed to be an implementation detail. We have written restful integration tests that directly access the model system index in order to initialize the cluster state. However, we should not do this because users should not be able to interact with it through restful APIs That being said, some of this implementation detail leaks out into the interface. For instance, in k-NN stats we have a stat that is the model system index status. So, in order to test this, we do need direct access to the system index. Similarly, for search, we execute the search against the system index and directly return the results. This is probably a bug - but we still need to test it. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 2b963b4)

jmazanec15 added backport 2.x integ-test-failure Integration test failures labels Jan 26, 2024

jmazanec15 requested review from heemin32, navneet1v, VijayanB, vamshin, naveentatikonda, junqiu-lei, martin-gaievski and ryanbogan as code owners January 26, 2024 17:50

jmazanec15 force-pushed the issue-1365 branch from 01b1a54 to f3302d2 Compare January 26, 2024 17:51

jmazanec15 mentioned this pull request Jan 26, 2024

[Flaky test] org.opensearch.knn.plugin.action.RestDeleteModelHandlerIT.testTrainingDeletedModel #888

Closed

heemin32 reviewed Jan 26, 2024

View reviewed changes

heemin32 previously approved these changes Jan 26, 2024

View reviewed changes

jmazanec15 dismissed heemin32’s stale review via 682cbf4 January 26, 2024 19:52

jmazanec15 and others added 6 commits January 26, 2024 11:52

Update src/test/java/org/opensearch/knn/plugin/action/RestDeleteModel…

682cbf4

…HandlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

Update src/test/java/org/opensearch/knn/plugin/action/RestGetModelHan…

f6d78fd

…dlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

Update src/test/java/org/opensearch/knn/plugin/action/RestSearchModel…

29462be

…HandlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

Update src/test/java/org/opensearch/knn/plugin/action/RestSearchModel…

b6eff15

…HandlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

Update src/test/java/org/opensearch/knn/plugin/action/RestGetModelHan…

0981202

…dlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

Update src/test/java/org/opensearch/knn/plugin/action/RestDeleteModel…

4d9a638

…HandlerIT.java Co-authored-by: Heemin Kim <[email protected]> Signed-off-by: John Mazanec <[email protected]>

ryanbogan previously approved these changes Jan 26, 2024

View reviewed changes

jmazanec15 requested a review from heemin32 January 26, 2024 19:58

Import sneakthrows

8902317

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 dismissed ryanbogan’s stale review via 8902317 January 26, 2024 20:01

jmazanec15 requested a review from ryanbogan January 26, 2024 21:34

ryanbogan approved these changes Jan 26, 2024

View reviewed changes

heemin32 approved these changes Jan 26, 2024

View reviewed changes

jmazanec15 merged commit 2b963b4 into opensearch-project:main Jan 26, 2024
50 checks passed

jmazanec15 mentioned this pull request Jan 26, 2024

[Backport 2.x] Refactor integ tests that access model index #1428

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor integ tests that access model index #1423

Refactor integ tests that access model index #1423

jmazanec15 commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading

jmazanec15 commented Jan 26, 2024

ryanbogan commented Jan 26, 2024

heemin32 Jan 26, 2024

jmazanec15 Jan 26, 2024

jmazanec15 commented Jan 26, 2024

ryanbogan left a comment

opensearch-trigger-bot bot commented Jan 26, 2024

Refactor integ tests that access model index #1423

Refactor integ tests that access model index #1423

Conversation

jmazanec15 commented Jan 26, 2024 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Jan 26, 2024 • edited Loading

Codecov Report

jmazanec15 commented Jan 26, 2024

ryanbogan commented Jan 26, 2024

heemin32 Jan 26, 2024

Choose a reason for hiding this comment

jmazanec15 Jan 26, 2024

Choose a reason for hiding this comment

jmazanec15 commented Jan 26, 2024

ryanbogan left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jan 26, 2024

jmazanec15 commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading