FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: Add selenium video support #6 by JosefShenhav · Pull Request #364 · testcontainers/testcontainers-python · GitHub

/ testcontainers-python Public

fix: Add selenium video support #6 - #364

Merged
alexanderankin merged 3 commits into
testcontainers:mainfrom
JosefShenhav:selenium-with-video
May 14, 2024
ConversationCommits (3)ChecksFiles changed
Merged

fix: Add selenium video support #6#364
alexanderankin merged 3 commits into
testcontainers:mainfrom
JosefShenhav:selenium-with-video

Conversation

JosefShenhav commented Jun 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Support video in selenium testcontainer.

Changes made:

  • Added network to default container.
  • Added video to selenium by write with_video.

JosefShenhav force-pushed the selenium-with-video branch 4 times, most recently from 65964f3 to 49cb763 Compare October 4, 2023 20:30
JosefShenhav force-pushed the selenium-with-video branch 3 times, most recently from a44a8ec to 79698c3 Compare January 26, 2024 07:29
alexanderankin approved these changes Mar 9, 2024
View reviewed changes

alexanderankin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm for 4.1 or similar

Comment thread selenium/testcontainers/selenium/__init__.py Outdated
Comment on lines 28 to 30
"firefox": "selenium/standalone-firefox:4.13.0-20231004",
"chrome": "selenium/standalone-chrome:4.13.0-20231004"
}

alexanderankin Mar 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

probably dont want to change the images, is this necessary? or what is the motivation

JosefShenhav Apr 19, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I will fix this - thanks

JosefShenhav May 10, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I updated the image because it's outdated and no longer supported.

alexanderankin added ✨ feat 🛠️ needs more work Need to invest more time, can be a rebase or code updates labels Mar 9, 2024
alexanderankin added the 👥 community feat feature but its a community module so we wont bump tc core for it label Mar 26, 2024
JosefShenhav commented Apr 22, 2024
View reviewed changes
Comment thread modules/selenium/testcontainers/selenium/__init__.py

def stop(self, force=True, delete_volume=True) -> None:
if self.video:
# get_wrapped_container().stop -> stop the container

JosefShenhav Apr 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe we should renamed video.stop to video.remove?

JosefShenhav commented Apr 22, 2024
View reviewed changes
Comment thread modules/selenium/testcontainers/selenium/__init__.py Outdated
super().start()

self.video \
.with_kwargs(network=network.name) \

JosefShenhav Apr 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I encountered an issue and would appreciate it if you could review it:
using with_network (network.connect), I aim to block the container's access from the host via a port.
Providing the network in the container run command should resolve this issue.

Note:
Using with_network, along with adding a delay before connecting to the network and after running the container, resolved the issue.

JosefShenhav force-pushed the selenium-with-video branch 2 times, most recently from 278c2bc to 79727ea Compare April 22, 2024 10:25

JosefShenhav commented Apr 22, 2024

Copy link
Copy Markdown
Contributor Author

@alexanderankin

JosefShenhav force-pushed the selenium-with-video branch 2 times, most recently from 4b18e98 to fc58c7c Compare April 22, 2024 10:53
JosefShenhav changed the title Selenium with video feat: Add slenium video support #6 Apr 22, 2024
alexanderankin changed the title feat: Add slenium video support #6 fix: Add slenium video support #6 Apr 22, 2024
alexanderankin changed the title fix: Add slenium video support #6 fix: Add selenium video support #6 Apr 22, 2024
JosefShenhav force-pushed the selenium-with-video branch from 1a8f372 to 6f40428 Compare May 10, 2024 15:22

JosefShenhav commented May 10, 2024

Copy link
Copy Markdown
Contributor Author

@alexanderankin I've just finished rebasing the branch onto main.

alexanderankin commented May 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

looks like tests are not passing. you can run the tests locally by cd into the directory, poetry install -E selenium and then poetry run pytest modules/selenium or poetry shell to activate the environment and run pytest commands as normal there.

JosefShenhav commented May 10, 2024

Copy link
Copy Markdown
Contributor Author

I would try soon, thanks :)

JosefShenhav commented May 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@alexanderankin The problem stemmed from permissions. The tempfile.TemporaryDirectory function generated a folder lacking the necessary permissions for writing within the container. Now it is working!

alexanderankin merged commit 3c8006c into testcontainers:main May 14, 2024
github-actions Bot mentioned this pull request May 14, 2024
Merged
alexanderankin removed their request for review May 14, 2024 07:23
alexanderankin pushed a commit that referenced this pull request May 14, 2024
chore(main): release testcontainers 4.4.1 (#551)
49b261e 
🤖 I have created a release *beep* *boop*
---


##
[4.4.1](testcontainers-v4.4.0...testcontainers-v4.4.1)
(2024-05-14)


### Bug Fixes

* Add memcached container
([#322](#322))
([690b9b4](690b9b4))
* Add selenium video support
[#6](#6)
([#364](#364))
([3c8006c](3c8006c))
* **core:** add empty _configure to DockerContainer
([#556](#556))
([08916c8](08916c8))
* **core:** remove version from compose tests
([#571](#571))
([38946d4](38946d4))
* **keycloak:** add realm imports
([#565](#565))
([f761b98](f761b98))
* **mysql:** Add seed support in MySQL
([#552](#552))
([396079a](396079a))
* url quote passwords
([#549](#549))
([6c5d227](6c5d227))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

alexanderankin alexanderankin approved these changes

Assignees

No one assigned

Labels

👥 community feat feature but its a community module so we wont bump tc core for it ✨ feat 🛠️ needs more work Need to invest more time, can be a rebase or code updates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL