Skip to content

fix: Add selenium video support #6 - #364

Merged
alexanderankin merged 3 commits into
testcontainers:mainfrom
JosefShenhav:selenium-with-video
May 14, 2024
Merged

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

Conversation

@JosefShenhav

@JosefShenhavJosefShenhav 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
JosefShenhavforce-pushed the selenium-with-video branch 4 times, most recently from 65964f3 to 49cb763CompareOctober 4, 2023 20:30
@JosefShenhav
JosefShenhavforce-pushed the selenium-with-video branch 3 times, most recently from a44a8ec to 79698c3CompareJanuary 26, 2024 07:29
alexanderankin
alexanderankin approved these changes Mar 9, 2024
View reviewed changes

@alexanderankinalexanderankin 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.

lgtm for 4.1 or similar

Comment threadselenium/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"
}

@alexanderankinalexanderankinMar 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.

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

@JosefShenhavJosefShenhavApr 19, 2024

Copy link
Copy Markdown
ContributorAuthor

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

@JosefShenhavJosefShenhavMay 10, 2024

Copy link
Copy Markdown
ContributorAuthor

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.

@alexanderankinalexanderankin added ✨ feat 🛠️ needs more work Need to invest more time, can be a rebase or code updates labels Mar 9, 2024
@alexanderankinalexanderankin 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
Comment threadmodules/selenium/testcontainers/selenium/__init__.py

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

@JosefShenhavJosefShenhavApr 22, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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
Comment threadmodules/selenium/testcontainers/selenium/__init__.py
JosefShenhav
JosefShenhav commented Apr 22, 2024
View reviewed changes
Comment threadmodules/selenium/testcontainers/selenium/__init__.py Outdated
super().start()

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

@JosefShenhavJosefShenhavApr 22, 2024

Copy link
Copy Markdown
ContributorAuthor

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
JosefShenhavforce-pushed the selenium-with-video branch 2 times, most recently from 278c2bc to 79727eaCompareApril 22, 2024 10:25
@JosefShenhav

JosefShenhav commented Apr 22, 2024

Copy link
Copy Markdown
ContributorAuthor

@alexanderankin

@JosefShenhav
JosefShenhavforce-pushed the selenium-with-video branch 2 times, most recently from 4b18e98 to fc58c7cCompareApril 22, 2024 10:53
@JosefShenhavJosefShenhav changed the title Selenium with videofeat: Add slenium video support #6Apr 22, 2024
@alexanderankinalexanderankin changed the title feat: Add slenium video support #6fix: Add slenium video support #6Apr 22, 2024
@alexanderankinalexanderankin changed the title fix: Add slenium video support #6fix: Add selenium video support #6Apr 22, 2024
@JosefShenhav

JosefShenhav commented May 10, 2024

Copy link
Copy Markdown
ContributorAuthor

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

@alexanderankin

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

JosefShenhav commented May 10, 2024

Copy link
Copy Markdown
ContributorAuthor

I would try soon, thanks :)

@JosefShenhav

JosefShenhav commented May 14, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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:mainMay 14, 2024
@github-actionsgithub-actionsBot 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexanderankinalexanderankinalexanderankin approved these changes

Assignees

No one assigned

Labels

👥 community featfeature but its a community module so we wont bump tc core for it✨ feat🛠️ needs more workNeed 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

@JosefShenhav@alexanderankin