feat(local): forward connection_settings.binaryPath to browserstack-local [SDK-7284] - #1176
Open
anish353 wants to merge 1 commit into
Open
feat(local): forward connection_settings.binaryPath to browserstack-local [SDK-7284]#1176anish353 wants to merge 1 commit into
anish353 wants to merge 1 commit into
Conversation
…ocal [SDK-7284]
`browserstack-local` accepts a `binarypath` option that skips the binary
download entirely, but the CLI never forwarded one, so a customer whose network
blocks the download had no way to point the run at a binary they had already
placed on disk. `setLocalArgs` now passes it through.
The key it forwards is deliberately all-lowercase: `browserstack-local` only
special-cases `binarypath`, and any other casing falls through its `addArgs`
default branch and is handed to the binary as an unknown CLI flag while the
download happens anyway. Both `binaryPath` and `binarypath` are accepted in
`browserstack.json` for the same reason — a silently ignored config key is the
failure mode this change exists to remove.
Also reorders two requires in the test file. `usageReporting` reassigns
`module.exports` after its own circular `require('./utils')`, so requiring it
before `utils` left `utils` holding a stale, empty exports object: nothing
stubbed in the test was visible to the code under test, and `setLocalArgs`
threw. That is why the existing `setLocalArgs` spec was already failing on
master.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A customer's network blocks the BrowserStackLocal binary download, so they asked for a way to point the CLI at a binary they had already placed on disk.
browserstack-localhas supported exactly that for a long time — abinarypathoption that short-circuitsLocalBinaryentirely — but the CLI never forwarded one.setLocalArgsforwards fiveconnection_settingskeys today (local_identifier,local_config_file,proxyHost,proxyPort,useCaCertificate).binaryPathappears nowhere inbin/, so setting it inbrowserstack.jsonis silently dropped and the download path is always taken. That silent drop is what made this hard to diagnose from the outside: there is no log line saying the key was ignored.Reported and root-caused on SDK-7284.
What lands here
bin/helpers/utils.jssetLocalArgsforwards a configured binary path asbinarypath. BothbinaryPathandbinarypathare accepted inbrowserstack.json.test/unit/bin/helpers/utils.jsbinaryPathis forwarded as the lowercase key, the lowercase config key is accepted too, and no key is emitted when none is configured. Plus a require-order fix, below.The forwarded key is deliberately all-lowercase.
browserstack-localonly special-casesbinarypathinaddArgs; any other casing falls through to itsdefault:branch and is handed to the binary as an unknown CLI flag — while the download still happens:Accepting both spellings in
browserstack.jsonis for the same reason — a user copyingbinarypathfrom the browserstack-local README should not land back in the silent-drop case this PR exists to remove.Verification
Against the published
browserstack-cypress-cli@1.36.18with its realbrowserstack-local@1.5.13, using a stub binary that reports{"state":"connected"}so the assertion is which binary got spawned:local_argsfromsetLocalArgs{key, localIdentifier, daemon, enable-logging-for-api, source}— nobinarypath{... , binarypath: "<path>"}On the fixed run
browserstack-localprintsBINARY PATH IS DEFINEDandstartreturns no error. The argv it hands the binary matches the shape in the customer's error line, including the duplicate--daemon/--sourcethe two layers each add.Test plan
setLocalArgs(forwarded key, lowercase alias, absent key).test/unit/bin/helpers/utils.js: 398 passing / 3 failing on this branch vs 393 passing / 5 failing on a cleanmaster. Net +5 passing, zero regressions. The 3 remaining failures are pre-existinggetVideoConfigspecs, red onmastertoo.Why two pre-existing failures go green
bin/helpers/usageReporting.jsrequires./utils(line 9) and then reassignsmodule.exports(line 21). RequiringusageReportingbeforeutilstherefore leavesutilsholding the empty exports object captured mid-cycle, while the test holds the real one — sousageReporting.cli_version_and_pathwas undefined insidesetLocalArgs, nothing stubbed in the test was visible to the code under test, and the existing spec threw. Requiringutilsfirst makes it capture the finished exports.This is a test-harness fix only; no production code depends on the ordering, since the CLI's entry points already load
utilsfirst. The stale-reference hazard inusageReporting.jsitself is left alone as out of scope for this ticket.