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

Adding UTs for IndexReplicationTask #109

Merged
merged 1 commit into from
Aug 24, 2021
Merged

Conversation

gbbafna
Copy link
Collaborator

@gbbafna gbbafna commented Aug 18, 2021

Description

Adding UTs for IndexReplicationTask

Issues Resolved

#108

Check List

  • Commits are signed 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.

@gbbafna
Copy link
Collaborator Author

gbbafna commented Aug 18, 2021

Things added in main code path :

  1. Some helper functions for UTs.
  2. Made many classes open as @OpenForTesting is not working.
  3. Made a function public to call from UTs.

krishna-ggk
krishna-ggk previously approved these changes Aug 18, 2021
Copy link
Collaborator

@krishna-ggk krishna-ggk left a comment

Choose a reason for hiding this comment

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

Thanks for bootstrapping UTs. Looks good for start.

Comment on lines +152 to +136
// Delay to let task execute
delay(1000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit - yield is a better way to yield back instead of delay.


}

open class NoOpClient(testName :String) : NoOpNodeClient(testName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this could be extracted out and can also be helpful for other tests.

Copy link
Collaborator Author

@gbbafna gbbafna Aug 20, 2021

Choose a reason for hiding this comment

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

I have extracted out from IndexReplicationTaskTests , but kept it in the same package as it needs all variables like follower index et all and is IndexReplicationTask specific as of now.

var taskManager = Mockito.mock(TaskManager::class.java)
replicationTask.setPersistent(taskManager)
var rc = ReplicationContext(followerIndex)
var rm = ReplicationMetadata(connectionName, ReplicationStoreMetadataType.INDEX.name, ReplicationOverallState.RUNNING.name, "reason", rc, rc, Settings.EMPTY)
Copy link
Member

Choose a reason for hiding this comment

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

Minor: We can probably move the logic to create ReplicationMetadata, ClusterState, etc objects to a helper utility

@gbbafna gbbafna merged commit 635decf into opensearch-project:main Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants