chore: Creates mock generation infrastructure #1763
Conversation
.github/workflows/code-health.yml
Outdated
| - name: Build | ||
| run: make build | ||
| - name: Linter | ||
| run: make lint |
There was a problem hiding this comment.
linter here takes 2m instead of 3m and linter version is centralized in make file
There was a problem hiding this comment.
I am not sure how it was before 🤔 I don't understand the advantages of adding these changes, maybe you could help me understand here. Could you clarify the current state without your changes and what your changes are trying to achieve? Thanks!
There was a problem hiding this comment.
the main advantage about the linter change is that we'll have the version only in one place, now it's in 2 places and we need to remember every time we upgrade it:
https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/GNUmakefile#L16
https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/.github/workflows/code-health.yml#L72
There was a problem hiding this comment.
with this change we use make lint from GH action so version is only in one place
There was a problem hiding this comment.
oh I see, thanks. I thought this was related somehow to the mock test but it is not.
There was a problem hiding this comment.
FYI by moving out of the golangci lint action to the make file version you are loosing the action ability to comment on the line with the failure and you gonna have to parse the log files to figure out what broke
There was a problem hiding this comment.
@gssbzn that's fair but we already have a pre-commit hook with linter so linter should rarely fail in GH
There was a problem hiding this comment.
Gus made an interesting comment on that. And I guess your answer seems more like "this is a duplicate" - so my point is that either it's useful having linter here (hence we need to make the results easily actionable, hence this does not seem a good idea) or it's not.
Since it's not actually related to the change you are proposing in the PR I would suggest to remove this change the things as it is. Your pros mentioned above to Andrea as well does not look that strong to me compared to Gus's comment.
There was a problem hiding this comment.
I pushed an offending commit (bypasing commit hooks) with current linter and the one in this PR to compare
Current linter: https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/7258950919/job/19775168073
Running [/home/runner/golangci-lint-1.55.0-linux-amd64/golangci-lint run --out-format=github-actions --timeout 10m] in [] ...
Error: test helper function should start from tb.Helper() (thelper)
Error: issues found
Ran golangci-lint in 169508ms
Linter in this PR: https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/7259036317/job/19775411159?pr=1763
golangci-lint run
Error: internal/testutil/acc/skip.go:18:[6](https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/7259036317/job/19775411159?pr=1763#step:8:7): test helper function should start from tb.Helper() (thelper)
func SkipIfTFAccNotDefined(tb testing.TB) {
^
make: *** [GNUmakefile:68: lint] Error 1
Error: Process completed with exit code 2.
So apart from having the linter version only in one place, here we have information about the offending file and line whereas in the current approach we only see the error not where it happens.
Please can you clarify why you think the current situation is better? Also I'm not sure I understand the part "the action ability to comment on the line with the failure ". thanks!
There was a problem hiding this comment.
I am not gonna try to answer on behalf of Gus, but I just have a question: would we see this linter error on GH if we move the linter in this new place?
| run: make tools | ||
| - name: Mock generation | ||
| run: mockery | ||
| - name: Check for uncommited files |
There was a problem hiding this comment.
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.
.github/workflows/code-health.yml
Outdated
| @@ -56,28 +68,11 @@ jobs: | |||
| with: | |||
| recreate: true | |||
| path: code-coverage-results.md | |||
| lint: | |||
There was a problem hiding this comment.
lint done in build to centralize version in make file
| @@ -0,0 +1,10 @@ | |||
| with-expecter: false | |||
There was a problem hiding this comment.
go generate syntax is deprecated in favor of mockery config file
| svc := mocksvc.NewDeploymentService(t) | ||
| ctx := context.Background() | ||
| for _, resp := range tc.mockResponses { | ||
| svc.On("GetAtlasSearchDeployment", ctx, dummyProjectID, clusterName).Return(resp.get()...).Once() |
| } | ||
|
|
||
| func (a *MockSearchDeploymentService) GetAtlasSearchDeployment(ctx context.Context, groupID, clusterName string) (*admin.ApiSearchDeploymentResponse, *http.Response, error) { |
There was a problem hiding this comment.
the mock is auto-generated now
| resp, err := searchdeployment.WaitSearchNodeStateTransition(ctx, dummyProjectID, "Cluster0", svc, testTimeoutConfig) | ||
| assert.Equal(t, tc.expectedError, err != nil) | ||
| assert.Equal(t, responseWithState(tc.expectedState), resp) | ||
| svc.AssertExpectations(t) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
thanks for the clarification
| } | ||
| resp, err := searchdeployment.WaitSearchNodeStateTransition(ctx, dummyProjectID, "Cluster0", svc, testTimeoutConfig) |
There was a problem hiding this comment.
we call the SUT (System Under Test) passing the mock collaborator
|
@lantoli Not sure I understand why we use mockery in github actions. Shoudn't we generate the mock skeleton while developing to be able to create the tests? |
.github/workflows/code-health.yml
Outdated
| @@ -89,11 +84,11 @@ jobs: | |||
| - name: Run ShellCheck | |||
| uses: bewuethr/shellcheck-action@v2 | |||
| call-acceptance-tests-workflow: | |||
| needs: [build, lint, shellcheck, unit-test, website-lint] | |||
| needs: [build, shellcheck, unit-test, website-lint] | |||
There was a problem hiding this comment.
why did you remove lint here? if the lint fails we should not run the test
There was a problem hiding this comment.
the lint is now running inside build, so threre is still a dependency on it
| @@ -0,0 +1,71 @@ | |||
| // Code generated by mockery. DO NOT EDIT. | |||
|
|
|||
| package mocksvc | |||
There was a problem hiding this comment.
why do we have two mock packages?
| permissions: | ||
| pull-requests: write |
There was a problem hiding this comment.
why are we weakening the permissions?
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
i see, thanks for the info, it looks like we'll need to a focused effort on this and change all workflows.
| @@ -74,7 +75,9 @@ 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) | |||
There was a problem hiding this comment.
won't this pollute the go.mod file?
There was a problem hiding this comment.
uhm, it didn' change anything in go.mod
There was a problem hiding this comment.
cool, double checking sometimes when doing go install and not using latest things get added to the go.mod
| @@ -0,0 +1,11 @@ | |||
| with-expecter: false | |||
| disable-version-string: true | |||
| dir: internal/testutil/mocksvc | |||
There was a problem hiding this comment.
[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
There was a problem hiding this comment.
thanks. I wanted to start simple, it's very easy to change at any moment to have mocks in their resource folders.
There was a problem hiding this comment.
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
|
there is not mock file change detection in CLI pre-commit hooks |
lantoli
requested review from
gssbzn,
AgustinBettati,
oarbusi and
andreaangiolillo
lantoli
changed the title
This reverts commit 5ef6390.
| @@ -70,14 +78,11 @@ jobs: | |||
| uses: golangci/golangci-lint-action@v3.7.0 | |||
| with: | |||
| version: v1.55.0 | |||
| args: --timeout 10m | |||
There was a problem hiding this comment.
timeout is in config file now
|
Description
Creates mock generation infrastructure.
Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-218288
Type of change:
Required Checklist:
Further comments