debugging functionality added #1569
Conversation
|
@sundanc It seems that this is not passing the tests; some of this is related to a common error, but others are down to you - if you don't understand why, please investigate and then ask in the channel :) It would also be helpful to split this into separate commits - these are independent changes, it seems? |
|
Thank you for your feedback. I've addressed the linting issues in Regarding the failing tests, I understand my commit message format needs to follow the project's convention with I also see that I should have separated these changes into different commits since they're independent. In the future, I'll split changes to address different concerns into separate commits for better clarity and easier review. Let me know if there's anything else I need to address. |
sundanc
force-pushed
the
main
branch
2 times, most recently
from
388c6e5 to
2758132
Compare
|
@neiljp Finally, it passed all the checks. I cleaned up broken & unformatted codes that are in my forked repo commit history. |
There was a problem hiding this comment.
@sundanc There's a lot to cover here, and I can definitely see some of it being helpful 👍
I've left inline comments which could help with understanding some points, but until those are clarified:
- an update to the README to indicate the profiling technique would clear up what it is (it's only in the source, otherwise?)
- a connectivity and server-reporting feature could be useful, as a standalone tool
Re my earlier point about commits, if these changes were in individual independent commits, I may have been able to easily merge them separately first. Or some could have been squashed together later into related changes.
| debug: venv | ||
| @echo "=== Running with debug enabled ===" | ||
| $(PYTHON) -m zulipterminal.cli.run -d | ||
|
|
||
| debug-profile: venv | ||
| @echo "=== Running with profiling enabled ===" | ||
| $(PYTHON) -m zulipterminal.cli.run --profile |
There was a problem hiding this comment.
My concern with these is that the extra options available to the run module cannot easily be added.
While eg. themes could be debugged by changing the config file, some options are only available on the command-line, such as 'explore mode' right now.
| debug-clean: | ||
| @echo "=== Cleaning debug files ===" | ||
| rm -f debug.log zulip-terminal.prof zulip-terminal-tracebacks.log |
There was a problem hiding this comment.
This certainly cleans files from the current folder, but not necessarily from where the files are accumulating.
Here I've only seen profile files in /tmp/, and with a random name; have you seen it in the local directory? Do you also see the zulip-terminal.prof name?
I'm also a little wary of making it too easy to delete the other files, since while that information can accumulate and it's not clear which is worth keeping, it can also often be really useful! I suspect a more refined solution could be to build upon the move to use .config and store debugging and tracebacks in different folders by name/date/branch/etc.
| #### Remote debugging example | ||
|
|
||
| Here's a complete example of how to debug a specific issue: | ||
|
|
||
| 1. Let's say you suspect an issue when sending messages. Find the function in the codebase that handles this (e.g., in `zulipterminal/ui/ui.py`). | ||
|
|
||
| 2. Add the debugger statement just before the suspicious code: | ||
| ```python | ||
| def send_message(self): | ||
| from pudb.remote import set_trace | ||
| set_trace() # This will pause execution here | ||
| # Rest of the function... | ||
| ``` | ||
|
|
||
| 3. Run Zulip Terminal with debug mode enabled: | ||
| ```bash | ||
| zulip-term -d | ||
| ``` | ||
|
|
||
| 4. When you trigger the send_message function, check debug.log for telnet connection details: | ||
| ```bash | ||
| tail -f debug.log | ||
| ``` | ||
|
|
||
| 5. Connect with telnet and you'll get an interactive debugger to step through the code. |
There was a problem hiding this comment.
How does this compare to the section above?
| # Added debug helper with proper path | ||
| "zulip-term-debug = zulipterminal.scripts.debug_helper:main", |
There was a problem hiding this comment.
This makes it available to users who install the basic package, not just run it from the source tree; is that the intention?
| elif args.command == "run": | ||
| cmd = ["zulip-term", "-d"] | ||
| if args.profile: | ||
| cmd.append("--profile") | ||
| logger.info("Running: %s", " ".join(cmd)) | ||
| subprocess.run(cmd, check=False) |
There was a problem hiding this comment.
This seems to simply wrap commands, as per:
zulip-term-debug runvszulip-term -dzulip-term-debug run --profilevszulip-term -d --profile
What is the benefit of this?
| # Check for color support | ||
| colors = os.environ.get("TERM", "unknown") | ||
| logger.info("TERM environment: %s", colors) | ||
|
|
||
| if "COLORTERM" in os.environ: | ||
| logger.info("COLORTERM: %s", os.environ["COLORTERM"]) | ||
|
|
||
| # Check for Unicode support | ||
| logger.info("Testing Unicode rendering capabilities:") | ||
| test_chars = [ | ||
| ("Basic symbols", "▶ ◀ ✓ ✗"), | ||
| ("Emoji (simple)", "😀 🙂 👍"), | ||
| ("Box drawing", "│ ┌ ┐ └ ┘ ├ ┤ ┬ ┴ ┼"), | ||
| ("Math symbols", "∞ ∑ √ ∫ π"), | ||
| ] |
There was a problem hiding this comment.
This looks like something that could belong in render_symbols.py, or perhaps an extension of it?
That existing tool checks the symbols that are currently being used, rather than an assortment of sample ones.
| logger.info("No obvious errors found in the log file.") | ||
|
|
||
|
|
||
| def test_connectivity(server_url: Optional[str] = None) -> None: |
There was a problem hiding this comment.
While the main zulip-term script does check for connectivity, and the server version can be listed from within it, I can see a simple tool along these lines being useful.
However, rather than having a script with semi-duplicate code from run.py, it would be useful to use existing functions from there - it looks very similar to it?
There is plenty of server information available other than the version, which could also be used to help with authentication issues, for example.
| content = f.read() | ||
|
|
||
| # Look for error patterns | ||
| error_patterns = [r"ERROR", r"Exception", r"Traceback", r"Failed to"] |
There was a problem hiding this comment.
This can find lots of issues (my files are quite large right now!), but could you give an example of how it has helped you? Do you have a specific motivation for this?
I would expect that reading through the file to get multiple lines of context would be more helpful?
If this is also for reporting errors to maintainers/developers, then context can be useful.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def analyze_debug_log(log_file: str = "debug.log") -> None: |
There was a problem hiding this comment.
If this tool were to exist, it would be useful to identify multiple possible files. We have a tracebacks file, an API log, and a thread-exceptions log as it stands.
I wouldn't expect debug.log to necessarily have the error_patterns you mention, since anything can be 'print'ed into them, so having a default of debug.log with those patterns is surprising.
neiljp
added
PR awaiting update
What does this PR do, and why?
This PR adds comprehensive debugging tools to help both users and developers troubleshoot issues in Zulip Terminal:
New
zulip-term-debugcommand-line utility with multiple functions:log: Analyzes debug logs for common error patternsconnect: Tests connectivity to Zulip serversterminal: Checks terminal capabilities for compatibilityrun: Runs Zulip Terminal with debugging enabledImproved makefile targets for debugging:
make debug: Run with debug mode enabledmake debug-profile: Run with profiling enabledmake debug-clean: Clean debug filesEnhanced documentation in README with examples for:
As a frequent user of Zulip Terminal, I've sometimes found it challenging to diagnose issues. These tools standardize debugging approaches and make it easier for both users to report problems and developers to solve them.
I'm planning to apply for GSoC 2025 and would love to contribute more to Zulip Terminal in the future!
External discussion & connections
debugging improvementsHow did you test this?
Self-review checklist for each commit