ci: Build mocks using make #1612

AndrewSisley · 2023-07-05T14:37:04Z

Relevant issue(s)

Resolves #1611

Description

Builds mocks using make, including in the CI tests. Documents how to build them. Enforces consistent mockery version.

codecov · 2023-07-05T14:47:14Z

Codecov Report

Patch coverage has no change and project coverage change: +0.11 🎉

Comparison is base (739b7bf) 73.75% compared to head (cf741cf) 73.86%.

@@             Coverage Diff             @@
##           develop    #1612      +/-   ##
===========================================
+ Coverage    73.75%   73.86%   +0.11%     
===========================================
  Files          193      193              
  Lines        19953    19953              
===========================================
+ Hits         14716    14737      +21     
+ Misses        4148     4132      -16     
+ Partials      1089     1084       -5

Flag	Coverage Δ
all-tests	`73.86% <ø> (+0.11%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 4 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 739b7bf...cf741cf. Read the comment docs.

islamaliev

Overall looks great. Just have some improvements suggestions

islamaliev · 2023-07-05T16:09:31Z

Makefile

+	mockery --srcpkg github.com/ipfs/go-datastore/query --output ./datastore/mocks --name Results --with-expecter
+	mockery --dir ./datastore --output ./datastore/mocks --name RootStore --with-expecter
+	mockery --dir ./datastore --output ./datastore/mocks --name Txn --with-expecter
+	mockery --dir ./datastore --output ./datastore/mocks --name DAGStore --with-expecter


todo: use configuration file for common properties

I think we should use configuration file so that we do not have a lot of repetitive arguments here:
If we just add .mockery.yaml file:

with-expecter: True

It also has another interesting option case: underscore that will produce, for example, file node_getter.go instead of NodeGetter.go, but we might want to discuss it first.

Also we could create a bash script that will be called like this:

genmock --dir ./client --names DB,Colleciton

It can receive a list of interfaces and figure out the output folder ("{$dir}/mocks")
This one (generated by ChatGPT) works for me:

#!/bin/bash # Parse command line arguments while [[ $# -gt 0 ]] do key="$1" case $key in --dir) dir="$2" shift # past argument shift # past value ;; --names) names="$2" shift # past argument shift # past value ;; *) # unknown option shift # past argument ;; esac done # Convert names to array IFS=',' read -r -a namesArray <<< "$names" # Set output directory output="${dir}/mocks" # Run mockery for each name for name in "${namesArray[@]}"; do mockery --dir ${dir} --output ${output} --name ${name} done

Would we want to force with-expecter: True for everything though? Are there any downsides besides a few extra auto-generated lines?

RE the bash, for now at least I don't think such a script would improve the readability of the codebase. I'd much rather have a few extra lines in the make file.

thought: don't need a bash script I think might be overkill.

thought: Could we do all these in a mockery config file, like define all the packages there and a global with-expecter: True and the output dir and everything there?

Example from their documentation:

with-expecter: True packages: github.com/vektra/mockery/v2/pkg: # (1)! interfaces: TypesPackage: RequesterVariadic: config: # (2)! with-expecter: False configs: - mockname: MockRequesterVariadicOneArgument unroll-variadic: False - mockname: MockRequesterVariadic io: config: all: True # (3)! interfaces: Writer: config: with-expecter: False # (4)!

You can privide these as config args in the file:

| name | templated | default | description | |------|-----------|---------|-------------| | `all` | :fontawesome-solid-x: | `#!yaml false` | Generate all interfaces for the specified packages. | | `boilerplate-file` | :fontawesome-solid-x: | `#!yaml ""` | Specify a path to a file that contains comments you want displayed at the top of all generated mock files. This is commonly used to display license headers at the top of your source code. | | `config` | :fontawesome-solid-x: | `#!yaml ""` | Set the location of the mockery config file. | | `dir` | :fontawesome-solid-check: | `#!yaml "mocks/{{.PackagePath}}"` | The directory where the mock file will be outputted to. | | `disable-config-search` | :fontawesome-solid-x: | `#!yaml false` | Disable searching for configuration files | | `disable-version-string` | :fontawesome-solid-x: | `#!yaml false` | Disable the version string in the generated mock files. | | `dry-run` | :fontawesome-solid-x: | `#!yaml false` | Print the actions that would be taken, but don't perform the actions. | | `filename` | :fontawesome-solid-check: | `#!yaml "mock_{{.InterfaceName}}.go"` | The name of the file the mock will reside in. | | `inpackage` | :fontawesome-solid-x: | `#!yaml false` | When generating mocks alongside the original interfaces, you must specify `inpackage: True` to inform mockery that the mock is being placed in the same package as the original interface. | | `mockname` | :fontawesome-solid-check: | `#!yaml "Mock{{.InterfaceName}}"` | The name of the generated mock. | | `outpkg` | :fontawesome-solid-check: | `#!yaml "{{.PackageName}}"` | Use `outpkg` to specify the package name of the generated mocks. | | `log-level` | :fontawesome-solid-x: | `#!yaml "info"` | Set the level of the logger | | [`packages`](features.md#packages-configuration) | :fontawesome-solid-x: | `#!yaml null` | A dictionary containing configuration describing the packages and interfaces to generate mocks for. | | `print` | :fontawesome-solid-x: | `#!yaml false` | Use `print: True` to have the resulting code printed out instead of written to disk. | | [`recursive`](features.md#recursive-package-discovery) | :fontawesome-solid-x: | `#!yaml false` | When set to `true` on a particular package, mockery will recursively search for all sub-packages and inject those packages into the config map. | | `tags` | :fontawesome-solid-x: | `#!yaml ""` | Set the build tags of the generated mocks. | | [`with-expecter`](features.md#expecter-structs) | :fontawesome-solid-x: | `#!yaml true` | Use `with-expecter: True` to generate `EXPECT()` methods for your mocks. This is the preferred way to setup your mocks. | | [`replace-type`](features.md#replace-types) | :fontawesome-solid-x: | `#!yaml null` | Replaces aliases, packages and/or types during generation.|

Links:
https://github.com/vektra/mockery/blob/master/docs/configuration.md
https://github.com/vektra/mockery/blob/master/docs/features.md

thought: Could we do all these in a mockery config file, like define all the packages there and a global with-expecter: True and the output dir and everything there

Similar to the bash-script, I dont think that is worth it right now and might have a negative impact on readability. I also like how simple and easy the individual commands are to read and copy-paste if someone just wants to regenerate a single (or take as a template to generate a new one)

islamaliev · 2023-07-05T16:24:22Z

tests/README.md

-This leads to more generated code, but it removes the need to pass strings around and increases type safety.
-
-For more information on mockery, please refer to the [official repository](https://github.com/vektra/mockery).
+To regenerate the mocks, run `make mock`.  `make test:ci` will also do this.


question: don't you think it's nice to have explanation for how the mocks are being generated?

I'm not attached to this text :D just wonder if this (or some of it) might still be useful.

You are right, I think I have chopped too much off here - I'll put some back (definitely the link at least). Thanks Islam :)

keep docs

shahzadlone

I like the make mock rule to easily help us generate the mocks, left some comments related to adding a config file.

I am very much against and still not convinced we should add that step in the ci. The ci should run the tests on the committed state not a modified state. If the autogen mocks are committed I would be okay with it.

Three solutions I can think of (I will approve) if either one of them is implemented:

Just don't add the step in make test:ci. (devs can run the make test and commit the new mocks if their ci fails and catches out of date mocks).
Clone the ci workflow that runs our tests, leave the original one unchanged and have the new one run with autogenerated mocks.
Have a separate ci workflow that specially checks if mocks are ever outdated(fails if they are), to enforce developers to always keep the mocks updated.

(1) and (2) are very quick, so shouldn't delay this PR from merging at all.

(3) we can do long-term and IMO should also not be too bad to do.

shahzadlone · 2023-07-06T15:30:29Z

Makefile

+	mockery --srcpkg github.com/ipfs/go-datastore/query --output ./datastore/mocks --name Results --with-expecter
+	mockery --dir ./datastore --output ./datastore/mocks --name RootStore --with-expecter
+	mockery --dir ./datastore --output ./datastore/mocks --name Txn --with-expecter
+	mockery --dir ./datastore --output ./datastore/mocks --name DAGStore --with-expecter


thought: don't need a bash script I think might be overkill.

thought: Could we do all these in a mockery config file, like define all the packages there and a global with-expecter: True and the output dir and everything there?

Example from their documentation:

with-expecter: True packages: github.com/vektra/mockery/v2/pkg: # (1)! interfaces: TypesPackage: RequesterVariadic: config: # (2)! with-expecter: False configs: - mockname: MockRequesterVariadicOneArgument unroll-variadic: False - mockname: MockRequesterVariadic io: config: all: True # (3)! interfaces: Writer: config: with-expecter: False # (4)!

You can privide these as config args in the file:

| name | templated | default | description | |------|-----------|---------|-------------| | `all` | :fontawesome-solid-x: | `#!yaml false` | Generate all interfaces for the specified packages. | | `boilerplate-file` | :fontawesome-solid-x: | `#!yaml ""` | Specify a path to a file that contains comments you want displayed at the top of all generated mock files. This is commonly used to display license headers at the top of your source code. | | `config` | :fontawesome-solid-x: | `#!yaml ""` | Set the location of the mockery config file. | | `dir` | :fontawesome-solid-check: | `#!yaml "mocks/{{.PackagePath}}"` | The directory where the mock file will be outputted to. | | `disable-config-search` | :fontawesome-solid-x: | `#!yaml false` | Disable searching for configuration files | | `disable-version-string` | :fontawesome-solid-x: | `#!yaml false` | Disable the version string in the generated mock files. | | `dry-run` | :fontawesome-solid-x: | `#!yaml false` | Print the actions that would be taken, but don't perform the actions. | | `filename` | :fontawesome-solid-check: | `#!yaml "mock_{{.InterfaceName}}.go"` | The name of the file the mock will reside in. | | `inpackage` | :fontawesome-solid-x: | `#!yaml false` | When generating mocks alongside the original interfaces, you must specify `inpackage: True` to inform mockery that the mock is being placed in the same package as the original interface. | | `mockname` | :fontawesome-solid-check: | `#!yaml "Mock{{.InterfaceName}}"` | The name of the generated mock. | | `outpkg` | :fontawesome-solid-check: | `#!yaml "{{.PackageName}}"` | Use `outpkg` to specify the package name of the generated mocks. | | `log-level` | :fontawesome-solid-x: | `#!yaml "info"` | Set the level of the logger | | [`packages`](features.md#packages-configuration) | :fontawesome-solid-x: | `#!yaml null` | A dictionary containing configuration describing the packages and interfaces to generate mocks for. | | `print` | :fontawesome-solid-x: | `#!yaml false` | Use `print: True` to have the resulting code printed out instead of written to disk. | | [`recursive`](features.md#recursive-package-discovery) | :fontawesome-solid-x: | `#!yaml false` | When set to `true` on a particular package, mockery will recursively search for all sub-packages and inject those packages into the config map. | | `tags` | :fontawesome-solid-x: | `#!yaml ""` | Set the build tags of the generated mocks. | | [`with-expecter`](features.md#expecter-structs) | :fontawesome-solid-x: | `#!yaml true` | Use `with-expecter: True` to generate `EXPECT()` methods for your mocks. This is the preferred way to setup your mocks. | | [`replace-type`](features.md#replace-types) | :fontawesome-solid-x: | `#!yaml null` | Replaces aliases, packages and/or types during generation.|

Links:
https://github.com/vektra/mockery/blob/master/docs/configuration.md
https://github.com/vektra/mockery/blob/master/docs/features.md

shahzadlone · 2023-07-06T15:31:25Z

Makefile

+
+.PHONY: mock
+mock:
+	$(MAKE) deps:mock


question: does this need a @.

Suggested change

$(MAKE) deps:mock

@$(MAKE) deps:mock

No strong preference, I can add one if you want

I think it caused a bug on macs, I forget the exact reason. Might as well

Makefile

AndrewSisley · 2023-07-06T16:50:15Z

Three solutions I can think of (I will approve) if either one of them is implemented:

Just don't add the step in make test:ci. (devs can run the make test and commit the new mocks if their ci fails and catches out of date mocks).

Yeah, I'm going to rip this line out. It is too controversial and is delaying the merge of make mock, if someone wants your (2) suggestion they can do it later too, I'm not sure the extra CI time is worth it and it will aggravate any test flakiness issues.

Remove ci stuff

shahzadlone

Thanks for doing the proposed changes, and taking the time to add this rule. Will save dev time for sure!

LGTM

shahzadlone · 2023-07-06T17:07:31Z

Yeah, I'm going to rip this line out. It is too controversial and is delaying the merge of make mock, if someone wants your (2) suggestion they can do it later too, I'm not sure the extra CI time is worth it and it will aggravate any test flakiness issues.

Made a issue to track: #1616

And always in the CI

I'm still in favour of this in the short term, but it is too controversial and is bogging down the highly desired `make mock` command.

Conversation has gone stale and suggested change is easy to make later

## Relevant issue(s) Resolves sourcenetwork#1611 ## Description Builds mocks using make, including in the CI tests. Documents how to build them. Enforces consistent mockery version.

## Relevant issue(s) Resolves #1616 ## Description - Add github action that will fail if there are any out of date mocks - Proposed a year ago here: #1612 (review) ## How has this been tested? - using `act` tool - manually through introducing a mock change in this commit: [`4b20f8f` (#2679)](4b20f8f) and then seeing the action fail here: https://github.com/sourcenetwork/defradb/actions/runs/9361512099/job/25768647150?pr=2679 - the last commit reverts the commit that was introduce the test the mock detection works, I didn't drop the commit and used the revert to have it documented better. Specify the platform(s) on which this was tested: - WSL2

AndrewSisley added area/testing Related to any test or testing suite refactor This issue specific to or requires *notable* refactoring of existing codebases and components labels Jul 5, 2023

AndrewSisley added this to the DefraDB v0.6 milestone Jul 5, 2023

AndrewSisley requested a review from a team July 5, 2023 14:37

AndrewSisley self-assigned this Jul 5, 2023

islamaliev previously requested changes Jul 5, 2023

View reviewed changes

AndrewSisley requested a review from islamaliev July 5, 2023 17:25

shahzadlone requested changes Jul 6, 2023

View reviewed changes

AndrewSisley force-pushed the 1611-regen-mocks-make branch 2 times, most recently from 99487e8 to b71916b Compare July 6, 2023 16:58

AndrewSisley requested a review from shahzadlone July 6, 2023 17:00

shahzadlone approved these changes Jul 6, 2023

View reviewed changes

shahzadlone mentioned this pull request Jul 6, 2023

Add a mandatory ci workflow to indicate when mocks are out dated #1616

Closed

AndrewSisley added 4 commits July 10, 2023 09:43

Build mocks using make

2087547

And always in the CI

PR FIXUP - Add back (with tweaks) the removed test-readme lines

a293814

PR FIXUP - Remove mock regen from ci

1e5b707

I'm still in favour of this in the short term, but it is too controversial and is bogging down the highly desired `make mock` command.

PR FIXUP - Add leading ampersand to Make call

8c09dbe

AndrewSisley force-pushed the 1611-regen-mocks-make branch from cf741cf to 8c09dbe Compare July 10, 2023 13:44

AndrewSisley merged commit 2af0303 into sourcenetwork:develop Jul 10, 2023

AndrewSisley deleted the 1611-regen-mocks-make branch July 10, 2023 13:57

shahzadlone mentioned this pull request Jun 4, 2024

ci(i): Add workflow to detect out of date mocks #2679

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Build mocks using make #1612

ci: Build mocks using make #1612

AndrewSisley commented Jul 5, 2023

codecov bot commented Jul 5, 2023 •

edited

Loading

islamaliev left a comment

islamaliev Jul 5, 2023

AndrewSisley Jul 5, 2023 •

edited

Loading

shahzadlone Jul 6, 2023

AndrewSisley Jul 6, 2023

islamaliev Jul 5, 2023

AndrewSisley Jul 5, 2023 •

edited

Loading

shahzadlone left a comment

shahzadlone Jul 6, 2023

shahzadlone Jul 6, 2023

AndrewSisley Jul 6, 2023

shahzadlone Jul 6, 2023

AndrewSisley Jul 6, 2023

AndrewSisley commented Jul 6, 2023 •

edited

Loading

shahzadlone left a comment

shahzadlone commented Jul 6, 2023

ci: Build mocks using make #1612

ci: Build mocks using make #1612

Conversation

AndrewSisley commented Jul 5, 2023

Relevant issue(s)

Description

codecov bot commented Jul 5, 2023 • edited Loading

Codecov Report

islamaliev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 5, 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

AndrewSisley Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

shahzadlone 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley commented Jul 6, 2023 • edited Loading

shahzadlone left a comment

Choose a reason for hiding this comment

shahzadlone commented Jul 6, 2023

codecov bot commented Jul 5, 2023 •

edited

Loading

AndrewSisley Jul 5, 2023 •

edited

Loading

AndrewSisley Jul 5, 2023 •

edited

Loading

AndrewSisley commented Jul 6, 2023 •

edited

Loading