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 tests to run for sameSite cookies in firefox and safari #14893

Closed

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Dec 12, 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

sameSite cookies have been implemented in firefox as per - https://hacks.mozilla.org/2020/08/changes-to-samesite-cookie-behavior/ and the tests pass locally.

Removed the xfail for firefox, safari, ie and remote.

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

  • Removed xfail markers for sameSite cookie tests in Firefox, Safari, IE, and remote, allowing these tests to run.
  • Enabled testing for sameSite cookie attributes: Strict, Lax, and None, ensuring compatibility with recent browser updates.

Changes walkthrough 📝

Relevant files
Tests
cookie_tests.py
Enable `sameSite` cookie tests for Firefox and Safari       

py/test/selenium/webdriver/common/cookie_tests.py

  • Removed xfail markers for Firefox, Safari, IE, and remote for sameSite
    cookie tests.
  • Enabled tests for sameSite cookie attributes: Strict, Lax, and None.
  • +0/-11   

    💡 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 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Coverage
    Verify that removing xfail markers for Firefox, Safari and IE is justified by testing on all these browsers to ensure sameSite cookie functionality works as expected

    Edge Case
    The test for same_site_none cookies doesn't actually verify the sameSite attribute value, unlike the other tests. Consider adding assertion after handling the secure cookie case

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add missing assertion to validate expected behavior when setting SameSite=None cookies on insecure sites

    Add an assertion to verify that the cookie was not set when using SameSite=None, as
    the test currently lacks verification logic.

    py/test/selenium/webdriver/common/cookie_tests.py [95-98]

     def test_add_cookie_same_site_none(same_site_cookie_none, driver):
         driver.add_cookie(same_site_cookie_none)
         # Note that insecure sites (http:) can't set cookies with the Secure directive.
    -    # driver.get_cookie would return None
    +    returned = driver.get_cookie("foo")
    +    assert returned is None, "Cookie with SameSite=None should not be set on insecure sites"
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion adds crucial verification logic to test_add_cookie_same_site_none by asserting that cookies with SameSite=None are not set on insecure sites, transforming a passive comment into an actual test assertion.

    9

    @navin772
    Copy link
    Contributor Author

    Closing as duplicate of #14794

    @navin772 navin772 closed this Dec 12, 2024
    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.

    1 participant