Open Bug 1470120 Opened 2 years ago Updated 2 years ago

Explore test mode for not restarting the browser on session deletion/creation

Categories

(Testing :: geckodriver, enhancement, P3)

enhancement

Tracking

(Not tracked)

People

(Reporter: ato, Unassigned)

References

Details

In https://bugzilla.mozilla.org/show_bug.cgi?id=1264259#c12 I
proposed a workaround to not make geckodriver restart Firefox on
session creation and deletion.  Because starting Firefox is expensive,
we might be able to have a test mode capability that will prevent
the process from being restarted in tests that need to make a lot
of new session requests (such as the WPT wdspec user prompt handler
tests).

The test mode would effectively make geckodriver behave like the
Marionette Python client, where start_session() end_session() do
not explicitly start or stop the Firefox process.

I’m not sure this is entirely kosher with regards to conforming to
the WebDriver specification tests, but it might be a worthwhile as
an experiment.  For example there might be other showstoppers I
haven’t thought of.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P3
I wonder how wide we want to expose such a feature outside of Geckodriver. I mean there is nothing in the spec which clarifies the behavior of `Delete Session` and how it has to handle the browser process. It only says that the remote end has to be closed.

Further for some tests we might need preferences which have to be set before Firefox starts. So how can a test select that `Delete Session` closes the browser process or not? Maybe for geckodriver we could have an optional argument, but then I wonder how we aren't getting in conflict with other drivers in wdclient.
(In reply to Henrik Skupin (:whimboo) from comment #1)

> I wonder how wide we want to expose such a feature outside of
> Geckodriver. I mean there is nothing in the spec which clarifies
> the behavior of `Delete Session` and how it has to handle the
> browser process. It only says that the remote end has to be
> closed.

You’re right.  In fact, if geckodriver connects to an already
existing Firefox process we would not expect it to close down the
process because the lifetime of the browser process is managed by
the user manually.

However, when geckodriver does manage the process the expectancy
is for it to shut down the process which is why I’m proposing a
non-standard extension capability.  It could also be a flag, but I
don’t want to spread configuration out too much.

> Further for some tests we might need preferences which have to be
> set before Firefox starts. So how can a test select that `Delete
> Session` closes the browser process or not? Maybe for geckodriver
> we could have an optional argument, but then I wonder how we
> aren't getting in conflict with other drivers in wdclient.

WPT tests don’t change any preferences, so we can rely on those that
are set in the original profile when Firefox starts.
Do you know how other drivers manage the session? Do they also restart the browser? If yes everyone would be affected by longer test duration due to application restarts.

Another idea which I would like to discuss with you is if we could think of re-using an existent session with the `new_session` fixture if the required capabilities exactly match those from the current session. In such a case it doesn't make sense to restart the browser. This would help me a lot for the refactoring of user prompt tests, where we have 3x the code size in each and every sub test due to code duplication. But I cannot use `pytest.mark.parametrized` because it would create new sessions with the exact capabilities for each parameter.

I'm fine in discussing this with you on Monday.
(In reply to Henrik Skupin (:whimboo) from comment #3)

> Do you know how other drivers manage the session? Do they also
> restart the browser? If yes everyone would be affected by longer
> test duration due to application restarts.

Sure, but Firefox is abmyssmally slow.  Having some way to make
the tests faster to run—at least locally—seems to be a general win
if browser restarting isn’t vital (for implementation specific
reasons).

> Another idea which I would like to discuss with you is if we could
> think of re-using an existent session with the `new_session`
> fixture if the required capabilities exactly match those from
> the current session. In such a case it doesn't make sense to
> restart the browser. This would help me a lot for the refactoring
> of user prompt tests, where we have 3x the code size in each
> and every sub test due to code duplication. But I cannot use
> `pytest.mark.parametrized` because it would create new sessions
> with the exact capabilities for each parameter.

I like this idea.  And it’s something that could be implemented
upstream and benefit all implementations.

I don’t see a technical reason why we can’t do that.
Status: ASSIGNED → NEW
Assignee: ato → nobody
David and Dave, before I will invest more time on this bug I think I miss a bit of background information in regards of Selenium. Maybe you can help me here, or point to the appropriate sources.

Lets say we have the following test in Selenium:

> driver = webdriver.Firefox(options=opts, capabilities=capabilities)
> driver.get("https://mozilla.org")
> driver.quit()

Here a new browser instance is created and started. Then a navigation takes place, and finally the session is cleaned-up including the shutdown of the Firefox process. What's unclear to me is `quit()`. Here Selenium wraps deleting the session, AND shutting down Firefox into a single command. Which I think is fine. But is there also a way to just delete the session, without shutting down Firefox? I read through various documentations but wasn't able to find anything related to this question. So currently I assume it is not supported, at least not in the Python bindings.

If it really doesn't exist, are there reasons for?
Flags: needinfo?(dburns)
Flags: needinfo?(dave.hunt)
Quit is designed to clean up everything. The reason it closes the browser and does all of that is because people don't do that. There is `close()` to close the window but that doesnt do anything to the session.

Since the driver also needs to start up the likes of geckodriver and clean that up it would be a major footgun to not do that cleanup for users.
Flags: needinfo?(dburns)
Flags: needinfo?(dave.hunt)
(In reply to David Burns :automatedtester from comment #6)
> Since the driver also needs to start up the likes of geckodriver and clean
> that up it would be a major footgun to not do that cleanup for users.

Sure, and I understand that. But what about an option to end and re-start a session without closing the browser?
Flags: needinfo?(dburns)
It won't be added to Selenium, it's too much of a footgun.

If you want to add it to wdspec then we can explore it but it's not something that I want to be widely shared.

What would this gain vs just scaling horizontally in taskcluster
Flags: needinfo?(dburns)
You need to log in before you can comment on or make changes to this bug.