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

pkg/testutil: add in-memory Plugin builder #283

Merged
merged 1 commit into from
Jul 21, 2019

Conversation

ahmetb
Copy link
Member

@ahmetb ahmetb commented Jul 21, 2019

Introducing builder-pattern methods to create in-memory Plugin/Platform objects
that has good defaults, but can be overwritten for test purposes. Readability
of the test files (e.g. validation/*_test.go) now speak for themselves.

The testutil.NewPlugin() returns a valid index.Plugin instance. Any tests
expected to fail can modify its values with the builder pattern. (ditto for
testutil.NewPlatform()).

  • had to move validation code to pkg/index/validation due to a cyclic
    import situation that occurred. This should be fine.
  • renamed some test cases for ease of readability
  • I've thought about adding code validation for **_test.go to not to have any
    index.Plugin{ or index.Platform{ strings. But this might be an overkill.

Fixes #270. Beware of merge conflicts that will raise due to in-flight PRs.

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 21, 2019
@codecov-io

This comment has been minimized.

@codecov-io
Copy link

codecov-io commented Jul 21, 2019

Codecov Report

Merging #283 into master will decrease coverage by 0.15%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
- Coverage   55.07%   54.91%   -0.16%     
==========================================
  Files          19       19              
  Lines         877      874       -3     
==========================================
- Hits          483      480       -3     
  Misses        341      341              
  Partials       53       53
Impacted Files Coverage Δ
pkg/receiptsmigration/migration.go 48.14% <100%> (ø) ⬆️
pkg/index/validation/validate.go 95% <100%> (ø)
cmd/validate-krew-manifest/main.go 58.33% <100%> (ø) ⬆️
pkg/index/indexscanner/scanner.go 69.56% <50%> (ø) ⬆️

Continue to review full report at Codecov.

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

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

This reads very nicely. Well done!

There are some left-over Plugin usages in util_test.go and platform_test.go which could be cleaned up. Did you leave those out intentionally?

pkg/testutil/plugin.go Outdated Show resolved Hide resolved
pkg/info/info_test.go Show resolved Hide resolved
@ahmetb
Copy link
Member Author

ahmetb commented Jul 21, 2019

There are some left-over Plugin usages in util_test.go and platform_test.go which could be cleaned up. Did you leave those out intentionally?

Yeah it seems like I forgot.
Perhaps we can even explore adding a check for preventing index.Plugin{} or index.Platform{} initializations and have all tests use this approach.

@ahmetb
Copy link
Member Author

ahmetb commented Jul 21, 2019

platform_test.go

So looks like platform.go can't use this utility package, because it uses a cyclic import when used in test case Test_matchPlatformToSystemEnvs().

I'm planning to move this method to pkg/installation as pkg/index.PluginSpec isn't the best place to have this method. I'll do this post this PR (#284 to track).

Introducing builder-pattern methods to create in-memory Plugin/Platform objects
that has good defaults, but can be overwritten for test purposes. Readability
of the test files (e.g. validation/*_test.go) now speak for themselves.

The testutil.NewPlugin() returns a _valid_ index.Plugin instance. Any tests
expected to fail can modify its values with the builder pattern. (ditto for
testutil.NewPlatform()).

- had to move validation code to pkg/index/validation due to a cyclic
  import situation that occurred. This should be fine.
- renamed some test cases for ease of readability
- I've thought about adding code validation for **_test.go to not to have any
  index.Plugin{ or index.Platform{ strings. But this might be an overkill atm.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 21, 2019
@corneliusweig
Copy link
Contributor

/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 Jul 21, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, corneliusweig

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:
  • OWNERS [ahmetb,corneliusweig]

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 merged commit 7bd3969 into kubernetes-sigs:master Jul 21, 2019
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. 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. 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.

minor: duplicate test object cleanup
4 participants