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.

Sign up for GitHub

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

Jump to bottom

fix: Add selenium video support #6 #364

Merged
merged 3 commits into from
May 14, 2024
Merged

fix: Add selenium video support #6 #364

merged 3 commits into from
May 14, 2024

Conversation

JosefShenhav
Copy link
Contributor

@JosefShenhav JosefShenhav commented Jun 16, 2023 •
edited
Loading

Support video in selenium testcontainer.

Changes made:

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

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

@alexanderankin alexanderankin left a comment

Choose a reason for hiding this comment

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

lgtm for 4.1 or similar

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"
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix this - thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@alexanderankin alexanderankin added the community-feat feature but its a community module so we wont bump tc core for it label Mar 26, 2024
JosefShenhav
JosefShenhav commented Apr 22, 2024
View reviewed changes
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
Copy link
Contributor Author

@JosefShenhav JosefShenhav Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

self.video \
.with_kwargs(network=network.name) \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 JosefShenhav force-pushed the selenium-with-video branch 2 times, most recently from 278c2bc to 79727ea Compare April 22, 2024 10:25
@JosefShenhav
Copy link
Contributor Author

@alexanderankin

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

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

@alexanderankin
Copy link
Member

alexanderankin commented May 10, 2024 •
edited
Loading

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
Copy link
Contributor Author

I would try soon, thanks :)

@JosefShenhav
Copy link
Contributor Author

JosefShenhav commented May 14, 2024 •
edited
Loading

@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!

image

@alexanderankin alexanderankin merged commit 3c8006c into testcontainers:main May 14, 2024
20 checks passed
@github-actions github-actions bot mentioned this pull request May 14, 2024
Merged
@alexanderankin alexanderankin removed their request for review May 14, 2024 07:23
alexanderankin pushed a commit that referenced this pull request May 14, 2024
@github-actions
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderankin 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-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JosefShenhav @alexanderankin