-
Notifications
You must be signed in to change notification settings - Fork 363
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
Initial receipt type #512
Initial receipt type #512
Conversation
Add receipt helper to testutil
internal/testutil/receipt.go
Outdated
func (r *Receipt) WithName(s string) *Receipt { r.v.ObjectMeta.Name = s; return r } | ||
func (r *Receipt) WithShortDescription(v string) *Receipt { r.v.Spec.ShortDescription = v; return r } | ||
func (r *Receipt) WithTypeMeta(v metav1.TypeMeta) *Receipt { r.v.TypeMeta = v; return r } | ||
func (r *Receipt) WithPlatforms(v ...index.Platform) *Receipt { r.v.Spec.Platforms = v; return r } | ||
func (r *Receipt) WithVersion(v string) *Receipt { r.v.Spec.Version = v; return r } | ||
func (r *Receipt) V() index.Receipt { return r.v } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not copy it like this.
Something that is much smaller like the following would be enough:
NewReceipt().WithPlugin(p).WithStatus()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could just remove instead, since I only added this for the validation test that I was unsure of. What do you think about just removing this for now? It will probably come up later but its not needed once I remove the validation stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't see enough reasons for this to exist yet.
Codecov Report
@@ Coverage Diff @@
## master #512 +/- ##
==========================================
- Coverage 59.12% 58.85% -0.28%
==========================================
Files 23 23
Lines 1003 1011 +8
==========================================
+ Hits 593 595 +2
- Misses 350 358 +8
+ Partials 60 58 -2
Continue to review full report at Codecov.
|
Interesting. Travis builds seem to be broken right now. I guess we either need to disable travis or this test for now :) |
@corneliusweig do you mind Making the error more Verbose in Test_fetchLatestTag_GitHubAPI? I suspect it’s hitting API rate limits of travis servers. |
if got.Name != "foo" && got.Kind != "Plugin" { | ||
t.Errorf("ReadReceiptFromFile() has not parsed the metainformations %v", got) | ||
return | ||
if cmp.Diff(got.Plugin, receipt) != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmp.Diff
already formats the differences in an easy to read way. You should do
if diff := cmp.Diff(got.Plugin, receipt); diff != "" {
t.Errorf("Parsed receipt has unexpected content: %s", diff)
}
@@ -90,6 +93,56 @@ func TestReadPluginFile(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestReadReceiptFile(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not particularly happy with the test below, as it exercises only a few things. For example, it doesn't check whether the status fields are parsed correctly.
We just need 1 test case that makes sure the fields are parsed decently. and the json:"foo,omitempty"
field-tags are accurate.
So I suggest we do this:
-
Add a few receipt test files in
testdata/receipts/foo.yaml
-
Parse the receipt file from disk.
-
Build as
NewReceipt().WithXxx(...)....V()
and usecmp.Diff
to do equality check.
@corneliusweig wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, so far Receipt
does not have any extra fields beside the ones from Plugin
. So we are cutting corners by parsing a plugin yaml as a receipt. I think that's ok for now, but we need to ensure that we extend this test as suggested by Ahmet.
And I'm in for the receipt builder pattern. The sooner we have it, the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah in the next PR, we should add the status
field and start working with it to develop tests like this (where we can roundtrip the type i.e. write/parse a YAML file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is this ok as is for now while status doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah not so useful but helps us pass the tests.
feel free to remove it altogether or add TODO that captures comment above, but this should be the next PR anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll just remove it from this PR and add the tests in the next one when they can be more meaningful with the new status field.
We can go ahead with this if you merge the small outstanding style comments. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chriskim06, 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:
Approvers can indicate their approval by writing |
ccd99e3
to
29c6f53
Compare
/lgtm |
This refactors the receipts to use a new
Receipt
type. The tests for it are pretty much identical to the old ones but using the new type. Once the status object gets added to receipts the tests will need to be updated.I decided to not do
type Receipt Plugin
, only because I figured this would be changed very soon and would save a bit of work down the line. If you'd like me to change it and start out without the embedded type just let me know.Related issues: #483, #507