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

[smitthakkar96]

Proposed changes

While working on #4370 I noticed that tests are broken on master (example) making it difficult to verify the correctness of my changes. This PR aims to fix the broken tests and, as an added bonus, introduce mockery to make it easy to generate mocks for the interfaces.

Why did moving global mocks to mocks per test fix the flakiness?

Since we had global mocks, some unused mock expectations were leaving side effects, causing other tests to fail when the whole suite was run together. You can read explanation in this or this comment. Also attaching a small example:

package main

import (
	"errors"
	"fmt"
	"testing"

	"github.com/stretchr/testify/mock"
)

type MyMockedObject struct {
	mock.Mock
}

func (m *MyMockedObject) DoSomething(number int) (bool, error) {

	args := m.Called(number)
	return args.Bool(0), args.Error(1)

}

var (
	testObj = new(MyMockedObject)
)

func TestSomethingElse(t *testing.T) {

	testObj.On("DoSomething", mock.Anything).Return(true, nil).Once()

	targetFuncThatDoesSomethingWithObj(testObj)

	testObj.On("DoSomething", mock.Anything).Return(false, errors.New("This is a side-effect")).Once()

}

func TestSomethingElse2(t *testing.T) {
	testObj.On("DoSomething", mock.Anything).Return(true, nil).Once()

	targetFuncThatDoesSomethingWithObj(testObj)
}

func targetFuncThatDoesSomethingWithObj(obj *MyMockedObject) {
	return_value, err := obj.DoSomething(1000)
	fmt.Printf("DoSomething return values bool=%v error=%v\n", return_value, err)
	fmt.Println("Calls = ", obj.Calls)
}

❯ go test -v ./... -count=1
=== RUN   TestSomethingElse
DoSomething return values bool=true error=<nil>
Calls =  [{*mock.Mock<0xc000014410> DoSomething [1000] [] [/Users/s.thakkar/projects/temp/side-effects/main_test.go:17 /Users/s.thakkar/projects/temp/side-effects/main_test.go:43 /Users/s.thakkar/projects/temp/side-effects/main_test.go:30] 0 0 false <nil> 0 <nil> <nil> []}]
--- PASS: TestSomethingElse (0.00s)
=== RUN   TestSomethingElse2
DoSomething return values bool=false error=This is a side-effect
Calls =  [{*mock.Mock<0xc000014410> DoSomething [1000] [] [/Users/s.thakkar/projects/temp/side-effects/main_test.go:17 /Users/s.thakkar/projects/temp/side-effects/main_test.go:43 /Users/s.thakkar/projects/temp/side-effects/main_test.go:30] 0 0 false <nil> 0 <nil> <nil> []} {*mock.Mock<0xc000014410> DoSomething [1000] [] [/Users/s.thakkar/projects/temp/side-effects/main_test.go:17 /Users/s.thakkar/projects/temp/side-effects/main_test.go:43 /Users/s.thakkar/projects/temp/side-effects/main_test.go:39] 0 0 false <nil> 0 <nil> <nil> []}]
--- PASS: TestSomethingElse2 (0.00s)
PASS
ok  	side-effects	0.833s

Why some mock calls were removed?

As explained in this comment they were unused.

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

• Please add the links to the dependent PR need to be merged before this (if any).

Special notes for your reviewer:

[namkyu1999]

LGTM 🚀 Introducing mockery seems like a good idea.

[namkyu1999]

Could you clarify why we need to move global variables to local variables?

[smitthakkar96]

@namkyu1999 I explained here in the comment #4285 (comment), I will update the PR description and also add AssertExpectations in the tests to help avoid similar situation where unmet expectations are causing weird behaviour. Will also address the codacy analysis thanks for your prompt review :)

[namkyu1999]

Could you also check Codacy Static Code Analysis ?

[smitthakkar96]

Just an update:

After adding AssertExpectations as expected, I was able to find unused mock expectations

--- FAIL: TestChaosExperimentHandler_SaveChaosExperiment (0.00s)
    --- FAIL: TestChaosExperimentHandler_SaveChaosExperiment/Save_Chaos_Experiment (0.00s)
        handler_test.go:69: PASS:	ProcessExperiment(*model.ChaosExperimentRequest,string,string)
        handler_test.go:69: PASS:	ProcessExperimentUpdate(*model.ChaosExperimentRequest,string,string,string,bool,string,string)
        handler_test.go:69: FAIL:	ProcessExperimentCreation(string,string,string,string,string,string,<nil>)
            		at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:125 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:213]
        handler_test.go:69: FAIL: 2 out of 3 expectation(s) were met.
            	The code you are testing needs to make 1 more call(s).
            	at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:69 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:219]
        handler_test.go:73: PASS:	Get(string,int,string)
    --- FAIL: TestChaosExperimentHandler_SaveChaosExperiment/Failed_to_create_experiment (0.00s)
        handler_test.go:69: PASS:	ProcessExperiment(*model.ChaosExperimentRequest,string,string)
        handler_test.go:69: PASS:	ProcessExperimentUpdate(*model.ChaosExperimentRequest,string,string,string,bool,string,string)
        handler_test.go:69: FAIL:	ProcessExperimentCreation(string,string,string,string,string,string,<nil>)
            		at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:205 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:213]
        handler_test.go:69: FAIL: 2 out of 3 expectation(s) were met.
            	The code you are testing needs to make 1 more call(s).
            	at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:69 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:219]
        handler_test.go:73: PASS:	Get(string,int,string)
FAIL
FAIL	github.com/litmuschaos/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler	0.919s
FAIL

--- FAIL: TestChaosExperimentHandler_DeleteChaosExperiment (0.00s)
    --- FAIL: TestChaosExperimentHandler_DeleteChaosExperiment/success:_Delete_Chaos_Experiment (0.00s)
        handler_test.go:65: FAIL:	ProcessExperimentUpdate(string,string,string,string,string,string,string)
            		at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:272 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:344]
        handler_test.go:65: FAIL:	ProcessExperimentDelete(string,string,string,string)
            		at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:274 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:344]
        handler_test.go:65: FAIL: 0 out of 2 expectation(s) were met.
            	The code you are testing needs to make 2 more call(s).
            	at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:65 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:350]
        handler_test.go:66: PASS:	ProcessExperimentRunDelete(string,string,string,string,string,string,string)
        handler_test.go:69: PASS:	Get(string,int,string)
        handler_test.go:69: PASS:	Get(string,int,string)
    --- FAIL: TestChaosExperimentHandler_DeleteChaosExperiment/failure:_mongo_error_while_retrieving_the_experiment_run_details (0.00s)
        handler_test.go:65: FAIL:	ProcessExperimentUpdate(string,string,string,string,string,string,string)
            		at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:310 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:344]
        handler_test.go:65: FAIL: 0 out of 1 expectation(s) were met.
            	The code you are testing needs to make 1 more call(s).
            	at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:65 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:350]
        handler_test.go:69: PASS:	Get(string,int,string)
        handler_test.go:69: PASS:	Get(string,int,string)
    --- FAIL: TestChaosExperimentHandler_DeleteChaosExperiment/failure:_unable_to_delete_experiment-runs (0.00s)
        handler_test.go:65: FAIL:	ProcessExperimentUpdate(string,string,string,string,string,string,string)
            		at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:329 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:344]
        handler_test.go:65: FAIL:	ProcessExperimentDelete(string,string,string,string)
            		at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:332 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:344]
        handler_test.go:65: FAIL: 0 out of 2 expectation(s) were met.
            	The code you are testing needs to make 2 more call(s).
            	at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:65 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:350]
        handler_test.go:66: PASS:	ProcessExperimentRunDelete(string,string,string,string,string,string,string)
        handler_test.go:69: PASS:	Get(string,int,string)
        handler_test.go:69: PASS:	Get(string,int,string)
        handler_test.go:69: FAIL:	Get(string,int,string)
            		at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:334 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:344]
        handler_test.go:69: FAIL: 2 out of 3 expectation(s) were met.
            	The code you are testing needs to make 1 more call(s).
            	at: [/Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:69 /Users/s.thakkar/projects/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler/handler_test.go:350]
FAIL
FAIL	github.com/litmuschaos/litmus/chaoscenter/graphql/server/pkg/chaos_experiment/handler	0.768s
FAIL

I am working on cleaning this up now and ensuring mocks aren't shared between two tests

[smitthakkar96]

@namkyu1999 I am bit lost with codacy checks on the markdown file. It seems to be a conflicting opinion. Could please point me in right direction? Thanks in advance :)

Also updated the PR, could you please take another look?

[smitthakkar96]

@namkyu1999 Codacy checks seem to have passed; adding three spaces after - solved it; however, across the whole repo, it seems like we are not following this rule. It also seems a bit ugly to add three spaces, shall we adjust the configuration to 1 space instead? https://github.com/remarkjs/remark-lint/tree/main/packages/remark-lint-list-item-indent#options

[namkyu1999]

@namkyu1999 Codacy checks seem to have passed; adding three spaces after - solved it; however, across the whole repo, it seems like we are not following this rule. It also seems a bit ugly to add three spaces, shall we adjust the configuration to 1 space instead? https://github.com/remarkjs/remark-lint/tree/main/packages/remark-lint-list-item-indent#options

What do you think @Saranya-jena ?

[Saranya-jena]

@smitthakkar96 @namkyu1999 Since this is not a mandatory check, we can ignore this error. Also if you want to contribute on updating this codacy configuration, please feel free to create an issue for the same, we would love to have this contribution.