[py] add safari service tests

[Delta456]

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

Add safari service tests

Motivation and Context

Add because they were missing

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

Tests

---

Description

• Added new test cases for the Safari service to ensure proper functionality.
• Implemented setup and teardown for environment variables in the test suite.
• Verified the service path is correctly set and updated based on environment variables.
• Tested the service's logging capabilities and service URL configuration.

---

Changes walkthrough 📝

	Relevant files
Documentation	__init__.py Add license header to Safari test init file                            py/test/selenium/webdriver/safari/init.py Added license information and file header. +16/-0
Tests	safari_service_tests.py Add Safari service test cases                                                        py/test/selenium/webdriver/safari/safari_service_tests.py Added new test cases for Safari service. Implemented environment variable setup and teardown. Tested service path and logging functionality. +54/-0

---

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

[codiumai-pr-agent-pro]

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 Hardcoded Path The test uses a hardcoded service path '/path/to/safaridriver'. This might not be portable across different environments. Incomplete Teardown The teardown only removes the 'SE_SAFARIDRIVER' environment variable. It should also reset any other changes made during the test. Incomplete Test The test_updates_path_after_setting_env_variable method doesn't actually verify that the path was updated to the new value.

[codiumai-pr-agent-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Best practice	Use a context manager for environment variable management in tests to ensure proper cleanup Consider using a context manager (with statement) for environment variable setup and teardown to ensure proper cleanup, even if an exception occurs during the test. py/test/selenium/webdriver/safari/safari_service_tests.py [31-35] @pytest.fixture(autouse=True) def setup_and_teardown(self): - os.environ["SE_SAFARIDRIVER"] = self.service_path - yield - os.environ.pop("SE_SAFARIDRIVER", None) + with pytest.MonkeyPatch.context() as mp: + mp.setenv("SE_SAFARIDRIVER", self.service_path) + yield Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion improves the reliability of the test by ensuring that environment variables are properly cleaned up, even if an exception occurs, which is a best practice in test setup and teardown.	8
Enhancement	Use platform-independent path construction for better cross-platform compatibility Instead of using a hardcoded path, consider using a more flexible approach like pathlib.Path or os.path.join to construct the service path, ensuring cross-platform compatibility. py/test/selenium/webdriver/safari/safari_service_tests.py [29] -service_path = "/path/to/safaridriver" +from pathlib import Path +service_path = str(Path("/path/to/safaridriver")) Apply this suggestion Suggestion importance[1-10]: 7 Why: Using pathlib.Path for path construction enhances cross-platform compatibility, making the code more robust and adaptable to different operating systems.	7
	Parameterize the logging test to cover both enabled and disabled scenarios Consider parameterizing the test_enable_logging function to test both True and False cases for the enable_logging parameter, ensuring comprehensive coverage of the feature. py/test/selenium/webdriver/safari/safari_service_tests.py [47-50] -def test_enable_logging(): - enable_logging = True +@pytest.mark.parametrize("enable_logging,expected", [(True, True), (False, False)]) +def test_enable_logging(enable_logging, expected): service = Service(enable_logging=enable_logging) - assert '--diagnose' in service.service_args + assert ('--diagnose' in service.service_args) == expected Apply this suggestion Suggestion importance[1-10]: 7 Why: Parameterizing the test increases coverage by testing both scenarios of the enable_logging feature, ensuring comprehensive validation of the functionality.	7
	Strengthen the test assertion to verify the exact updated path is used Consider adding an assertion to check if the new path is actually used after updating the environment variable, as the current test might not fully verify the behavior. py/test/selenium/webdriver/safari/safari_service_tests.py [40-45] def test_updates_path_after_setting_env_variable(self, service): new_path = "/foo/bar" os.environ["SE_SAFARIDRIVER"] = new_path service.executable_path = self.service_path # Simulating the update - assert "safaridriver" in service.executable_path + assert new_path in service.executable_path Apply this suggestion Suggestion importance[1-10]: 6 Why: The suggestion improves the test by ensuring that the updated path is correctly used, which enhances the accuracy and reliability of the test case.	6

💡 Need additional feedback ? start a PR chat

[VietND96]

I guess the tests are not running in CI

[Delta456]

I guess the tests are not running in CI

Yes, looks like I need to edit the CI to add it to CI / Python / Safari Tests / Integration Tests (safari) (pull_request) ?

[VietND96]

Let me add full tests of Safari to CI in trunk first. Last few days I added but there were 9 test failures, so I decided to add the launcher tests only.
Probably I mark skip for those failure and we will get back to fix later.

[Delta456]

This test passes locally. We can add this test in the CI and get back to the rest later.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.21%. Comparing base (57f8398) to head (350587e).
Report is 880 commits behind head on trunk.

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #14700      +/-   ##
==========================================
+ Coverage   58.48%   59.21%   +0.72%     
==========================================
  Files          86       91       +5     
  Lines        5270     5852     +582     
  Branches      220      260      +40     
==========================================
+ Hits         3082     3465     +383     
- Misses       1968     2127     +159     
- Partials      220      260      +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[VietND96]

While patching 4.26.1 for python, we missed catching a broken for safari webdriver #14699 due to no safari tests in CI.
I also checked other bindings java, rb, there is runner: macOS used there. However, there is no browser: safari is enabled.
So, via this PR, we want to enable safari-test for python. There are few failed tests marked as xfail_safari (some of tests are being marked as fail in other browser, remote)
@AutomatedTester, can you review this to see if it is fine?