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.

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

e2e: add logfile for e2e tests #1775

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

parikshithb
Copy link
Member

@parikshithb parikshithb commented Jan 28, 2025

  • Creates a log file (ramen-e2e.log) in the current directory by default.
  • Log DEBUG level messages to the file and INFO level messages to the console.
  • Remove caller (file:lineno) from console logs for cleaner output.
  • Reverted 2cd063a

Fixes: #1722

@parikshithb parikshithb marked this pull request as draft January 28, 2025 12:15
@nirs
Copy link
Member

nirs commented Jan 28, 2025

Fixes: #1722

Good to link the issue, but we should start a PR with a commit message describing the change. We can describe what was done and what will be done later. This helps people to review the change.

e2e/main_test.go Outdated
}

log := logger.Sugar()
executableName := "ramen-e2e"
Copy link
Member

Choose a reason for hiding this comment

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

Should be part of CreateLogger()

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this. Using private constant called logFilePath in test/log

e2e/main_test.go Outdated Show resolved Hide resolved
e2e/main_test.go Outdated
log := logger.Sugar()
executableName := "ramen-e2e"
logger := util.CreateLogger(executableName, false)
defer logger.Sync()
Copy link
Member

Choose a reason for hiding this comment

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

See kubectl-gather for how to handle the return value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still facing lint issues wrt to defer, will fix it.

e2e/main_test.go Outdated Show resolved Hide resolved
e2e/util/logger.go Outdated Show resolved Hide resolved
"go.uber.org/zap/zapcore"
)

func CreateLogger(executableName string, verbose bool) *zap.SugaredLogger {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need the executable name or verbose flag.

The executable name is available in os.Args[0], but it may be "go" when we run the tests as go test ..., and "ramen-e2e" when we run from the executable. It will be best to support both use cases. If using os.Args[0] does not work, it is fine to hard code the default log name using private constant in this file.

We can add later a --logfile argument and use it here, but lets defer this work so we can have the default log file available sooner. Using custom logfile is nice to have and we can add it when after we finished with more important stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

wrt using os.Args[0]:

// Determine the log file name, fallback to "ramen-e2e.log" if running via go test
logFileName := "ramen-e2e.log" // Default log file name
if len(os.Args) > 0 {
    if os.Args[0] == "go" {
        // If we are running with `go test`, use the default log file name
        logFileName = "ramen-e2e.log"
    } else {
        // If we are running the executable directly, use the base name of the executable
        logFileName = filepath.Base(os.Args[0]) + ".log"
    }
}

Currently i have hardcoded logfile name to ramen-e2e.log. What do you think about above code suggestion for os.Args[0]?

Copy link
Member

Choose a reason for hiding this comment

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

Lets use constant log name. Add a private constant in the log.go.

When we add a --logfile option the option will use a default value anyway.

e2e/util/logger.go Outdated Show resolved Hide resolved
e2e/util/logger.go Outdated Show resolved Hide resolved
e2e/util/logger.go Outdated Show resolved Hide resolved
@nirs
Copy link
Member

nirs commented Jan 28, 2025

#1723 was merged, so you need to revert 2cd063a as part of your pr.

e2e/main_test.go Show resolved Hide resolved
e2e/util/logger.go Outdated Show resolved Hide resolved
This commit introduces a dedicated log file for e2e tests:

* Creates a log file (ramen-e2e.log) in the current directory by default.
* Log DEBUG level messages to the file and INFO level messages to the console.
* Remove caller (file:lineno) from console logs for cleaner output.

Signed-off-by: Parikshith <[email protected]>
This reverts commit 2cd063a.

The original commit redirected e2e test logs to ramen-e2e.log using tee.
The new logger writes to ramen-e2e.log by default

Signed-off-by: Parikshith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

e2e: Add logfile
2 participants