Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase #11877

reta · 2024-01-12T19:51:56Z

Description

Extract cluster management for integration tests into JUnit test rules our of OpenSearchIntegTestCase. @neetikasinghal this is follow up on the work you have done for parameterizing integration test cases. We now have removed the test cluster management logic from the OpenSearchIntegTestCase to dedicated JUnit rule, which gave us an opportunity implement two favours of cluster setting parametrization:

ParameterizedDynamicSettingsOpenSearchIntegTestCase (was implemented by ParameterizedOpenSearchIntegTestCase ): the test clusters settings are applied dynamically before each test method
ParameterizedStaticSettingsOpenSearchIntegTestCase (was not supported): the clusters settings are applied at startup time

CC @andrross should open up the road for parameterized segment replication test cases

Related Issues

N/A

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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.

github-actions · 2024-01-12T19:59:03Z

Compatibility status:

Checks if related components are compatible with change 359e5c6

Incompatible components

Incompatible components: [https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/cross-cluster-replication.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/alerting.git]

github-actions · 2024-01-12T20:47:20Z

❌ Gradle check result for 6912b3c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-01-12T20:59:49Z

❌ Gradle check result for 97f4803: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-01-15T18:53:15Z

❌ Gradle check result for 1739955: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-01-15T19:50:34Z

❌ Gradle check result for 1739955: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

reta · 2024-01-15T20:00:41Z

❌ Gradle check result for 1739955: FAILURE

#10006

github-actions · 2024-01-15T20:48:09Z

❌ Gradle check result for 1739955: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

reta · 2024-01-15T20:52:45Z

❌ Gradle check result for 1739955: FAILURE

#9191
#10749
#10755

github-actions · 2024-01-15T21:39:18Z

❕ Gradle check result for 1739955: UNSTABLE

TEST FAILURES:

      2 org.opensearch.index.shard.RemoteStoreRefreshListenerTests.classMethod
      1 org.opensearch.remotestore.RemoteIndexPrimaryRelocationIT.testPrimaryRelocationWhileIndexing
      1 org.opensearch.index.shard.RemoteStoreRefreshListenerTests.testRefreshAfterCommit

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov · 2024-01-15T21:40:37Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (5dd4b61) 71.38% compared to head (359e5c6) 71.36%.
Report is 9 commits behind head on main.

Files	Patch %	Lines
...h/aggregations/bucket/FastFilterRewriteHelper.java	87.40%	4 Missing and 13 partials ⚠️
...egations/bucket/composite/CompositeAggregator.java	88.37%	3 Missing and 2 partials ⚠️
...gregations/bucket/composite/InternalComposite.java	0.00%	2 Missing ⚠️
...ions/bucket/histogram/DateHistogramAggregator.java	84.61%	0 Missing and 2 partials ⚠️
.../bucket/histogram/AutoDateHistogramAggregator.java	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #11877      +/-   ##
============================================
- Coverage     71.38%   71.36%   -0.03%     
- Complexity    59354    59425      +71     
============================================
  Files          4923     4923              
  Lines        279095   279178      +83     
  Branches      40559    40581      +22     
============================================
- Hits         199228   199222       -6     
- Misses        63292    63442     +150     
+ Partials      16575    16514      -61

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

github-actions · 2024-01-16T19:05:17Z

❌ Gradle check result for 37c6bcf: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-01-16T20:33:17Z

❌ Gradle check result for 22d3199: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-01-16T21:20:48Z

❌ Gradle check result for e428d4c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

reta · 2024-01-16T22:59:45Z

❌ Gradle check result for e428d4c: FAILURE

#9191
#10983
#10154

github-actions · 2024-01-16T23:13:25Z

❌ Gradle check result for e428d4c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

reta · 2024-01-16T23:21:54Z

❌ Gradle check result for e428d4c: FAILURE

#9191

github-actions · 2024-01-16T23:52:16Z

❌ Gradle check result for e428d4c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

reta · 2024-01-17T00:00:24Z

❌ Gradle check result for e428d4c: FAILURE

#1703

github-actions · 2024-01-17T00:49:25Z

❕ Gradle check result for e428d4c: UNSTABLE

TEST FAILURES:

      1 org.opensearch.remotestore.RemoteStoreStatsIT.testStatsResponseAllShards
      1 org.opensearch.action.admin.cluster.node.tasks.ResourceAwareTasksTests.testTaskResourceTrackingDuringTaskCancellation

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

neetikasinghal · 2024-01-17T20:27:54Z

thanks for explaining, then for what use-cases should ParameterizedDynamicSettingsOpenSearchIntegTestCase be used as it can be confusing from its name.

There are javadocs for each class (and ParameterizedOpenSearchIntegTestCase), is it helpful or there are gaps that are unclear? Would appreciate your feedback, thank you.

done

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

github-actions · 2024-01-17T21:09:40Z

❕ Gradle check result for 2bef064: UNSTABLE

TEST FAILURES:

      1 org.opensearch.repositories.azure.AzureBlobStoreRepositoryTests.testReadRange
      1 org.opensearch.remotestore.RemoteIndexPrimaryRelocationIT.testPrimaryRelocationWhileIndexing

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

test/framework/src/main/java/org/opensearch/test/OpenSearchTestClusterRule.java

andrross

Thanks @reta, in general I think this is great and a better way to structure the test logic within the JUnit framework.

github-actions · 2024-01-17T22:14:54Z

❕ Gradle check result for 359e5c6: UNSTABLE

TEST FAILURES:

      1 org.opensearch.remotestore.RemoteIndexPrimaryRelocationIT.testPrimaryRelocationWhileIndexing

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

reta · 2024-01-18T12:32:26Z

@peternied how does it look to you?

peternied · 2024-01-18T20:49:51Z

@peternied how does it look to you?

Thanks for checking in @reta I don't have time for a second pass today, but I'm not concerned about merging with @andrross 's approval. I'll create issues or a follow up PR if there is anything I'd like to see addressed.

opensearch-trigger-bot · 2024-01-18T20:54:38Z

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-11877-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c267c5affa52bb3d176b81f87c9175090f7469e5
# Push it to GitHub
git push --set-upstream origin backport/backport-11877-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

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

… out of OpenSearchIntegTestCase (opensearch-project#11877) * Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Branched off parametrized tests into static / dynamic settings flavors Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Refactor internalCluster() method to return an Optional value Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> --------- Signed-off-by: Andriy Redko <andriy.redko@aiven.io> (cherry picked from commit c267c5a)

… out of OpenSearchIntegTestCase (#11877) (#11941) * Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Branched off parametrized tests into static / dynamic settings flavors Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Refactor internalCluster() method to return an Optional value Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> --------- Signed-off-by: Andriy Redko <andriy.redko@aiven.io> (cherry picked from commit c267c5a)

Rishikesh1159 · 2024-01-23T19:32:24Z

@reta seems like we are not running all parameter cases of a parameter factory when cluster scope is default or SCOPE.SUITE. Instead we are only running the first case of parameter factory multiple times (second case is missing).

Example Use Case

In above example, when we run test class, we are only running the first case of CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), false) twice. This is because we are setting the parameter values in @override nodeSettings(). But this method is called only once when the cluster is being build here. And in case of SCOPE.SUITE we only build cluster once here.

So when same test method tries to run with second/different parameters, the node settings are never overridden again with new parameter values because the cluster is already present. Node settings are overridden only when new cluster is built. So this way test is running again with same settings passed in first parameter values.

Current flow of test runs with multiple parameters and scope set to SCOPE.SUITE or default:

Build Cluster -> run testMethod1() with param1 -> run testMethod2() with param1 -> run testMethod1() with param1 -> run testMethod2() with param1 -> Remove cluster

Instead correct flow for using static settings should be :

Build Cluster -> run testMethod1() with param1 -> run testMethod2() with param1 -> Remove Cluster -> Build cluster again -> run testMethod1() with param2 -> run testMethod2() with param2-> Remove cluster

reta · 2024-01-23T19:48:36Z

@Rishikesh1159 thanks for bringing it up

So when same test method tries to run with second/different parameters, the node settings are never overridden again with new parameter values because the cluster is already present. Node settings are overridden only when new cluster is built. So this way test is running again with same settings passed in first parameter values.

The cluster should be shut down once the change of parameters is detected, so the new node settings should be picked up, I will double check shortly

andrross · 2024-01-23T20:51:26Z

@Rishikesh1159 initializeSuiteScope() should be invoked before each method invocation, and when it detects that parameters have changed it should rebuild the cluster. Can you look into whether that is happening in the case you're testing?

Rishikesh1159 · 2024-01-23T21:03:57Z

@andrross Yes initializeSuiteScope() is being invoked on every test method invocation. But the problem is the conditional check here if (isSuiteScopedTest(targetClass)).

We have three type of scopes:

@ClusterScope(scope = Scope.SUITE) - example
@ClusterScope(scope = Scope.TEST) - example
@OpenSearchIntegTestCase.SuiteScopeTestCase - example

The conditional check was only detecting 3rd type of scope. But majority of tests are 1st type including this example (Use Case)

reta · 2024-01-23T21:32:13Z

@Rishikesh1159 initializeSuiteScope() should be invoked before each method invocation, and when it detects that parameters have changed it should rebuild the cluster. Can you look into whether that is happening in the case you're testing?

@andrross the @Rishikesh1159's explanation (#11877 (comment)) is correct, I was thinking exactly the way you described but it turned out initializeSuiteScope() is not the same as @ClusterScope(scope = Scope.SUITE), it should be fixed by #12000, thanks folks

neetikasinghal · 2024-01-23T22:28:11Z

@reta seems like we are not running all parameter cases of a parameter factory when cluster scope is default or SCOPE.SUITE. Instead we are only running the first case of parameter factory multiple times (second case is missing).

Example Use Case

In above example, when we run test class, we are only running the first case of CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), false) twice. This is because we are setting the parameter values in @override nodeSettings(). But this method is called only once when the cluster is being build here. And in case of SCOPE.SUITE we only build cluster once here.

So when same test method tries to run with second/different parameters, the node settings are never overridden again with new parameter values because the cluster is already present. Node settings are overridden only when new cluster is built. So this way test is running again with same settings passed in first parameter values.

Current flow of test runs with multiple parameters and scope set to SCOPE.SUITE or default:

Build Cluster -> run testMethod1() with param1 -> run testMethod2() with param1 -> run testMethod1() with param1 -> run testMethod2() with param1 -> Remove cluster

Instead correct flow for using static settings should be :

Build Cluster -> run testMethod1() with param1 -> run testMethod2() with param1 -> Remove Cluster -> Build cluster again -> run testMethod1() with param2 -> run testMethod2() with param2-> Remove cluster

thanks @Rishikesh1159, when I was debugging one of the tests today I realized that even with concurrent search parameter set to true, it wasnt taking the concurrent search path. This explains on why it was the case.

reta · 2024-01-23T22:30:33Z

thanks @Rishikesh1159, when I was debugging one of the tests today I realized that even with concurrent search parameter set to true, it wasnt taking the concurrent search path. This explains on why it was the case.

Sorry about that @neetikasinghal , the issue slipped in

… out of OpenSearchIntegTestCase (opensearch-project#11877) * Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Branched off parametrized tests into static / dynamic settings flavors Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Refactor internalCluster() method to return an Optional value Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> --------- Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

… out of OpenSearchIntegTestCase (opensearch-project#11877) * Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Branched off parametrized tests into static / dynamic settings flavors Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Refactor internalCluster() method to return an Optional value Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> --------- Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: Shivansh Arora <hishiv@amazon.com>

reta added the skip-changelog label Jan 12, 2024

reta changed the title ~~Extract cluster management for integration tests into JUnit test rules our of OpenSearchIntegTestCase~~ [WIP] Extract cluster management for integration tests into JUnit test rules our of OpenSearchIntegTestCase Jan 12, 2024

reta mentioned this pull request Jan 12, 2024

[Meta] [Segment Replication] Run all integration tests with segment replication enabled #6761

Closed

12 tasks

reta force-pushed the junit.rule branch from 6912b3c to 97f4803 Compare January 12, 2024 20:13

reta force-pushed the junit.rule branch from 97f4803 to 1739955 Compare January 15, 2024 18:24

reta changed the title ~~[WIP] Extract cluster management for integration tests into JUnit test rules our of OpenSearchIntegTestCase~~ [WIP] Extract cluster management for integration tests into JUnit test rules out of OpenSearchIntegTestCase Jan 15, 2024

reta changed the title ~~[WIP] Extract cluster management for integration tests into JUnit test rules out of OpenSearchIntegTestCase~~ [WIP] Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase Jan 15, 2024

reta force-pushed the junit.rule branch from 1739955 to 37c6bcf Compare January 16, 2024 18:56

reta force-pushed the junit.rule branch from 37c6bcf to 22d3199 Compare January 16, 2024 19:50

reta force-pushed the junit.rule branch from 22d3199 to e428d4c Compare January 16, 2024 20:34

reta mentioned this pull request Jan 16, 2024

[Segment Replication] Add Segment Replication Specific Integration Tests #11773

Merged

8 tasks

Address code review comments

359e5c6

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

andrross reviewed Jan 17, 2024

View reviewed changes

test/framework/src/main/java/org/opensearch/test/OpenSearchTestClusterRule.java Show resolved Hide resolved

andrross reviewed Jan 17, 2024

View reviewed changes

andrross approved these changes Jan 18, 2024

View reviewed changes

neetikasinghal approved these changes Jan 18, 2024

View reviewed changes

andrross added the backport 2.x Backport to 2.x branch label Jan 18, 2024

andrross merged commit c267c5a into opensearch-project:main Jan 18, 2024
35 of 40 checks passed

opensearch-trigger-bot bot added the backport-failed label Jan 18, 2024

reta mentioned this pull request Jan 18, 2024

[Backport] [2.x] Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase (#11877) #11941

Merged

reta mentioned this pull request Jan 23, 2024

Fix test cluster behavior @ClusterScope(scope = Scope.SUITE) and @OpenSearchIntegTestCase.SuiteScopeTestCase for parameterized test cases #12000

Merged

8 tasks

Rishikesh1159 mentioned this pull request Feb 8, 2024

[Segment Replication] Support random selection of replication strategy for parameterized Integ tests to run with Segment Replication #12261

Closed

BrewTestBot mentioned this pull request Feb 21, 2024

opensearch 2.12.0 Homebrew/homebrew-core#163463

Merged

Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase #11877

Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase #11877

Conversation

reta commented Jan 12, 2024 • edited Loading

Description

Related Issues

Check List

github-actions bot commented Jan 12, 2024 • edited Loading

Compatibility status:

Incompatible components

Skipped components

Compatible components

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 15, 2024

github-actions bot commented Jan 15, 2024

reta commented Jan 15, 2024

github-actions bot commented Jan 15, 2024

reta commented Jan 15, 2024

github-actions bot commented Jan 15, 2024

codecov bot commented Jan 15, 2024 • edited Loading

Codecov Report

github-actions bot commented Jan 16, 2024

github-actions bot commented Jan 16, 2024

github-actions bot commented Jan 16, 2024

reta commented Jan 16, 2024

github-actions bot commented Jan 16, 2024

reta commented Jan 16, 2024

github-actions bot commented Jan 16, 2024

reta commented Jan 17, 2024

github-actions bot commented Jan 17, 2024

neetikasinghal commented Jan 17, 2024

github-actions bot commented Jan 17, 2024

andrross left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2024

reta commented Jan 18, 2024

peternied commented Jan 18, 2024

opensearch-trigger-bot bot commented Jan 18, 2024

Rishikesh1159 commented Jan 23, 2024 • edited Loading

Example Use Case

Current flow of test runs with multiple parameters and scope set to SCOPE.SUITE or default:

Instead correct flow for using static settings should be :

reta commented Jan 23, 2024

andrross commented Jan 23, 2024

Rishikesh1159 commented Jan 23, 2024 • edited Loading

reta commented Jan 23, 2024 • edited Loading

neetikasinghal commented Jan 23, 2024 • edited Loading

Example Use Case

Current flow of test runs with multiple parameters and scope set to SCOPE.SUITE or default:

Instead correct flow for using static settings should be :

reta commented Jan 23, 2024

reta commented Jan 12, 2024 •

edited

Loading

github-actions bot commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 15, 2024 •

edited

Loading

Rishikesh1159 commented Jan 23, 2024 •

edited

Loading

Rishikesh1159 commented Jan 23, 2024 •

edited

Loading

reta commented Jan 23, 2024 •

edited

Loading

neetikasinghal commented Jan 23, 2024 •

edited

Loading