-
Notifications
You must be signed in to change notification settings - Fork 51
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
add Purley silicon Ignition firmware image + ME FPT parsing test #445
Conversation
Redistributale, taken from https://edk2.groups.io/g/devel/message/50921 We can use this as a fixture for testing pkg/uefi/meregion.go Signed-off-by: Daniel Maslowski <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #445 +/- ##
=======================================
Coverage 34.21% 34.21%
=======================================
Files 209 209
Lines 14086 14086
=======================================
Hits 4820 4820
Misses 8377 8377
Partials 889 889
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
134fc4f
to
0eafcf3
Compare
pkg/uefi/meregion_test.go
Outdated
|
||
fpt, err := NewMEFPT(meRegion) | ||
if err != nil { | ||
t.Errorf("expected no error reading a valid ME FPT") |
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.
might as well make this
t.Fatalf("NewMEFPT: got %v, want nil", err)
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.
IIUC correctly, it may be against conventions, though technically, we may get both an error and a result, right? I'd want to keep the test running to the end, in that case, so that it may uncover another possible error.
0eafcf3
to
f373b45
Compare
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.
very minor tweaks and this can go in
pkg/uefi/meregion_test.go
Outdated
|
||
fpt, err := NewMEFPT(meRegion) | ||
if err != nil { | ||
t.Errorf("expected no error reading a valid ME FPT, got %v", err) |
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.
This should be log.Fatalf, you're not going anywhere when this fails.
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.
That is what this question was about:
#445 (comment)
pkg/uefi/meregion_test.go
Outdated
|
||
fpt, err := NewMEFPT(meRegion) | ||
if err != nil { | ||
t.Errorf("expected no error reading a valid ME FPT, got %v", err) |
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.
The general form of these errors messages, is the got, want format, e..g
reading ME FPT: got %v, want nil
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.
Yea I saw that in the earlier suggestions and find it... irritating/lossy.
Why not provide the detailed idea in the test - which is the spec of behavior?
What I'm saying here is: "I expect this function to return either a result or an error, but never both".
Anyway, will change it if that's the way to go.
pkg/uefi/meregion_test.go
Outdated
} | ||
entries := len(fpt.Entries) | ||
if entries != expectedEntries { | ||
t.Errorf("expected %d entries, got %d", expectedEntries, entries) |
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.
ftp.Entries: got %d, want %d
and so on.
Signed-off-by: Daniel Maslowski <[email protected]>
f373b45
to
8d9bb9d
Compare
Redistributale, taken from https://edk2.groups.io/g/devel/message/50921
We can use this as a fixture for testing pkg/uefi/meregion.go