Improve logging and e2e script #99
Conversation
📝 WalkthroughWalkthroughThis pull request introduces new command-line options and parameters to control functionality in the end-to-end testing workflow. The script is updated with Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as e2e.sh Script
participant D as Dependency Directory
participant T as Test Runner
U->>S: Execute script with (--clean, --debug)
S->>S: Parse options and set CLEAN & DEBUG flags
alt CLEAN flag set
S->>D: Check & remove existing dependency directory
else
S->>D: Verify if dependency directory exists
Note right of S: Skip download/build if directory exists
end
S->>T: Assemble parameters (include debug flag if enabled)
T-->>S: Execute end-to-end tests
sequenceDiagram
participant F as Test Flag
participant TS as NewSuite Function
participant L as Logger
F->>TS: Pass debug flag value
TS->>L: Configure logging level (Debug if true, Info otherwise)
L-->>TS: Logger initialized accordingly
TS-->>F: Test suite ready
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for "maunium.net/go/mautrix/id"" ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/e2e.sh (2)
31-38: Fix indentation consistency.The flag handling section uses tabs while the rest of the file uses spaces. Maintain consistent indentation throughout the script.
- --clean) - CLEAN=1 - shift - ;; - --debug) - DEBUG=1 - shift - ;; + --clean) + CLEAN=1 + shift + ;; + --debug) + DEBUG=1 + shift + ;;
183-186: Make array handling more robust.The array initialization and usage could be more robust by ensuring it's always properly initialized, even when empty.
-ADD_PARAM=() -if [ $DEBUG -eq 1 ] ; then - ADD_PARAM+=("-debug") -fi +# Initialize array with default empty value +declare -a ADD_PARAM=() + +# Add debug flag if enabled +if [ $DEBUG -eq 1 ] ; then + ADD_PARAM+=("-debug") +fi + +# Ensure array is properly expanded even when empty +if [ ${#ADD_PARAM[@]} -eq 0 ]; then + ADD_PARAM=("") +fitests/e2e/blockchain/network.go (2)
60-60: LGTM! Log level changes align with PR objectives.The changes appropriately move verbose operational logs to debug level, which helps reduce log clutter during normal runs while maintaining the ability to access detailed information when needed via the new
--debugflag.A few observations:
- The changes are consistent across all methods (StartNewNetwork, startNewNode, Stop)
- The log messages maintain their informative content
- The modifications align with the PR's goal of streamlining logging output
Consider adding debug logs for additional operations that might be useful during troubleshooting:
- Node configuration details
- Genesis configuration summary
- Network cleanup operations
Example addition:
func (n *Network) startNewNode(...) (*Node, chan error, error) { n.logger.Debugf("Starting blockchain node %d (http port %d)...", nodeIndex, httpPort) + n.logger.Debugf("Node configuration: staking port %d, bootstrap IDs %s, bootstrap IPs %s", + stakingPort, bootstrapIDsArg, bootstrapIPsArg)Also applies to: 183-183, 189-189, 195-195, 261-261, 313-313, 336-336
206-206: Consider moving UseExistingNetwork logs to debug level for consistency.While StartNewNetwork and other methods have been updated to use debug level logging, UseExistingNetwork still uses info level. For consistency with the PR's objective of moving verbose output to debug level, consider updating these log statements as well.
- logger.Info("Connecting to existing network...") + logger.Debug("Connecting to existing network...") - logger.Info("Preparing EVM admin...") + logger.Debug("Preparing EVM admin...") - logger.Info("Preparing CMB contracts...") + logger.Debug("Preparing CMB contracts...") - logger.Info("Blockchain network started") + logger.Debug("Blockchain network started")Also applies to: 218-218, 224-224, 230-230
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
scripts/e2e.sh(4 hunks)tests/e2e/blockchain/network.go(5 hunks)tests/e2e/blockchain/node.go(1 hunks)tests/e2e/bot/bot.go(1 hunks)tests/e2e/bot/factory.go(1 hunks)tests/e2e/e2e_test.go(3 hunks)tests/e2e/matrix/conduit.go(3 hunks)tests/e2e/partner_plugin/factory.go(1 hunks)tests/e2e/partner_plugin/partner_plugin.go(1 hunks)tests/e2e/tests/suite.go(3 hunks)
✅ Files skipped from review due to trivial changes (5)
- tests/e2e/partner_plugin/factory.go
- tests/e2e/partner_plugin/partner_plugin.go
- tests/e2e/blockchain/node.go
- tests/e2e/bot/factory.go
- tests/e2e/matrix/conduit.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit Tests
- GitHub Check: Build docker image
- GitHub Check: E2E Tests
- GitHub Check: Static Analysis
🔇 Additional comments (5)
tests/e2e/bot/bot.go (1)
40-40: LGTM! Appropriate log level adjustment.Moving the bot stop message to debug level is appropriate as it's non-critical status information.
tests/e2e/e2e_test.go (2)
32-33: LGTM! Well-structured debug flag implementation.The debug flag is properly defined and integrated with appropriate default value and description.
Also applies to: 44-44, 56-56
96-106: LGTM! Proper debug flag propagation.The debug flag is correctly passed to the NewSuite function, enabling configurable logging levels.
tests/e2e/tests/suite.go (2)
43-49: LGTM! Well-implemented logging configuration.The zap logger configuration properly handles the debug flag, defaulting to InfoLevel and switching to DebugLevel when debug is enabled.
156-156: LGTM! Consistent log level adjustment.Moving service status messages to debug level aligns with the PR's objective of reducing verbose output during normal operation.
Also applies to: 182-182
Summary by CodeRabbit