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

chore: Creates mock generation infrastructure #1763

Merged
merged 27 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
add48d9
use mockery
lantoli Dec 17, 2023
b41ba5e
don't include test utilities in build
lantoli Dec 17, 2023
cc90022
Revert "don't include test utilities in build"
lantoli Dec 17, 2023
0cbacbc
TEMPORARY: testing mock change detection
lantoli Dec 17, 2023
630cd1b
Revert "TEMPORARY: testing mock change detection"
lantoli Dec 17, 2023
bb449fb
centralize lint version in make file
lantoli Dec 17, 2023
fee2a7e
fix call to mockery
lantoli Dec 17, 2023
c76d184
timeout in linter
lantoli Dec 17, 2023
d171e9b
TEMPORARY - delete mock file
lantoli Dec 17, 2023
a3dd559
Revert "TEMPORARY - delete mock file"
lantoli Dec 17, 2023
194a843
use assert
lantoli Dec 17, 2023
05ed39b
TEMPORARY - failing unit test
lantoli Dec 17, 2023
481fd10
Revert "TEMPORARY - failing unit test"
lantoli Dec 17, 2023
9520446
use testify mock
lantoli Dec 17, 2023
92abb8f
change service name
lantoli Dec 17, 2023
19e3159
use state
lantoli Dec 17, 2023
8907cd8
disable version mockery generated files
lantoli Dec 17, 2023
b761422
refactor response
lantoli Dec 17, 2023
a125d08
responses in one line each
lantoli Dec 17, 2023
5b7f3ad
rename err
lantoli Dec 17, 2023
7d1595d
clarify file detection message
lantoli Dec 18, 2023
4b4066b
add info in contributing file
lantoli Dec 18, 2023
3b478f7
move timeout to config file
lantoli Dec 18, 2023
5ef6390
TEMPORARY - linter fail
lantoli Dec 19, 2023
233ce19
Revert "TEMPORARY - linter fail"
lantoli Dec 19, 2023
7191cd1
remove asdf comment
lantoli Dec 19, 2023
650b772
revert lint changes
lantoli Dec 19, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions .github/workflows/code-health.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,30 @@ jobs:
runs-on: ubuntu-latest
marcosuma marked this conversation as resolved.
Show resolved Hide resolved
steps:
AgustinBettati marked this conversation as resolved.
Show resolved Hide resolved
- uses: actions/checkout@v4
- name: Set up Go
uses: actions/setup-go@v5
- uses: actions/setup-go@v5
lantoli marked this conversation as resolved.
Show resolved Hide resolved
with:
go-version-file: 'go.mod'
- name: Mock generation
run: make tools && mockery
- name: Check for uncommited files
Copy link
Member Author

@lantoli lantoli Dec 17, 2023

Choose a reason for hiding this comment

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

taken from Atlas CLI.
We don't run mockery in each make build so it doesn't get slow, but we detect if it was not run when pushing the commits.

run: |
export FILES=$(git ls-files -o -m --directory --exclude-standard --no-empty-directory)
export LINES=$(echo "$FILES" | awk 'NF' | wc -l)
if [ $LINES -ne 0 ]; then
echo "Detected files that need to be committed:"
echo "$FILES" | sed -e "s/^/ /"
echo ""
echo "Mock skeletons are not up-to-date, you may have forgotten to run mockery before committing your changes."
exit 1
fi
- name: Build
run: make build
unit-test:
needs: build
runs-on: ubuntu-latest
permissions:
pull-requests: write
Comment on lines -28 to -29
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we weakening the permissions?

Copy link
Member Author

@lantoli lantoli Dec 18, 2023

Choose a reason for hiding this comment

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

@gssbzn the intention was to simplify the action, what problems or risks do you see removing this? (I'm not sure why we're defining permissions in this job but not in others)

Copy link
Collaborator

Choose a reason for hiding this comment

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

at some point we'll have to do some audit of gh actions permissions and be sure GH actions only use what they need, I'm not sure when was this permission added or why, a blame to understand the context would be great before trying to change it

I started some of this auditing on the CLI using https://github.com/ossf/scorecard but it's quite consuming and I haven't had the time to dig deeper on it

Copy link
Member Author

Choose a reason for hiding this comment

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

i see, thanks for the info, it looks like we'll need to a focused effort on this and change all workflows.

steps:
- name: Checkout
uses: actions/checkout@v4
- name: Set up Go
uses: actions/setup-go@v5
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version-file: 'go.mod'
- name: Unit Test
Expand Down Expand Up @@ -70,14 +78,11 @@ jobs:
uses: golangci/golangci-lint-action@v3.7.0
with:
version: v1.55.0
args: --timeout 10m
Copy link
Member Author

Choose a reason for hiding this comment

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

timeout is in config file now

website-lint:
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
- name: Install Go
uses: actions/setup-go@v5
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version-file: 'go.mod'
- name: website lint
Expand Down
3 changes: 1 addition & 2 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,8 @@ issues:
text: "^hugeParam: req is heavy"

run:
timeout: 10m
tests: true
build-tags:
- integration
skip-dirs:
- internal/mocks
modules-download-mode: readonly
11 changes: 11 additions & 0 deletions .mockery.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
with-expecter: false
Copy link
Member Author

@lantoli lantoli Dec 17, 2023

Choose a reason for hiding this comment

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

go generate syntax is deprecated in favor of mockery config file

disable-version-string: true
dir: internal/testutil/mocksvc
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] since your interfaces are not centralized don't centralize the mocks, it leads to a lot of hooks if an interface happens to share a name and also to a lot of issues later on when you refactor your interfaces

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks. I wanted to start simple, it's very easy to change at any moment to have mocks in their resource folders.

Copy link
Collaborator

Choose a reason for hiding this comment

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

just be vigilant this doesn't become the permanent state, I'm currently trying to de-tangle some mock mess in the CLI and collocation would've made this easier

outpkg: mocksvc
filename: "{{ .InterfaceName | snakecase }}.go"
mockname: "{{.InterfaceName}}"

packages:
? github.com/mongodb/terraform-provider-mongodbatlas/internal/service/searchdeployment
: interfaces:
DeploymentService:
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,8 @@ To do this you can:
- Helper methods must have their own tests, e.g. `common_advanced_cluster_test.go` has tests for `common_advanced_cluster.go`.
- `internal/testutils/acc` contains helper test methods for Acceptance and Migration tests.
- Tests that need the provider binary like End-to-End tests don’t belong to the source code packages and go in `test/e2e`.
- [Testify Mock](https://pkg.go.dev/github.com/stretchr/testify/mock) and [Mockery](https://github.com/vektra/mockery) are used for test doubles in unit tests. Mocked interfaces are generated in folder `internal/testutil/mocksvc`.


### Creating New Resource and Data Sources

Expand Down
2 changes: 2 additions & 0 deletions GNUmakefile
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ VERSION=$(GITTAG:v%=%)
LINKER_FLAGS=-s -w -X 'github.com/mongodb/terraform-provider-mongodbatlas/version.ProviderVersion=${VERSION}'

GOLANGCI_VERSION=v1.55.0
MOCKERY_VERSION=v2.38.0

export PATH := $(shell go env GOPATH)/bin:$(PATH)
export SHELL := env PATH=$(PATH) /bin/bash
Expand Down Expand Up @@ -74,6 +75,7 @@ tools: ## Install dev tools
go install github.com/terraform-linters/tflint@v0.49.0
go install github.com/rhysd/actionlint/cmd/actionlint@latest
go install golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment@latest
go install github.com/vektra/mockery/v2@$(MOCKERY_VERSION)
Copy link
Collaborator

Choose a reason for hiding this comment

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

won't this pollute the go.mod file?

Copy link
Member Author

Choose a reason for hiding this comment

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

uhm, it didn' change anything in go.mod

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool, double checking sometimes when doing go install and not using latest things get added to the go.mod

curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell go env GOPATH)/bin $(GOLANGCI_VERSION)

.PHONY: check
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,155 +3,126 @@ package searchdeployment_test
import (
"context"
"errors"
"log"
"net/http"
"reflect"
"testing"
"time"

"github.com/mongodb/terraform-provider-mongodbatlas/internal/common/conversion"
"github.com/mongodb/terraform-provider-mongodbatlas/internal/common/retrystrategy"
"github.com/mongodb/terraform-provider-mongodbatlas/internal/service/searchdeployment"
"github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/mocksvc"
"github.com/stretchr/testify/assert"
"go.mongodb.org/atlas-sdk/v20231115002/admin"
)

type stateTransitionTestCase struct {
expectedResult *admin.ApiSearchDeploymentResponse
name string
mockResponses []SearchDeploymentResponse
expectedError bool
var (
updating = "UPDATING"
idle = "IDLE"
unknown = ""
sc400 = conversion.IntPtr(400)
sc500 = conversion.IntPtr(500)
sc503 = conversion.IntPtr(503)
)

type testCase struct {
expectedState *string
name string
mockResponses []response
expectedError bool
}

func TestSearchDeploymentStateTransition(t *testing.T) {
testCases := []stateTransitionTestCase{
testCases := []testCase{
{
name: "Successful transition to IDLE",
mockResponses: []SearchDeploymentResponse{
{
DeploymentResp: responseWithState("UPDATING"),
},
{
DeploymentResp: responseWithState("IDLE"),
},
mockResponses: []response{
{state: &updating},
{state: &idle},
},
expectedResult: responseWithState("IDLE"),
expectedError: false,
expectedState: &idle,
expectedError: false,
},
{
name: "Successful transition to IDLE with 503 error in between",
mockResponses: []SearchDeploymentResponse{
{
DeploymentResp: responseWithState("UPDATING"),
},
{
DeploymentResp: nil,
HTTPResponse: &http.Response{StatusCode: 503},
Err: errors.New("Service Unavailable"),
},
{
DeploymentResp: responseWithState("IDLE"),
},
mockResponses: []response{
{state: &updating},
{statusCode: sc503, err: errors.New("Service Unavailable")},
{state: &idle},
},
expectedResult: responseWithState("IDLE"),
expectedError: false,
expectedState: &idle,
expectedError: false,
},
{
name: "Error when transitioning to an unknown state",
mockResponses: []SearchDeploymentResponse{
{
DeploymentResp: responseWithState("UPDATING"),
},
{
DeploymentResp: responseWithState(""),
},
mockResponses: []response{
{state: &updating},
{state: &unknown},
},
expectedResult: nil,
expectedError: true,
expectedState: nil,
expectedError: true,
},
{
name: "Error when API responds with error",
mockResponses: []SearchDeploymentResponse{
{
DeploymentResp: nil,
HTTPResponse: &http.Response{StatusCode: 500},
Err: errors.New("Internal server error"),
},
mockResponses: []response{
{statusCode: sc500, err: errors.New("Internal server error")},
},
expectedResult: nil,
expectedError: true,
expectedState: nil,
expectedError: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
mockService := MockSearchDeploymentService{
MockResponses: tc.mockResponses,
}

resp, err := searchdeployment.WaitSearchNodeStateTransition(context.Background(), dummyProjectID, "Cluster0", &mockService, testTimeoutConfig)

if (err != nil) != tc.expectedError {
t.Errorf("Case %s: Received unexpected error: %v", tc.name, err)
}

if !reflect.DeepEqual(tc.expectedResult, resp) {
t.Errorf("Case %s: Response did not match expected output", tc.name)
svc := mocksvc.NewDeploymentService(t)
marcosuma marked this conversation as resolved.
Show resolved Hide resolved
ctx := context.Background()
for _, resp := range tc.mockResponses {
svc.On("GetAtlasSearchDeployment", ctx, dummyProjectID, clusterName).Return(resp.get()...).Once()
Copy link
Member Author

@lantoli lantoli Dec 17, 2023

Choose a reason for hiding this comment

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

mock programming

}
resp, err := searchdeployment.WaitSearchNodeStateTransition(ctx, dummyProjectID, "Cluster0", svc, testTimeoutConfig)
Copy link
Member Author

Choose a reason for hiding this comment

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

we call the SUT (System Under Test) passing the mock collaborator

assert.Equal(t, tc.expectedError, err != nil)
assert.Equal(t, responseWithState(tc.expectedState), resp)
svc.AssertExpectations(t)
Copy link
Member Author

Choose a reason for hiding this comment

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

check mock expectations

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just want to point out that "AssertExpectations asserts that everything specified with On and Return was in fact called as expected. Calls may have occurred in any order." (link) In some tests we might want to avoid this call and instead use other methods such as AssertNumberOfCalls, AssertNotCalled, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for the clarification

})
}
}

func TestSearchDeploymentStateTransitionForDelete(t *testing.T) {
testCases := []stateTransitionTestCase{
testCases := []testCase{
{
name: "Regular transition to DELETED",
mockResponses: []SearchDeploymentResponse{
{
DeploymentResp: responseWithState("UPDATING"),
},
{
DeploymentResp: nil,
HTTPResponse: &http.Response{StatusCode: 400},
Err: errors.New(searchdeployment.SearchDeploymentDoesNotExistsError),
},
mockResponses: []response{
{state: &updating},
{statusCode: sc400, err: errors.New(searchdeployment.SearchDeploymentDoesNotExistsError)},
},
expectedError: false,
},
{
name: "Error when API responds with error",
mockResponses: []SearchDeploymentResponse{
{
DeploymentResp: nil,
HTTPResponse: &http.Response{StatusCode: 500},
Err: errors.New("Internal server error"),
},
mockResponses: []response{
{statusCode: sc500, err: errors.New("Internal server error")},
},
expectedError: true,
},
{
name: "Failed delete when responding with unknown state",
mockResponses: []SearchDeploymentResponse{
{
DeploymentResp: responseWithState("UPDATING"),
},
{
DeploymentResp: responseWithState(""),
},
mockResponses: []response{
{state: &updating},
{state: &unknown},
},
expectedError: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
mockService := MockSearchDeploymentService{
MockResponses: tc.mockResponses,
}

err := searchdeployment.WaitSearchNodeDelete(context.Background(), dummyProjectID, clusterName, &mockService, testTimeoutConfig)

if (err != nil) != tc.expectedError {
t.Errorf("Case %s: Received unexpected error: %v", tc.name, err)
svc := mocksvc.NewDeploymentService(t)
ctx := context.Background()
for _, resp := range tc.mockResponses {
svc.On("GetAtlasSearchDeployment", ctx, dummyProjectID, clusterName).Return(resp.get()...).Once()
}
err := searchdeployment.WaitSearchNodeDelete(ctx, dummyProjectID, clusterName, svc, testTimeoutConfig)
assert.Equal(t, tc.expectedError, err != nil)
svc.AssertExpectations(t)
})
}
}
Expand All @@ -162,7 +133,10 @@ var testTimeoutConfig = retrystrategy.TimeConfig{
Delay: 0,
}

func responseWithState(state string) *admin.ApiSearchDeploymentResponse {
func responseWithState(state *string) *admin.ApiSearchDeploymentResponse {
if state == nil {
return nil
}
return &admin.ApiSearchDeploymentResponse{
GroupId: admin.PtrString(dummyProjectID),
Id: admin.PtrString(dummyDeploymentID),
Expand All @@ -172,26 +146,20 @@ func responseWithState(state string) *admin.ApiSearchDeploymentResponse {
NodeCount: nodeCount,
},
},
StateName: admin.PtrString(state),
StateName: state,
}
}

type MockSearchDeploymentService struct {
MockResponses []SearchDeploymentResponse
index int
type response struct {
state *string
statusCode *int
err error
}

func (a *MockSearchDeploymentService) GetAtlasSearchDeployment(ctx context.Context, groupID, clusterName string) (*admin.ApiSearchDeploymentResponse, *http.Response, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

the mock is auto-generated now

if a.index >= len(a.MockResponses) {
log.Fatal(errors.New("no more mocked responses available"))
func (r *response) get() []interface{} {
var httpResp *http.Response
if r.statusCode != nil {
httpResp = &http.Response{StatusCode: *r.statusCode}
}
resp := a.MockResponses[a.index]
a.index++
return resp.DeploymentResp, resp.HTTPResponse, resp.Err
}

type SearchDeploymentResponse struct {
DeploymentResp *admin.ApiSearchDeploymentResponse
HTTPResponse *http.Response
Err error
return []interface{}{responseWithState(r.state), httpResp, r.err}
}
Loading