-
Notifications
You must be signed in to change notification settings - Fork 138
Adding logging to functional tests #3781
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3781 +/- ##
=======================================
Coverage 86.99% 86.99%
=======================================
Files 128 128
Lines 16434 16434
Branches 62 62
=======================================
Hits 14297 14297
Misses 1960 1960
Partials 177 177 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a2f5c82
to
156a037
Compare
4787775
to
a51d45e
Compare
a51d45e
to
fe688a8
Compare
tests/framework/request.go
Outdated
@@ -20,18 +22,30 @@ func Get( | |||
url, address string, | |||
timeout time.Duration, | |||
headers, queryParams map[string]string, | |||
logging bool, |
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.
Why is this boolean needed? In general, passing booleans to a function is frowned upon because it can be ambiguous from a caller perspective. It also makes a function do multiple things if a boolean is passed, versus the ideal scenario where each function has one purpose.
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 didn't want it, but i cannot find other way to stop printing logs in some scenarious, in all scenarious it is very useful, but there is a case in tracing_test:
sendRequests(helloURL, 25)
sendRequests(worldURL, 25)
sendRequests(helloworldURL, 25)
That looks like a noise when there is 75 times printed request and 75 times printed received body.
Do you think it is ok to leave this long log then? In all other cases it is printed only once and is informative
so. now, for exact this scenario with bool
parameter i can minimize it to the next view, since i don't see any use in seeing details on those requests

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.
You could do something like the following which can enable logging in a clearer way, and only need to update the function call for the times when you want to disable logging:
package main
import (
"fmt"
)
// Option represents a functional option
type Option func(*Options)
// Options holds configuration for the function
type Options struct {
logEnabled bool
}
// WithLoggingDisabled is a functional option that disables logging
func WithLoggingDisabled() Option {
return func(opts *Options) {
opts.logEnabled = false
}
}
// MyFunction performs some logic and optionally logs
func MyFunction(opts ...Option) {
// Default options (logging enabled)
options := &Options{
logEnabled: true,
}
// Apply functional options to modify the default behavior
for _, opt := range opts {
opt(options)
}
// Perform the main logic
fmt.Println("Executing function logic")
// Perform logging if enabled
if options.logEnabled {
fmt.Println("Logging is enabled")
}
}
func main() {
// Call the function with default behavior (logging enabled)
fmt.Println("Case 1: Default logging enabled")
MyFunction()
// Call the function with logging disabled
fmt.Println("\nCase 2: Logging disabled")
MyFunction(WithLoggingDisabled())
}
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.
Thank you, added this approach!
fe688a8
to
b0cd789
Compare
b0cd789
to
d394c64
Compare
@@ -0,0 +1,13 @@ | |||
package framework | |||
|
|||
type Option func(*Options) |
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 moved it to separate in case if it can be needed not only for requests in the future
d394c64
to
5a9b319
Compare
5a9b319
to
0125c08
Compare
Proposed changes
Problem: As a developer of NGF
I want the NFR and functional tests to log as they execute
So I can more easily debug a failure when it occurs.
Solution: - Log statements are added to all non-functional and functional tests to include assertions, requests, responses, and anywhere there is a chance for an error to occur.
Testing: Tested with Plus and without, compared failures output with new logging and without
Closes #2449
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes