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

Test/chaos_experiment_runs: Handler and service tests for experiment runs #4136

Merged

Conversation

SohamRatnaparkhi
Copy link
Contributor

@SohamRatnaparkhi SohamRatnaparkhi commented Aug 17, 2023

Proposed changes

Closes #4141
This PR consists tests for handlers and services in choas_experiment_runs package

Types of changes

What types of changes does your code introduce to Litmus? Put an x in the boxes that apply

  • New feature (non-breaking change which adds functionality)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices applies)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc
  • I have signed the commit for DCO to be passed.
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added necessary documentation (if appropriate)

Dependency

Special notes for your reviewer:

This PR is related to LFX Mentorship-Term2

Signed-off-by: SohamRatnaparkhi <soham.ratnaparkhi@gmail.com>
Signed-off-by: SohamRatnaparkhi <soham.ratnaparkhi@gmail.com>
Signed-off-by: SohamRatnaparkhi <soham.ratnaparkhi@gmail.com>
Signed-off-by: SohamRatnaparkhi <soham.ratnaparkhi@gmail.com>
Signed-off-by: SohamRatnaparkhi <soham.ratnaparkhi@gmail.com>
Signed-off-by: SohamRatnaparkhi <soham.ratnaparkhi@gmail.com>
Signed-off-by: SohamRatnaparkhi <soham.ratnaparkhi@gmail.com>
Signed-off-by: SohamRatnaparkhi <soham.ratnaparkhi@gmail.com>
@SohamRatnaparkhi SohamRatnaparkhi changed the title Test/chaos_experiment_runs Test/chaos_experiment_runs: Handler and service tests for experiment runs Aug 17, 2023
@namkyu1999 namkyu1999 added language/go kind/unit-test LFX-MENTORSHIP Linux Foundation Mentor ship Issue labels Aug 18, 2023
@amityt amityt requested a review from namkyu1999 August 24, 2023 13:53
}
cursor, _ := mongo.NewCursorFromDocuments(findResult, nil, nil)

infrastructureService.On("GetInfra", mock.Anything, mock.Anything, mock.Anything).Return(infra, nil).Once()
Copy link
Member

Choose a reason for hiding this comment

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

Hey @SohamRatnaparkhi proposed changes looks good to me. Just a small request, Can you check this test case again, please? It failed. I think you need to mock operator.Get(ctx, mongodb.ChaosInfraCollection, query). Because the handler creates a new InfrastructureService in the RunChaosWorkFlow function but reuses the operator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @namkyu1999
I think we should remove that test case because in any case it is going to fail in test environment. I feel so because, on in the function at this line (link), it starts a mongo session which i guess is not possible in test environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I remove the testcase and make a new commit?

Copy link
Member

Choose a reason for hiding this comment

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

@SohamRatnaparkhi
We can use mtest package to mock session operations. But for using this package, we have to move this global variable to a local one.

So, we have two choices,

  1. Moving global variable to local, using mtest pkg, and updating unit test case.
  2. Remove this test case and complete task 1 in another pr.

Either way, I'm all for it.

reference

  1. mtest
  2. mtest example
  3. mocking session operations example

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 think we should remove this testcase now and then create a new PR.
Actually, there are few ither instances where I have written a testcase because it was having a similar issue.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @SohamRatnaparkhi, Are you working on this issue? If yes, how about creating a GitHub issue for tracking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @namkyu1999 ,
I had forgotten about this completely. Yep we can definitely create an issue for this.

@namkyu1999 namkyu1999 self-requested a review September 11, 2023 05:55
Signed-off-by: SohamRatnaparkhi <soham.ratnaparkhi@gmail.com>
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.

Testcases for choas_experiment_run package
3 participants