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

Reset WebDriver and vncRecordingContainer when Selenium container stops #5116

Merged
merged 2 commits into from
May 18, 2022

Conversation

tobiasstadler
Copy link
Contributor

@tobiasstadler tobiasstadler commented Feb 22, 2022

Some of our tests look similar to:

void BaseTestClass {

    @Container
    private static final BrowserWebDriverContainer<?> container = new BrowserWebDriverContainer<>("...");

    ...
}
void MyTest1 extends BaseTestClass {

    @Test
    void test1() {
        ...
    }

    ...
}
void MyTest2 extends BaseTestClass {

    @Test
    void test2() {
        ...
    }

    ...
}

The selenium container is started before any test of one class are run and stopped after all test of one class are run. Currently driver is created only once in BrowserWebDriverContainer.java and when the container stops driver.quit() is called. So the second and any subsequent test class will see a "stopped" driver which will fail the tests of that class.

Right now we recreate/start/stop the selenium container manually in @BeforeAll/@AfterAll block, but it would be nice if we could reuse the container definition.

@tobiasstadler tobiasstadler force-pushed the selenium-reset-driver branch 2 times, most recently from e42b56c to c12e889 Compare February 23, 2022 14:20
@tobiasstadler
Copy link
Contributor Author

@kiview Would you please have a look?

@kiview
Copy link
Member

kiview commented Mar 4, 2022

Hey @tobiasstadler, sure thing, but can you please update the description of the PR, so that we have the context? Like, which use cases does this unblock?

@tobiasstadler
Copy link
Contributor Author

@kiview I updated the description

@tobiasstadler
Copy link
Contributor Author

ping

@kiview
Copy link
Member

kiview commented Mar 17, 2022

I am not sure I understand your example correctly. In case of using a JUnit-Rule or the Jupiter-Extension on a per-test-method basis, each test would instantiate a new BrowserWebDriverContainer object.

@tobiasstadler
Copy link
Contributor Author

@kiview You are right, I described the wrong case. Sorry! I changed the description to actually describe or problem.

@tobiasstadler
Copy link
Contributor Author

ping

@tobiasstadler
Copy link
Contributor Author

ping

@bsideup bsideup added this to the next milestone May 17, 2022
@bsideup
Copy link
Member

bsideup commented May 17, 2022

Hey @tobiasstadler! Thanks for spotting and fixing it. Do you think you can provide a simple test that will verify that this is no longer an issue? (so that we won't introduce a regression in the future)

@kiview kiview changed the title [Selenium] Reset the WebDriver when the Selenium container stops Reset the WebDriver when the Selenium container stops May 18, 2022
@kiview kiview changed the title Reset the WebDriver when the Selenium container stops Reset WebDriver and vncRecordingContainer when Selenium container stops May 18, 2022
@kiview kiview merged commit 6dc05d3 into testcontainers:master May 18, 2022
@kiview
Copy link
Member

kiview commented May 18, 2022

Thanks a lot for the contribution @tobiasstadler, we will take care of the test ourselves 🙂

@tobiasstadler
Copy link
Contributor Author

Thank You!

@tobiasstadler tobiasstadler deleted the selenium-reset-driver branch May 19, 2022 17:22
eddumelendez added a commit that referenced this pull request May 19, 2022
eddumelendez added a commit that referenced this pull request May 19, 2022
eddumelendez added a commit that referenced this pull request May 25, 2022
eddumelendez added a commit that referenced this pull request May 26, 2022
eddumelendez added a commit that referenced this pull request May 26, 2022
eddumelendez added a commit that referenced this pull request May 26, 2022
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.

None yet

3 participants