itest: separate tapd instance logs into per-node files#1988
itest: separate tapd instance logs into per-node files#1988darioAnongba wants to merge 5 commits intomainfrom
Conversation
Add a build step for the tapd-itest binary in the build-itest target. This binary is needed because tapd instances are now started as separate OS processes during integration tests, enabling per-instance log files.
Refactor tapdHarness to start each tapd instance as a separate OS process (exec.Command) instead of running in-process. This enables per-instance log separation. Each tapd instance now writes to its own log file (e.g., tapd-Alice.log, tapd-Bob.log) in the test log directory, matching the pattern used by lnd and lightning-terminal integration tests. Key changes: - Replace in-process config structs with CLI argument construction - Add start()/stop() using exec.Command with process lifecycle management - Add addLogFile() for per-instance log file creation - Add setCliFlag(), setBoolFlag(), updateLndNode() helper methods - Expose rpcHost(), tlsCertPath, restListenAddr, backoff configs for test access
Remove tap.SetupLoggers call from setupLogging since tapd instances now run as separate OS processes with their own loggers. Aperture logger setup is retained since aperture still runs in-process.
Update test files to use the new tapdHarness accessors instead of the removed clientCfg struct fields.
Add tapd-itest to the artifact upload and chmod steps so the binary is available in CI integration test jobs.
Summary of ChangesHello @darioAnongba, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the debugging experience for multi-node integration tests by isolating the log output of each Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring of the integration test harness. Running tapd as a separate process will greatly improve log clarity and make debugging multi-node test failures much easier. The switch from an in-process configuration to building CLI arguments is handled cleanly, and the new helper functions for modifying flags between test runs are a great addition for flexibility. I have a few suggestions to enhance maintainability and address a minor issue in the new logging setup.
| // configuration. | ||
| // configuration. The tapd instance will be started as a separate OS process | ||
| // with its own log file. | ||
| func newTapdHarness(t *testing.T, ht *harnessTest, cfg tapdConfig, |
There was a problem hiding this comment.
The newTapdHarness function has grown quite large (over 200 lines) due to the refactoring to build CLI arguments. To improve readability and maintainability, consider breaking it down into smaller helper functions. Each helper could be responsible for a specific group of arguments, for example:
buildBaseCliArgs(...)appendProofCourierArgs(args, ...)appendRfqArgs(args, ...)appendDbArgs(args, ...)
This would make the main function a more readable sequence of calls to these helpers.
| // setCliFlag sets or updates a CLI flag in the harness's argument list. | ||
| // This can be used between stop() and start() to modify configuration before | ||
| // restarting. The flag name should not include the "--" prefix. | ||
| func (hs *tapdHarness) setCliFlag(flag, value string) { |
There was a problem hiding this comment.
According to the repository's style guide (rule 18), function comments must begin with the function name, and the examples show it capitalized for exported functions. This comment should start with SetCliFlag. This also applies to SetBoolFlag, UpdateLndNode, and AddLogFile in this file.
| // setCliFlag sets or updates a CLI flag in the harness's argument list. | |
| // This can be used between stop() and start() to modify configuration before | |
| // restarting. The flag name should not include the "--" prefix. | |
| func (hs *tapdHarness) setCliFlag(flag, value string) { | |
| // SetCliFlag sets or updates a CLI flag in the harness's argument list. | |
| // This can be used between stop() and start() to modify configuration before | |
| // restarting. The flag name should not include the "--" prefix. | |
| func (hs *tapdHarness) setCliFlag(flag, value string) { |
References
- Function comments must begin with the function name. (link)
| // Pass the node's stderr to both a buffer and the file, so we can | ||
| // capture errors and also persist them. | ||
| var errb bytes.Buffer | ||
| w := io.MultiWriter(&errb, file) | ||
| hs.cmd.Stderr = w |
There was a problem hiding this comment.
The errb buffer is initialized and written to via io.MultiWriter, but its content is never read. This makes the buffer and the multi-writer seem like dead code. If capturing stderr in a buffer isn't needed, this could be simplified to just redirect stderr to the log file.
| // Pass the node's stderr to both a buffer and the file, so we can | |
| // capture errors and also persist them. | |
| var errb bytes.Buffer | |
| w := io.MultiWriter(&errb, file) | |
| hs.cmd.Stderr = w | |
| // Pass stderr to the file. | |
| hs.cmd.Stderr = file |
Pull Request Test Coverage Report for Build 21921677537Details
💛 - Coveralls |
2 participants
When debugging multi-node integration test failures, all tapd logs were intermingled in console output, making it very difficult to trace which instance produced which log line. With this change, each
tapdinstance writes to its own log file in the test log directory.tapd-itestbinary) during integration tests, enabling per-instance log files (e.g.,tapd-Alice.log,tapd-Bob.log)SetupLoggersfor one instance overwrites all others. This matches the approach used bylndandlightning-terminalitests.setBoolFlag,setCliFlag, andupdateLndNodehelpers for test-time config changes between stop/start cycles.Fixes #1987