pkg/reflectutil: add unit tests for UnknownNonEmptyFields#4919
pkg/reflectutil: add unit tests for UnknownNonEmptyFields#4919varsha-0007 wants to merge 2 commits intolima-vm:masterfrom
Conversation
jandubois
left a comment
jandubois
left a comment
There was a problem hiding this comment.
Please also address linter errors.
Bonus suggestions from Claude Code for additional test coverage:
- Multiple unknown fields: every unknown-field assertion uses
cmp.Contains(result, "Unknown"), which doesn't check that onlyUnknownis reported, nor the result's full contents/order. A case with two unknown non-empty fields and anassert.DeepEqualagainst a sorted/expected slice would catch regressionscmp.Containswon't.- Mixed known + unknown: no case where some non-empty fields are known and others aren't (the realistic usage pattern).
- Nil pointer:
UnknownNonEmptyFields((*testStruct)(nil))callsorigVal.Elem()on anilpointer, thenval.NumField()panics. If the documented contract is "panics on nil", add a test; if not, this is a latent bug worth surfacing to the maintainers rather than silently leaving uncovered.
|
@jandubois Sure, will look into it! Thanks for the detailed review! 🙂 |
| }, | ||
| { | ||
| // Unknown is non-empty and not in known list — must be reported. | ||
| name: "one unknown field", |
There was a problem hiding this comment.
This is actually a mix of one known and one unknown field, and an exact duplicate of the mixed case later in this file.
| expected: []string{"Meta"}, | ||
| }, | ||
| { | ||
| // Reviewer suggestion: multiple unknown fields at once. |
There was a problem hiding this comment.
Please don't include "history" comments. We have git blame and GitHub issue and PR comments for that.
Comments in the source code should explain the WHAT and WHY, but not the history behind changes.
| defer func() { | ||
| if r := recover(); r == nil { | ||
| t.Errorf("expected panic but did not panic") | ||
| } |
There was a problem hiding this comment.
Maybe add a check on the panic message, so you are not silently accepting a different panic than the one you are expecting?
| ) | ||
|
|
||
| // testStruct is used as the input type for all test cases. | ||
| // Count field removed — it was unused in any test case (reviewer feedback). |
There was a problem hiding this comment.
Don't explain the history.
| // Count field removed — it was unused in any test case (reviewer feedback). |
|
You have fixed the linter errors but have not addressed my feedback. |
|
@jandubois, will do it now!! |
jandubois
left a comment
jandubois
left a comment
There was a problem hiding this comment.
There still is a linter error, and please squash your commits!
varsha-0007
force-pushed
the
add-reflectutil-tests
branch
from
3dd6d28 to
c37de27
Compare
| @@ -1,18 +1,31 @@ | |||
| // SPDX-FileCopyrightText: Copyright The Lima Authors | |||
|
|
|||
There was a problem hiding this comment.
What's up with all the random blank lines you have inserted?
If you create/update your changes with AI, then you MUST review them before submitting them to the PR. Please don't waste reviewer time; otherwise it will be faster if we use AI ourselves to create these kind of PRs.
There was a problem hiding this comment.
You're right, I should have reviewed the changes more carefully before pushing. I apologize for wasting your time. Fixing now.
varsha-0007
force-pushed
the
add-reflectutil-tests
branch
2 times, most recently
from
740f1c0 to
d4c5cdd
Compare
varsha-0007
force-pushed
the
add-reflectutil-tests
branch
from
d4c5cdd to
580866d
Compare
Signed-off-by: varsha <varsha.narvi.07@gmail.com>
2 participants
pkg/reflectutilhas no test file, unlike other utility packagessuch as
pkg/ptr,pkg/textutil, andpkg/osutil.This PR adds
reflectutil_test.gocovering the following cases: