Closed
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3115 +/- ##
==========================================
- Coverage 60.54% 60.52% -0.02%
==========================================
Files 598 598
Lines 39885 39889 +4
==========================================
- Hits 24147 24143 -4
- Misses 15738 15746 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
leongross
force-pushed
the
ci/errcheck
branch
3 times, most recently
from
f601c90 to
7c25401
Compare
leongross
force-pushed
the
ci/errcheck
branch
from
7c25401 to
9bde7fe
Compare
leongross
force-pushed
the
ci/errcheck
branch
from
9bde7fe to
294bee3
Compare
3 tasks
rminnich
requested changes
Sep 28, 2024
Member
rminnich
left a comment
rminnich
left a comment
There was a problem hiding this comment.
I think golangci-lint is failing badly here, it is unaware of how flag.Parse works.
This needs to be rethought in many places.
rminnich
added
the
Awaiting author
leongross
force-pushed
the
ci/errcheck
branch
2 times, most recently
from
1dbd814 to
44b4a69
Compare
rminnich
reviewed
Oct 16, 2024
add linter option 'unparam' to ci Signed-off-by: leongross <leon.gross@9elements.com>
* fix unparam issues, remove unused error returns * fix changed tests when no error is returned anymore * add TODOs and //nolint:unparam tags for future rework/refactoring Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
leongross
force-pushed
the
ci/errcheck
branch
7 times, most recently
from
9f74632 to
52843d9
Compare
leongross
commented
Mar 3, 2025
Comment on lines
+226
to
+228
| if err := handle.SetSocketReceiveBufferSize(bufSize, true); err != nil { | ||
| return cmd, fmt.Errorf("parseFlags: %w", err) | ||
| } |
Member
Author
There was a problem hiding this comment.
This causes the unit test for ip to fail:
ip_linux_test.go:136: unexpected error: parseFlags: operation not permitted
--- FAIL: TestParseFlags (0.00s)
--- PASS: TestParseFlags/no_args (0.00s)
--- PASS: TestParseFlags/inet4 (0.00s)
--- PASS: TestParseFlags/inet6 (0.00s)
--- PASS: TestParseFlags/mpls (0.00s)
--- PASS: TestParseFlags/bridge (0.00s)
--- PASS: TestParseFlags/link (0.00s)
--- PASS: TestParseFlags/family (0.00s)
--- PASS: TestParseFlags/family_inet6 (0.00s)
--- PASS: TestParseFlags/family_err (0.00s)
--- PASS: TestParseFlags/resolve (0.00s)
--- PASS: TestParseFlags/color (0.00s)
--- FAIL: TestParseFlags/rcvBuf (0.00s
In this case, the SetSocketReceiveBufferSize function error is just not checked. It seems that running this with non-root permissions fails, but since the error is not checked, we don't catch this error.
Comment on lines
+51
to
+52
| t.Errorf("got %v, want nil", err) | ||
| } |
Member
Author
There was a problem hiding this comment.
The test for PCI fail for the cases 2,3,4:
=== RUN TestRun
=== RUN TestRun/switch_hexdump_case_1
=== RUN TestRun/switch_hexdump_case_2
pci_linux_test.go:51: got permission denied, want nil
=== RUN TestRun/switch_hexdump_case_3
pci_linux_test.go:51: got permission denied, want nil
=== RUN TestRun/switch_hexdump_case_4
pci_linux_test.go:51: got permission denied, want nil
=== RUN TestRun/readJSON_true,_without_error
=== RUN TestRun/readJSON_true,_error_in_os.ReadFile
=== RUN TestRun/readJSON_true,_error_in_json.Unmarshal
=== RUN TestRun/dumpJSON
=== RUN TestRun/invoke_registers
--- FAIL: TestRun (0.03s)
Again, permission denied errors indicate that we should move this to vmtest root permissioned integration tests.
Comment on lines
+181
to
+183
| if err := session.Run("echo hello u-root"); err != nil { | ||
| t.Fatalf("can't run command: %v", err) | ||
| } |
Member
Author
There was a problem hiding this comment.
Uncovers unhandled error:
=== RUN TestSessionRun
2025/03/03 14:44:54 127.0.0.1:41516 logged in with key SHA256:fzAJZugrckV2ucFAxumbs7eIMsDcar11KOUKojV/4D0
2025/03/03 14:44:54 Request exec
2025/03/03 14:44:54 Executing non-PTY command zsh [-c echo hello u-root]
2025/03/03 14:44:54 Exit status 0
sshd_test.go:182: can't run command: EOF
--- FAIL: TestSessionRun (0.13s)
FAIL
FAIL github.com/u-root/u-root/cmds/core/sshd 0.157s
FAIL
Signed-off-by: leongross <leon.gross@9elements.com>
Member
Author
|
Closing this for now. Reworking the error strategies in u-root should follow a systematic approach that we define (e.g. when we use nolint directives etc). We should define an easy-to-follow pattern so that we do not have to discuss this on each and every PR. Until we establish this I close this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up PR on #3111, currently rebased on its branch until it is merged to circumvent merge conflicts and duplicates.