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.

Sign up for GitHub

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

[Filebeat] fix minor unreachable code #32659

Merged
merged 1 commit into from
Sep 7, 2022
+3 −4
Merged

[Filebeat] fix minor unreachable code #32659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions filebeat/input/file/glob_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,14 @@ func TestGlob(t *testing.T) {
if err != nil {
t.Fatal(err)
}
os.MkdirAll(filepath.Join(root, "foo/bar/baz/qux/quux"), 0o755)
if err := os.MkdirAll(filepath.Join(root, "foo/bar/baz/qux/quux"), 0o755); err != nil {
t.Fatal(err)
}
for _, test := range globTests {
pattern := filepath.Join(root, test.pattern)
matches, err := Glob(pattern, 4)
if err != nil {
t.Fatal(err)
continue
}
var normalizedMatches []string
for _, m := range matches {
Expand Down Expand Up @@ -92,7 +93,6 @@ func TestGlobPatterns(t *testing.T) {
for i, p := range patterns {
if p != test.expectedPatterns[i] {
t.Fatalf("%q expanded to %q instead of %q", test.pattern, patterns, test.expectedPatterns)
break
}
}
}
Expand Down
1 change: 0 additions & 1 deletion filebeat/input/syslog/rfc3164_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,6 @@ func TestPriority(t *testing.T) {
ParserRFC3164([]byte(log), l)
assert.Equal(t, d, l.Priority())
})
return
Copy link
Member

@rdner rdner Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This looks suspicious and it changes how the test ran. Before it was running only once and now for 120 iterations. I think it's safer to remove the loop in order to preserve the existing logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks suspicious and it changes how the test run. Before it was running only once and now for 120 iterations.

Yes. IMO, that was the intent of this test, only the return statement was added by mistake

I think it's safer to remove the loop in order to preserve the existing logic.

I'm keeping an open mind on this.
I'll wait a few hours, and if no other revier has a problem with it, I'll delete the loop.
@efd6 @ShourieG @cmacknz

Copy link
Contributor

Choose a reason for hiding this comment

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

Running the loop only once is not correct IMO. The point of the test is to check that a variety of syslog priorities are properly parsed into the destination value. Testing only a single value (1) is not interesting. I'm not entirely sure why it starts at 1 and stops at 120 since it would not cost significantly more to test the exhaustive range of valid priority values.

}
}

Expand Down