fix: failing backend-unit-tests #4285

SohamRatnaparkhi · 2023-11-08T10:22:37Z

Proposed changes

Updated the mock service.go files chaos-experiments and chaos-experiment-runs
Fixed failing test in chaos-hub; i.e. TestGetClonePath , TestListPredefinedWorkflowDetails and TestGetChartsData
Updated .gitignore to include the tmp folder. (The tmp folder is required to pass certain test cases.). Then after adding the tmp folder, re-added the tmp folder to .gitignore
Added dummy YAML files.

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

nil

Special notes for your reviewer:

This PR refers to the failing tests in Add actions for chaoscenter unit tests #4244 and this comment by @Jonsy13
Commenting tmp folder from the .gitignore might not be the best solution because whenever the test is run locally, the chart repo will be cloned. I had commented the tmp folder because at least one tmp folder needs to be present in choashub folder to pass the tests.
Changing tmp to .\tmp was required, as code was unable to find the \tmp path/directory

Signed-off-by: SohamRatnaparkhi <[email protected]>

chartserviceversion YAML files. Signed-off-by: SohamRatnaparkhi <[email protected]>

Signed-off-by: SohamRatnaparkhi <[email protected]>

SohamRatnaparkhi · 2023-11-08T11:23:50Z

There is a strange thing about the 2 failing test-cases of UpdateChaosExperiment and DisableCronExperiment.
When all the testcases of the package are run at once, these testcases fail, but when they are run individually, they pass. I'm adding screenshot showing that these tcs pass when run individually :)

Jonsy13 · 2023-11-08T11:49:47Z

@amityt Can you check this please!

namkyu1999 · 2023-11-11T03:41:47Z

Still failed. Can you PTAL @SohamRatnaparkhi ?

SohamRatnaparkhi · 2023-11-11T04:11:35Z

There is a strange thing about the 2 failing test-cases of UpdateChaosExperiment and DisableCronExperiment.
When all the testcases of the package are run at once, these testcases fail, but when they are run individually, they pass. I'm adding screenshot showing that these tcs pass when run individually :)

Hi @namkyu1999
The 2 tests failing are

UpdateExp
DisableCronExp

But strangely, they are passing in my local dev environment. I have attached its screenshot above.

I'm currently trying to debug the reason for same.

Signed-off-by: SohamRatnaparkhi <[email protected]>

…ix/tests

Signed-off-by: SohamRatnaparkhi <[email protected]>

SohamRatnaparkhi · 2023-11-11T14:40:23Z

Finally, all the test-cases passing!
@Jonsy13 @Saranya-jena @namkyu1999 @amityt PTAL!

namkyu1999

Could you confirm my comments?

namkyu1999 · 2023-11-11T16:49:28Z

chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go

@@ -281,7 +281,7 @@ func TestGetChartsData(t *testing.T) {
 			name:      "success: url is valid",
 			projectID: uuid.New().String(),
 			repoData: model.CloningInput{
-				Name:       uuid.New().String(),
+				Name:       "container-kill",


Why did you change this line? IMO, It's not relevant to your PR.

Yep, that's true. The issue was causing the chaos-hub tests to fail because the generated UUID was unique every time, resulting in the file being absent and triggering an error, indicating 'file not found'

namkyu1999 · 2023-11-11T16:49:52Z

chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go

+	succeedProjectID := "project-1"
+	succeedName := "workflow-hub-1"


We can kindoff refine the names and stuff, but we will require a defined file system to pass the tests.
Same reason as above.

namkyu1999 · 2023-11-11T16:52:53Z

chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go

@@ -694,7 +694,7 @@ func TestChaosExperimentHandler_DisableCronExperiment(t *testing.T) {
 			given: func() {
 				chaosExperimentService.On("ProcessExperimentUpdate", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errors.New("error while updating")).Once()
 			},
-			wantErr: true,
+			wantErr: false,


name: "Failure: mongo error while updating the experiment details",
This test case should be failed. we need to keep wantErr: true. IMO we need to change our test code.

Yep that's true.

Actually the test code returns an error as when it is run individually (which is the expected result).

But when run collectively, for some weird reason, the update experiment service returns null error which fails the tc because returned error is null but it expects an error.

Since it is a unit test, it is correct that the test should pass when run individually.

The reason this happens is that in the case of chaos hub, we have code to download a file or check for the existence of a file, but other test code has already downloaded the file.

If you can't fix it now, I suggest commenting it out.

Okay cool!
I'll do it.

namkyu1999 · 2023-11-11T16:53:39Z

chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go

@@ -463,7 +463,7 @@ func TestChaosExperimentHandler_UpdateChaosExperiment(t *testing.T) {

 				chaosExperimentService.On("ProcessExperimentUpdate", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errors.New("experiment update failed")).Once()
 			},
-			wantErr: true,
+			wantErr: false,


name: "Update failed",
This test case should be failed. we need to keep wantErr: true. IMO we need to change our test code.

Yep. The same reason as above, as there is some issue in the ProcessExperimentUpdate service IG.

Currently, I am unable to solve it. Where should we update the current code?

Thank you!

Tried to fix in my PR #4372, reason why tests were passing when ran individually but failing when ran all together is because of the global mocks. Some tests are setting expectations but those expectations weren't met. If we had AssertExpectations it would have highlighted the problem. As a best practice we should avoid having global mocks to avoid cases where one test leaves side-effects causing other tests to fail.

Another way to debug issues why mocks aren't working as expected is by inspecting ExpectedCalls on the mock object. You can either print the ExpectedCalls before every test is run or set breakpoints using runtime.Breakpoint() and debug using dlv https://github.com/go-delve/delve

namkyu1999 · 2023-11-11T16:55:11Z

chaoscenter/graphql/server/pkg/chaoshub/handler/handler.go

@@ -23,7 +23,7 @@ import (
 	"gopkg.in/yaml.v2"
 )

-const DefaultPath = "/tmp/"
+const DefaultPath = "./tmp/"


Can you explain why you changed this code, please?

Indeed. The issue stemmed from the tmp/ directory going unnoticed; the code couldn't locate it.

While one solution could be adding a tmp directory in the root, it seemed irrelevant for the rest of the code.

Consequently, I opted to update it to ./tmp, making it relative to the chaos-hub directory.

Would love to discuss on this as I am not sure how apt the solution is.

I wouldn't recommend changing production code that's currently working well just to make our test code pass. We don't inspect the potential side effects that could occur on that code.

Yep, that's true.
So should we close this PR and then, I'll comment out the failing test in a new PR?

You can complete this in the current PR. I will create a new pull request to address the failing test cases(commented test cases).

Saranya-jena · 2024-06-07T06:16:57Z

Unit tests are now fixed, closing this PR.

SohamRatnaparkhi added 2 commits November 8, 2023 15:44

feat: update mock service choas exp/exp-run files

bcd7f66

Signed-off-by: SohamRatnaparkhi <[email protected]>

refactor/fix: GetClonePath test's returned path

bb1a58a

Signed-off-by: SohamRatnaparkhi <[email protected]>

SohamRatnaparkhi mentioned this pull request Nov 8, 2023

Add actions for chaoscenter unit tests #4244

Merged

9 tasks

SohamRatnaparkhi added 6 commits November 8, 2023 16:23

chore: update .gitignore to include tmp directory

6f1c2b1

Signed-off-by: SohamRatnaparkhi <[email protected]>

feat: add dummy container-kill and podtato-head

4ed37a9

chartserviceversion YAML files. Signed-off-by: SohamRatnaparkhi <[email protected]>

refractor: update default path for charts and fix test case

253ff3a

Signed-off-by: SohamRatnaparkhi <[email protected]>

refractor: fix file path in handler test

8c6523e

Signed-off-by: SohamRatnaparkhi <[email protected]>

feat: update handler path again

f474f26

Signed-off-by: SohamRatnaparkhi <[email protected]>

chore: update .gitignore to include tmp/ directory

c16e97a

Signed-off-by: SohamRatnaparkhi <[email protected]>

namkyu1999 assigned SohamRatnaparkhi Nov 8, 2023

namkyu1999 added the kind/unit-test label Nov 8, 2023

Jonsy13 requested review from Saranya-jena and amityt November 8, 2023 12:07

Saranya-jena approved these changes Nov 10, 2023

View reviewed changes

SohamRatnaparkhi added 2 commits November 11, 2023 19:43

fix: error handling in chaos experiment handler tests

0cfa15d

Signed-off-by: SohamRatnaparkhi <[email protected]>

Merge branch 'master' of https://github.com/litmuschaos/litmus into f…

3651244

…ix/tests

SohamRatnaparkhi force-pushed the fix/tests branch from 42d5815 to 3651244 Compare November 11, 2023 14:14

fix: compilation error in grpc handler tests

4807bee

Signed-off-by: SohamRatnaparkhi <[email protected]>

namkyu1999 requested changes Nov 11, 2023

View reviewed changes

Saranya-jena added the DO NOT MERGE label Nov 15, 2023

smitthakkar96 mentioned this pull request Jan 8, 2024

test(graphql): fix broken tests for chaoscenter/graphql/server and integrate mockery to generate mocks #4372

Merged

9 tasks

Saranya-jena closed this Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: failing backend-unit-tests #4285

fix: failing backend-unit-tests #4285

SohamRatnaparkhi commented Nov 8, 2023 •

edited

Loading

SohamRatnaparkhi commented Nov 8, 2023

Jonsy13 commented Nov 8, 2023 •

edited

Loading

namkyu1999 commented Nov 11, 2023

SohamRatnaparkhi commented Nov 11, 2023

SohamRatnaparkhi commented Nov 11, 2023 •

edited

Loading

namkyu1999 left a comment

namkyu1999 Nov 11, 2023

SohamRatnaparkhi Nov 11, 2023

namkyu1999 Nov 11, 2023

SohamRatnaparkhi Nov 11, 2023 •

edited

Loading

namkyu1999 Nov 11, 2023

SohamRatnaparkhi Nov 11, 2023

namkyu1999 Nov 12, 2023

SohamRatnaparkhi Nov 12, 2023

namkyu1999 Nov 11, 2023

SohamRatnaparkhi Nov 11, 2023 •

edited

Loading

smitthakkar96 Jan 6, 2024 •

edited

Loading

smitthakkar96 Jan 6, 2024

namkyu1999 Nov 11, 2023

SohamRatnaparkhi Nov 11, 2023

namkyu1999 Nov 12, 2023

SohamRatnaparkhi Nov 12, 2023

namkyu1999 Nov 12, 2023

Saranya-jena commented Jun 7, 2024

		succeedProjectID := "project-1"
		succeedName := "workflow-hub-1"

fix: failing backend-unit-tests #4285

fix: failing backend-unit-tests #4285

Conversation

SohamRatnaparkhi commented Nov 8, 2023 • edited Loading

Proposed changes

Types of changes

Checklist

Dependency

Special notes for your reviewer:

SohamRatnaparkhi commented Nov 8, 2023

Jonsy13 commented Nov 8, 2023 • edited Loading

namkyu1999 commented Nov 11, 2023

SohamRatnaparkhi commented Nov 11, 2023

SohamRatnaparkhi commented Nov 11, 2023 • edited Loading

namkyu1999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SohamRatnaparkhi Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SohamRatnaparkhi Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

smitthakkar96 Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saranya-jena commented Jun 7, 2024

SohamRatnaparkhi commented Nov 8, 2023 •

edited

Loading

Jonsy13 commented Nov 8, 2023 •

edited

Loading

SohamRatnaparkhi commented Nov 11, 2023 •

edited

Loading

SohamRatnaparkhi Nov 11, 2023 •

edited

Loading

SohamRatnaparkhi Nov 11, 2023 •

edited

Loading

smitthakkar96 Jan 6, 2024 •

edited

Loading