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

feat: add test module #55

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ThatsMrTalbot
Copy link
Contributor

@ThatsMrTalbot ThatsMrTalbot commented Jan 24, 2024

Adds a simple module to add targets to a shared "make test" target. Also includes a built in target for testing golang packages

Signed-off-by: Adam Talbot <adam.talbot@venafi.com>
@jetstack-bot jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Jan 24, 2024
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sgtcodfish for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 24, 2024
# See the License for the specific language governing permissions and
# limitations under the License.

## Run golang tests
Copy link
Member

Choose a reason for hiding this comment

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

What does "golang" mean in this context?
Generally we have unit and e2e tests using both ginkgo and gotestsum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just thinking unit tests since e2e tests usually require extra setup so should setup their own target. I can reword the target to test-go-unit or something?

## @category [shared] Test
.PHONY: test-go
test-go: | $(NEEDS_GOTESTSUM)
$(if $(go_test_targets),$(GOTESTSUM) $(addprefix --junitfile=, $(junitfile)) -- $(go_test_targets))
Copy link
Member

@inteon inteon Jan 24, 2024

Choose a reason for hiding this comment

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

Is this general enough to model tests like https://github.com/cert-manager/approver-policy/blob/main/make/test-smoke.mk#L58-L63 here? & is that what we are aiming for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends if we care much about the runner, I only used gotestsum because I saw cert-manager repo using it and it already existed as a tool. Happy to use ginkgo instead if thats easier.

Of could use a flag to specify what runner you want then have a ifdef else statement here

Copy link
Member

Choose a reason for hiding this comment

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

The challenge is that unit tests sometimes use gotestsum and e2e tests use ginkgo or another combination of both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could not define any go_test_targets and just add your own target to shared_test_targets. Then you could use whatever runner you like.

Copy link
Contributor Author

@ThatsMrTalbot ThatsMrTalbot Jan 24, 2024

Choose a reason for hiding this comment

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

You have given me an idea for an improvement though 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants