Skip to content

Fix exception traceback to omit superfluous 'Backtrace' information #14975

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Fix exception traceback to omit superfluous 'Backtrace' information #14975

wants to merge 3 commits into from

Conversation

@SmartManoj
Copy link

@SmartManoj SmartManoj commented Dec 29, 2024 •
edited by qodo-merge-pro bot
Loading

User description

Fixes #9977

Filter out superfluous 'StackTrace' information from Selenium 4 exception tracebacks.

  • Add a check for an environment variable to conditionally include the stacktrace.

For more details, open the Copilot Workspace session.

PR Type

Bug fix

Description

  • Addresses issue Selenium 4 exceptions produce a Traceback that contains this superfluous 'Backtrace' info.  #9977 where Selenium 4 exceptions contain superfluous 'Backtrace' information
  • Added environment variable INCLUDE_STACKTRACE to control stacktrace visibility in exceptions
  • Implemented stacktrace filtering to remove unnecessary 'Backtrace' information
  • Modified exception handling to make stack traces more concise and readable
  • Changes maintain backward compatibility through environment variable toggle

Changes walkthrough 📝

Relevant files
Enhancement
exceptions.py
Add environment variable control for stacktrace display   

py/selenium/common/exceptions.py

  • Added import of os module
  • Modified __str__ method to only include stacktrace when
    INCLUDE_STACKTRACE environment variable is set
    		• +2/-1     
    Bug fix
    errorhandler.py
    Implement stacktrace filtering functionality                         

    py/selenium/webdriver/remote/errorhandler.py

  • Added import of os module
  • Added new filter_stacktrace method to remove 'Backtrace' information
  • Modified check_response to filter stacktrace when INCLUDE_STACKTRACE
    is not set
    		• +9/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @SmartManoj
    Fix exception traceback to omit superfluous 'Backtrace' information
    f859be1 
    Fixes SeleniumHQ#9977

Filter out superfluous 'StackTrace' information from Selenium 4 exception tracebacks.

* Add a check for an environment variable to conditionally include the stacktrace.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/SeleniumHQ/selenium/issues/9977?shareId=XXXX-XXXX-XXXX-XXXX).
    @CLAassistant
    Copy link

    CLAassistant commented Dec 29, 2024 •
    edited
    Loading

    CLA assistant check
    All committers have signed the CLA.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 29, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    9977 - Fully compliant

    Compliant requirements:

    • Remove superfluous 'Backtrace' information from Selenium 4 exception tracebacks
    • Keep traditional call stack information for debugging
    • Maintain backward compatibility through INCLUDE_STACKTRACE environment variable
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Environment Variable Check

    The environment variable check is done on every exception string formatting. Consider caching the environment variable value to avoid repeated lookups.

    if self.stacktrace and os.getenv("INCLUDE_STACKTRACE"):

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 29, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Use a constant instead of a magic string to improve code maintainability

    Consider using a constant for the 'Backtrace' string to avoid magic strings and make
    maintenance easier.

    py/selenium/webdriver/remote/errorhandler.py [148]

    -return [line for line in stacktrace if 'Backtrace' not in line]
+BACKTRACE_IDENTIFIER = 'Backtrace'
+return [line for line in stacktrace if BACKTRACE_IDENTIFIER not in line]
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: The suggestion improves code maintainability by eliminating a magic string, making the code more maintainable and reducing the risk of typos, though the impact is relatively minor.

    		4
    Cache environment variable lookup result to improve performance by avoiding repeated system calls

    Cache the environment variable check result to avoid repeated lookups of
    INCLUDE_STACKTRACE in the str method.

    py/selenium/common/exceptions.py [42-44]

    -if self.stacktrace and os.getenv("INCLUDE_STACKTRACE"):
+include_stacktrace = os.getenv("INCLUDE_STACKTRACE")
+if self.stacktrace and include_stacktrace:
     stacktrace = "\n".join(self.stacktrace)
     exception_msg += f"Stacktrace:\n{stacktrace}"
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While the suggestion offers a minor performance optimization by caching the environment variable lookup, the impact is minimal since str is not typically called in performance-critical paths.

    		3

    @diemol
    Copy link
    Member

    diemol commented Dec 29, 2024

    Thanks for rasing the PR, however we commented the code was going to stay as it was, is there any other motivation for this?

    @SmartManoj
    Copy link
    Author

    SmartManoj commented Dec 30, 2024

    Those stack traces are not useful for SLMs.

    SmartManoj
    SmartManoj commented Dec 30, 2024
    View reviewed changes
    py/selenium/common/exceptions.py
    if self.screen:
    exception_msg += "Screenshot: available via screen\n"
    if self.stacktrace:
    if self.stacktrace and os.getenv("INCLUDE_STACKTRACE"):
    Copy link
    Author

    @SmartManoj SmartManoj Dec 30, 2024 •
    edited
    Loading

    Choose a reason for hiding this comment

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

    Suggested change
    if self.stacktrace and os.getenv("INCLUDE_STACKTRACE"):
    if self.stacktrace and not os.getenv("EXCLUDE_STACKTRACE"):

    Will this change be accepted?

    @diemol
    Copy link
    Member

    diemol commented Dec 30, 2024

    You should be able to exclude those or train the SLM to ignore them. We don't want to change the code as we pass along the information the browser sends back. You should engage more with the Chrome folks in the open issue in their bug tracker.

    @diemol diemol closed this Dec 30, 2024
    @SmartManoj
    Copy link
    Author

    SmartManoj commented Dec 30, 2024 •
    edited
    Loading

    When will these stack traces be useful?
    Until the Chrome folks fix that, could you add this as a temporary fix?

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    Selenium 4 exceptions produce a Traceback that contains this superfluous 'Backtrace' info.

    3 participants

    @SmartManoj @CLAassistant @diemol