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

Add safari driver #1220

Merged
merged 41 commits into from
Jan 9, 2024
Merged

Add safari driver #1220

merged 41 commits into from
Jan 9, 2024

Conversation

Andmedoctopus
Copy link
Contributor

No description provided.

@Andmedoctopus
Copy link
Contributor Author

@fsouza @jsfehler could you take a look, please?

@fsouza
Copy link
Contributor

fsouza commented Dec 13, 2023

@fsouza @jsfehler could you take a look, please?

Is there any way to write tests for this? Maybe we make it run only on macOS in GHA.

@Andmedoctopus
Copy link
Contributor Author

@fsouza @jsfehler could you take a look, please?

Is there any way to write tests for this? Maybe we make it run only on macOS in GHA.

I added MacOS to CI. Seems Splinter has some bug with Safari. I will fix them and let you know when its done

@Andmedoctopus
Copy link
Contributor Author

@fsouza Please take a look. CI almost stable

@Andmedoctopus
Copy link
Contributor Author

@fsouza CI is green, please take a look after holidays ☃️ 🙂

@@ -20,6 +20,7 @@ jobs:
runs-on: ubuntu-latest

strategy:
max-parallel: 3
Copy link
Contributor

Choose a reason for hiding this comment

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

curious on why we're making this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI was unstable. Removed all limitation. Works ok



class ClickElementsTest:
def test_click_links(self):
self.browser.links.find_by_text("FOO").click()
self.assertIn("BAR!", self.browser.html)
self.browser.visit(EXAMPLE_APP)
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we need to make this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The visit in setUp was unstable for some reason and page just stuck. Now seems ok. Removed all visit in tests

Comment on lines 84 to 86
def test_should_be_able_to_change_user_agent(self):
"Remote should not support custom user agent"
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

can we explicitly skip it with a message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@Andmedoctopus
Copy link
Contributor Author

@fsouza I have fixed all remarks. Could you take a look, please?

@Andmedoctopus
Copy link
Contributor Author

@fsouza could you take a look to the PR, please?

Copy link
Contributor

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

@Andmedoctopus thanks for the ping, sorry I somehow missed the earlier notification. This looks good, thanks for contributing!

@fsouza fsouza merged commit ac7263b into cobrateam:master Jan 9, 2024
26 checks passed
@Andmedoctopus
Copy link
Contributor Author

@fsouza thank you! Could you release a new version of splinter, please?

@Andmedoctopus
Copy link
Contributor Author

@fsouza thank you! Could you release a new version of splinter, please?

@fsouza, could you make a new release, please?

@fsouza
Copy link
Contributor

fsouza commented Jan 16, 2024

Will tag a new release once #1229 passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants