Closed Bug 1746216 Opened 2 years ago Closed 2 years ago

Implement "session.status" command

Categories

(Remote Protocol :: WebDriver BiDi, task, P2)

task
Points:
2

Tracking

(firefox97 fixed)

RESOLVED FIXED
97 Branch
Tracking Status
firefox97 --- fixed

People

(Reporter: whimboo, Assigned: jdescottes)

References

()

Details

(Whiteboard: [bidi-m2-mvp])

Attachments

(2 files)

No description provided.
Blocks: 1731576

I imagine that similarly to session.new, this should not be implemented in the root/session.jsm module, because it's a static command which is not associated to any session? So it should be added around https://searchfox.org/mozilla-central/rev/dc323d0d9a3b722ca8ff0d1b2b752e5bd00dab9b/remote/webdriver-bidi/WebDriverBiDiConnection.jsm#166

Correct. But I also wonder if this could be static method on the session module so that we do not pollute the code in WebDriverBiDiConnection. We might also want to move session.new if that makes sense.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #2)

Correct. But I also wonder if this could be static method on the session module so that we do not pollute the code in WebDriverBiDiConnection. We might also want to move session.new if that makes sense.

I think it would make sense to implement it in WebDriverBiDi.jsm. This singleton is already responsible for keeping track of already created sessions, which is important to implement session.status.

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

As I see in https://searchfox.org/mozilla-central/source/testing/geckodriver/src/marionette.rs#193-209 we do not implement the status command for WebDriver HTTP in Marionette itself, which might actually still be a good idea so that marionette clients could benefit from. Then we could also share this implementation.

James, what do you think?

Flags: needinfo?(james)

That only really makes sense if we're using --connect-existing with geckodriver, since otherwise there wouldn't be a Firefox instance to query. And even then it only makes a difference if you imagine that there are multiple geckodriver instances all trying to connect to the same firefox, so you want to query if one of the other instances already started a session.

Basically it seems like a really niche use case which is only worthwhile supporting if it's totally trivial to do so.

Flags: needinfo?(james)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/712468713d83
[bidi] Implement "session.status" command r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/c34862fba9e0
[wdspec] Add wdspec tests for session.status r=webdriver-reviewers,whimboo
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/32114 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch

(In reply to James Graham [:jgraham] from comment #7)

That only really makes sense if we're using --connect-existing with geckodriver, since otherwise there wouldn't be a Firefox instance to query.

My question was actually more around marionette client, and I know that we wanna keep the current behavior of geckodriver. But there is actually one thing which isn't clear to me. How should geckodriver behave when we are using --connect-existing and at least one BiDi session is already active. Given that only a single session is allowed for the classic webdriver, how could geckodriver connect and create a session? I haven't fully read your other PR for the classic session refactoring, but we would need a way to allow geckodriver to create a new session - maybe HTTP handles its own list of sessions? If yes should there also be the possibility to hook into an existing session if the session id is known? If not BiDi clients would have to re-connect once the HTTP session has been created.

Flags: needinfo?(james)
Priority: P3 → P2
Whiteboard: [bidi-m2-mvp]
Points: --- → 2

This bug has been moved to milestone 2 because it was blocking another M2 bug to get fixed.

In reality the status command is not that useful most of the scenarios we care about; generally the person running tests via marionette already knows whether the browser has an existing test connection. This command only really makes sense for WebDriver-as-a-service where you don't have any existing insight into the status of the remote end.

So in general I don't think it's worth putting much effort in here. I don't have a use case for marionette clients to use the status command, and we haven't heard other demand from internal users, and using marionette directly is largely unsupported anyway, so I don't think we should prioritise changing anything there.

Even with the refactoring we are limited to a single HTTP session at a time (although there could be an existing BiDi-only session). Since marionette is only involved in HTTP sessions I think the existing setup is fine; for --connect-existing, right now, if there's already a session with the same geckodriver instance we'd return a status indicating that a session can't be created. If there's a session with another geckodriver instance, we'd return a status that creating a session might be possible, but then when it comes to actually trying to create the session should fail due to the existing session. That mismatch between the status output and the actual effect of trying to create a session is explicitly allowed by the spec.

Flags: needinfo?(james)
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.