Skip to content

[py] use enable_logging for Safari logging #2123

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

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Jan 6, 2025

User description

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

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

Description

Uses enable_logging for Safari service as a parameter with the latest version

Motivation and Context

Types of changes

  • Change to the site (I have double-checked the Netlify deployment, and my changes look good)
  • Code example added (and I also added the example to all translated languages)
  • Improved translation
  • Added new translation (and I also added a notice to each document missing translation)

Checklist

  • I have read the contributing document.
  • I have used hugo to render the site/docs locally and I am sure it works.

PR Type

Enhancement, Documentation, Tests


Description

  • Updated Safari WebDriver to use enable_logging parameter.

  • Added Python test for enable_logging in Safari service.

  • Updated documentation for Safari WebDriver across multiple languages.

  • Added version badge for enable_logging feature in documentation.


Changes walkthrough 📝

Relevant files
Tests
test_safari.py
Add test for Safari `enable_logging` parameter                     

examples/python/tests/browsers/test_safari.py

  • Replaced service_args with enable_logging in Safari service.
  • Added a test case for enable_logging parameter.
  • +1/-1     
    Documentation
    safari.en.md
    Update English documentation for Safari `enable_logging` 

    website_and_docs/content/documentation/webdriver/browsers/safari.en.md

  • Added version badge for enable_logging feature.
  • Linked Python example for enable_logging usage.
  • +1/-0     
    safari.ja.md
    Update Japanese documentation for Safari `enable_logging`

    website_and_docs/content/documentation/webdriver/browsers/safari.ja.md

  • Added version badge for enable_logging feature.
  • Linked Python example for enable_logging usage.
  • +1/-0     
    safari.pt-br.md
    Update Portuguese documentation for Safari `enable_logging`

    website_and_docs/content/documentation/webdriver/browsers/safari.pt-br.md

  • Added version badge for enable_logging feature.
  • Linked Python example for enable_logging usage.
  • +1/-0     
    safari.zh-cn.md
    Update Chinese documentation for Safari `enable_logging` 

    website_and_docs/content/documentation/webdriver/browsers/safari.zh-cn.md

  • Added version badge for enable_logging feature.
  • Linked Python example for enable_logging usage.
  • +1/-0     

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

    Copy link

    netlify bot commented Jan 6, 2025

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit 6cd2d0d

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 6, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Test Coverage

    The test case only verifies that the code runs without errors but doesn't validate if logging is actually enabled. Consider adding assertions to verify the logging functionality.

    def test_enable_logging():
        service = webdriver.SafariService(enable_logging=True)
    
        driver = webdriver.Safari(service=service)
    
        driver.quit()

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 6, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add verification steps to ensure the test actually validates the logging functionality

    Add assertions to verify that logging is actually enabled when setting
    enable_logging=True. Without assertions, the test only verifies that the code runs
    without errors but not the expected functionality.

    examples/python/tests/browsers/test_safari.py [16-21]

     def test_enable_logging():
         service = webdriver.SafariService(enable_logging=True)
         driver = webdriver.Safari(service=service)
    -    driver.quit()
    +    try:
    +        # Perform some action to generate logs
    +        driver.get("https://www.selenium.dev")
    +        # Verify logs are present
    +        assert driver.get_log('safari'), "No Safari logs were generated when logging was enabled"
    +    finally:
    +        driver.quit()
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion significantly improves test quality by adding actual verification of the logging functionality, transforming it from a smoke test to a meaningful functional test. The addition of try-finally block also ensures proper cleanup.

    9

    Copy link
    Member

    @harsha509 harsha509 left a comment

    Choose a reason for hiding this comment

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

    Thank you @Delta456 !

    @harsha509 harsha509 merged commit 8f04176 into SeleniumHQ:trunk Jan 7, 2025
    7 of 9 checks passed
    selenium-ci added a commit that referenced this pull request Jan 7, 2025
    @Delta456 Delta456 deleted the py_safari_log branch March 19, 2025 10: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.

    2 participants