Skip to content

Split-up git test into unit and integration parts #1017

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

Merged
merged 1 commit into from
Jan 21, 2020
Merged

Split-up git test into unit and integration parts #1017

merged 1 commit into from
Jan 21, 2020

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Jan 15, 2020

We now use a dedicated file-name pattern for integration tests and unit
tests. The unit tests introduce a new layer of mockability via the
public Repository and Worktree interfaces. These interfaces are
used inside the tests to spy on the method call.

Needs #1014

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 15, 2020
@saschagrunert saschagrunert changed the title Split-up git test into unit and integration parts WIP: Split-up git test into unit and integration parts Jan 15, 2020
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 15, 2020
@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 15, 2020

err := testRepo.sut.Push(Master)
require.Nil(t, err)
func (r *repoMock) CommitObject(plumbing.Hash) (*object.Commit, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TL;DR: I'd highly recommend counterfeiter (or something similar) over implementing the fakes yourself.

Instead of implementing the fakes yourself, you could use counterfeiter (& go generate).

E.g. this allows you to just run go generate ./... and it will create / update the fake for you. The fake then has nice methods to check on call counts and call args, stub whole method calls, inject fake return values, and those kinda things. When you are generating fakes for exported interfaces/funcs it will also add a compile time assertion to check if the fake actually implements the interface (to avoid drift). However, you can also fake unexported stuff just fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay cool, I'm now using the counterfeiter generated mocks. The thing is that we now have to provide two additional methods to overwrite the inner and worktree to avoid making them public... I also cannot use the same package because I will get a cyclic import if I do so...

@saschagrunert saschagrunert changed the title WIP: Split-up git test into unit and integration parts Split-up git test into unit and integration parts Jan 17, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 17, 2020
@saschagrunert
Copy link
Member Author

This is ready for review, too :)

@justaugustus
Copy link
Member

This looks great from my feeble testing understanding, but I'd like to leave @hoegaarden and @hasheddan as final approvers here.

/hold unhold once they approve

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 19, 2020
Copy link
Contributor

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

Glad to see the use of counterfeiter here :)

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2020
@justaugustus
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 20, 2020
@saschagrunert
Copy link
Member Author

Had to rebase on top of the latest master branch.

We now use a dedicated file-name pattern for integration tests and unit
tests. The unit tests introduce a new layer of mockability via the
public `Repository` and `Worktree` interfaces. These interfaces are
used inside the tests to spy on the method call.

Signed-off-by: Sascha Grunert <[email protected]>
@justaugustus
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 21, 2020
@k8s-ci-robot k8s-ci-robot merged commit fadb585 into kubernetes:master Jan 21, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hasheddan, justaugustus, saschagrunert

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • pkg/OWNERS [hasheddan,justaugustus,saschagrunert]

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

@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jan 21, 2020
@saschagrunert saschagrunert deleted the git-test-split branch January 21, 2020 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/release Categorizes an issue or PR as relevant to SIG Release. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants