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

[py] allow logging diagnose in safari driver #14606

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

Delta456
Copy link
Contributor

@Delta456 Delta456 commented Oct 16, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Allow enabling logs in safari driver through enable_logging parameter

Motivation and Context

Most of the time one needs to pass a --diagnose flag to enable debugging which is verbose and not Pythonic.

So service = webdriver.SafariService(enable_logging=True) will be a shorthand for service = webdriver.SafariService(service_args=["--diagnose"])

Just like how Java binding has .withLogging(true)

The comment for enable_logging also contains the location of log files generated by the safari driver.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement


Description

  • Introduced a new enable_logging parameter in the Safari WebDriver Service class to simplify enabling logging.
  • Automatically appends --diagnose to service_args when enable_logging is set to True, providing a more Pythonic way to enable logging.
  • Updated the class docstring to document the new enable_logging parameter and its effect.

Changes walkthrough 📝

Relevant files
Enhancement
service.py
Add logging capability to Safari WebDriver service             

py/selenium/webdriver/safari/service.py

  • Added enable_logging parameter to the Service class.
  • Appended --diagnose to service_args if enable_logging is True.
  • Updated docstring to include enable_logging parameter.
  • +5/-0     

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Documentation Update
    The docstring for the enable_logging parameter mentions the log file location, but this information should be verified for accuracy and completeness.

    Error Handling
    There's no error handling or validation for the enable_logging parameter. Consider adding a check to ensure it's a boolean value.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Validate the operating system when enabling logging for Safari driver

    Consider adding a check to ensure that the enable_logging parameter is only used on
    macOS, as Safari and safaridriver are only supported on this platform.

    py/selenium/webdriver/safari/service.py [34-44]

     def __init__(
         self,
         executable_path: str = None,
         port: int = 0,
         service_args: typing.Optional[typing.List[str]] = None,
         env: typing.Optional[typing.Mapping[str, str]] = None,
         reuse_service=False,
         enable_logging: bool = False,
         driver_path_env_key: str = None,
         **kwargs,
     ) -> None:
    +    import sys
    +    if enable_logging and sys.platform != 'darwin':
    +        raise ValueError("Safari logging is only supported on macOS")
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion is highly relevant as it ensures that the enable_logging feature is only used on macOS, which is the only platform supporting Safari and safaridriver. This prevents potential runtime errors on unsupported platforms.

    9
    Enhancement
    Provide user feedback when enabling a feature that generates log files

    Consider adding a warning or log message when logging is enabled, to inform the user
    about the location of the log files.

    py/selenium/webdriver/safari/service.py [48-49]

    +import warnings
    +
     if enable_logging:
         self.service_args.append("--diagnose")
    +    warnings.warn("Safari logging enabled. Logs can be found at ~/Library/Logs/com.apple.WebDriver/")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a warning message when logging is enabled informs users about the log file location, enhancing user experience and making the feature more transparent. This is a useful enhancement for user feedback.

    7
    Best practice
    Use a constant for command-line flags to improve code maintainability

    Consider using a constant for the --diagnose flag to improve maintainability and
    reduce the risk of typos.

    py/selenium/webdriver/safari/service.py [48-49]

    +DIAGNOSE_FLAG = "--diagnose"
    +
     if enable_logging:
    -    self.service_args.append("--diagnose")
    +    self.service_args.append(DIAGNOSE_FLAG)
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a constant for the --diagnose flag enhances maintainability by reducing the risk of typos and making the code easier to update if the flag changes. This is a good practice for improving code quality.

    6

    💡 Need additional feedback ? start a PR chat

    @harsha509 harsha509 merged commit e9fb68b into SeleniumHQ:trunk Oct 17, 2024
    12 of 13 checks passed
    @Delta456 Delta456 deleted the safari_logging branch October 17, 2024 15:54
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants