feat(functional-tests): add pairing E2E test with marionette authority#20119
feat(functional-tests): add pairing E2E test with marionette authority#20119
Conversation
de2e451 to
23dda8b
Compare
| expect(pairUrl).toBeTruthy(); | ||
| expect(pairUrl).toContain('/pair#'); | ||
| expect(pairUrl).toContain('channel_id='); | ||
| if (!pairUrl) throw new Error('pairUrl is null'); |
4a66af3 to
ee5412b
Compare
a9e9176 to
a1dc25e
Compare
Add end-to-end test for the Firefox device pairing flow using a Marionette-driven Firefox authority and Playwright supplicant. - Minimal Marionette TCP client (marionette.ts) - Firefox process manager with temp profile lifecycle (marionette-firefox.ts) - Playwright fixture using bundled Firefox (125+) as Marionette authority - Uses beginOAuthFlow for proper PKCE/keys_jwk registration - Condition-based waits replacing hardcoded sleeps
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| /** | ||
| * End-to-end pairing flow test. |
There was a problem hiding this comment.
This is the most important comment of the PR, we need marionette to access FF internals to get the QR code url. While iterating it turns out you don't need to take a picture and decode the url, you can use the internal libs to get the url.
Once you have the url (channel_id & channel_key) you can build the pairing url and get the flow connected.
| } from '../../lib/pairing-helpers'; | ||
|
|
||
| // Increase timeout — pairing involves launching a separate Firefox + channel negotiation | ||
| test.setTimeout(120_000); |
There was a problem hiding this comment.
Locally this test takes about 60s to complete,=
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| /** | ||
| * Minimal Marionette protocol client over raw TCP. |
There was a problem hiding this comment.
I probably would have never thought of doing it this way, but I like it. This introduces zero deps and just talks to marionette via tcp.
There was a problem hiding this comment.
Interesting... Can you get Claude to write some specific tests for it? There's enough here I think it should be under test. It'd be confusing if something in this layer started failing unexpectedly.
| export { expect } from '@playwright/test'; | ||
|
|
||
| /** | ||
| * Fetch the pairing channel server URI from the target's well-known config. |
There was a problem hiding this comment.
Lots of tokens were spent trying to figure this out. Turns out that the channel server on local/stage/prod are all different 🤷🏽
There was a problem hiding this comment.
Really? What does that even mean? Are they actually different versions, do they just behave differently? Can we get local aligned with prod?
| this.buffer = Buffer.alloc(0); | ||
|
|
||
| await this.rawConnect(); | ||
| const helloRaw = await this.recvPacket(); |
| headless?: boolean; | ||
| } | ||
|
|
||
| export class MarionetteFirefox { |
There was a problem hiding this comment.
Add comment explaining what "MarionetteFirefox" is.
| await client.connect(10, 2000); | ||
| for (let attempt = 0; ; attempt++) { | ||
| try { | ||
| await client.newSession(); |
There was a problem hiding this comment.
Is this loop really needed? It seems kind of odd that you have to do this for a connection (line 100) and then again for a session...
| killProcessOnPort(marionettePort); | ||
|
|
||
| // Create temp profile with FXA + Marionette prefs | ||
| const profileDir = fs.mkdtempSync( |
There was a problem hiding this comment.
Do we have to worry about file permissions here? Guessing not since its using os.tempdir()?
| resolve(); | ||
| }, 5000) | ||
| ); | ||
| await Promise.race([exited, timeout]); |
There was a problem hiding this comment.
I don't get it... So it's trying to kill the process again, if just respond fast enough the first time? If the first kill didn't work, why would the second one?
|
|
||
| // Create temp profile with FXA + Marionette prefs | ||
| const profileDir = fs.mkdtempSync( | ||
| path.join(os.tmpdir(), 'fx-pairing-test-') |
There was a problem hiding this comment.
You probably want to add a postfix here. This will always return the same directory, which could cause problems if two processes try to run this code concurrently.
| ): Promise<MarionetteFirefox> { | ||
| const { | ||
| firefoxBinary, | ||
| marionettePort = 2828, |
There was a problem hiding this comment.
Does it always run on this port? What if there's a port conflict?
| const parsedPid = parseInt(pid.trim(), 10); | ||
| if (isNaN(parsedPid) || parsedPid <= 0) continue; | ||
| try { | ||
| process.kill(parsedPid, 'SIGKILL'); |
There was a problem hiding this comment.
It's hard to know what's running on the port provided here... Should you there be some kinda guard rail here. Like checking for 'marionette' in the process path something?
| try { | ||
| await this.sendCommand('WebDriver:DeleteSession'); | ||
| } catch { | ||
| // Ignore errors during session deletion |
| * Try to extract a complete packet from the buffer. | ||
| * Packet format: "LENGTH:JSON_BODY" | ||
| */ | ||
| private tryDeliverPacket(): void { |
There was a problem hiding this comment.
I'm not so sure about this function... Seems like it works, but I can see it being flaky. I'll outline why...
| if (!this.pendingResolve) return; | ||
|
|
||
| const colonIdx = this.buffer.indexOf(0x3a); // ':' | ||
| if (colonIdx === -1) return; |
There was a problem hiding this comment.
Are we sure the buffer will always contains this? What if the buffer is big, and the ':' is in the next/prev chunk. Maybe this isn't a big deal... and the message format is always small enough to fit in the buffer... just checking though.
| ): Promise<Record<string, unknown>> { | ||
| for (let attempt = 0; attempt < retries; attempt++) { | ||
| try { | ||
| if (this.sock) { |
There was a problem hiding this comment.
This implies we don't want two active connections, which makes sense... But multiple MarionetteClient instnaces can be created which means you could have multiple sockets created and bound to the marionette port... If marinonette doesn't support multiple clients, then we should make class a singleton to avoid any future issues.
| const expectedId = this.msgId; | ||
| const msg = JSON.stringify([0, expectedId, name, params]); | ||
| const packet = `${msg.length}:${msg}`; | ||
| this.sock.write(packet, 'utf-8'); |
There was a problem hiding this comment.
This isn't checking if the write went through. It could have been partial / not fully drained...
| const packet = `${msg.length}:${msg}`; | ||
| this.sock.write(packet, 'utf-8'); | ||
|
|
||
| const raw = await this.recvPacket(); |
There was a problem hiding this comment.
AFAICT, if the buffer hasn't recieved data yet, this will just return... I'm not sure how this ensures synchronization. I bet this works most the time, but I can also see it failing if marionnette is slow or laggy to respond to the command.
Because
This pull request
lib/marionette.ts) to drive a real Firefox instance for the authority side of pairinglib/marionette-firefox.ts) that launches Playwright's bundled Firefox with Marionette enabled, manages temp profiles, and cleans up on teardownlib/fixtures/pairing.ts) that wires the Marionette authority into the test lifecycle usingfirefox.executablePath()tests/pairing/pairingFlow.spec.ts) exercising: OAuth sign-in viabeginOAuthFlow→ start pairing → channel handshake → authority approval → supplicant confirm → both sides completelib/pairing-constants.ts) and helpers (lib/pairing-helpers.ts) with condition-based waits (no hardcoded sleeps)CI_WAF_TOKENbypass header in CI for stage/production runsIssue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-9501
Checklist
Put an
xin the boxes that applyOther information (Optional)
How to test:
Note: Expect ~30-35s per run.
MARIONETTE_HEADLESS=falseshows the authority Firefox window;--headedshows the Playwright supplicant browser.est.