I did not preserve this very_verbose method call; it is here for the purposes of extra debug logging, and as far as I can tell has not been useful for quite some time. Happy to put it back in if folks would prefer; just let me know

This is the type of print statement I think is honestly easier just to insert yourself if/when you need it, custom to your specific needs.

This does have a side effect of making this variable available when using the local browser, when formerly it was only available when using the remote browser. As far as I can tell, this does not impact the experience of running tests locally in any way, and doing it this way makes it a bit cleaner to add an alternative remote browser. Let me know if you have any concerns about the change, and I can find a solution that more thoroughly preserves existing behavior.

totally fine by me, simple + easy-to-search => good

I find $globals are relatively innocuous when all refs are same-file, esp when well namespaced.

1. There's only reference to $http_client outside connect.rb, in steps.rb! It could easily be replaced by a def change_orientation() method in connect.rb that is invoked in steps.rb. The same pattern is already used by with_read_timeout() in steps.rb which is invoked in steps.rb and thereby hides the $http_client. Then $http_client would only be ref-ed same-file, woot.
2. This one could be more namespaced: e.g. $selenium_http_client. ick, but imo global is not the time to be picky about terse and clean to read.

The above two changes would make me happier, but I'm not unhappy as-is either.

this check is necessary to avoid throwing an exception if flakiness_for_test returns nil for whatever reason. I don't think we want to block our ability to run the test in the event we don't have flakiness data

I wouldn't hate if this was over-rideable by an env var. Not a must-have, but would allow say a 3rd party to use their suacelabs endpoint which happened to be on us-east-1 or whatever.

totally fine by me, simple + easy-to-search => good

I find $globals are relatively innocuous when all refs are same-file, esp when well namespaced.

1. There's only reference to $http_client outside connect.rb, in steps.rb! It could easily be replaced by a def change_orientation() method in connect.rb that is invoked in steps.rb. The same pattern is already used by with_read_timeout() in steps.rb which is invoked in steps.rb and thereby hides the $http_client. Then $http_client would only be ref-ed same-file, woot.
2. This one could be more namespaced: e.g. $selenium_http_client. ick, but imo global is not the time to be picky about terse and clean to read.

The above two changes would make me happier, but I'm not unhappy as-is either.

1. I like+prefer the simplification you've done here with window-size=1280,1024 always being passed in.
2. This /does/, technically, I think, change options on Safari (which might ignore, no idea), and probably more worrying what happens on stuff like iPad? Probably a no-op?
3. I'm still consistently confused by how browsers.json aka $browser_config intersects (or doesn't??), which also has resolution related fields, but that's kind of a me problem:

  {
    "name": "Chrome",
    "browserName": "chrome",
    "browserVersion": "105",
    "platformName": "Windows 10",
    "sauce:options": {
      "screenResolution": "1280x1024",
      "extendedDebugging": true
    }
  },

I suspect there's a simplification to be had here, somewhere, if only because I can't quite grok what we have now lol.

OK, so unwinding this just for myself if for no other reason:

1. capabilities: in SeleniumBrowser.remote(url, capabilities: nil) below is generally ~~= $browser_config
2. because SeleniumBrowser.remote is only called from def saucelabs_browser() in connect.rb
3. which sets capabilities = Selenium::WebDriver::Remote::Capabilities.new($browser_config.except('name')), and then more-or-less just sets a locally defined capabilities[:sauce_options] and passes $browser_config.
4. And $browser_config == browser.json

Would it be crazy to add another method to SeleniumBrowser called SeleniumBrowser.remote_saucelabs, which invokes self.remote, and refactors the sauce-labs specific capabilities munging code (and loading browser.json) out of connect.rb?

This /does/, technically, I think, change options on Safari (which might ignore, no idea), and probably more worrying what happens on stuff like iPad?

Oh, good catch; I figured making Safari more consistent with the other browsers was a positive, but I wasn't thinking at all about mobile devices. I think I'll scope this down to just if [:chrome, :firefox].include?(browser) for now.

The relationship between this setting and the one in browsers.json is also unclear to me; in particular, it's not clear to me whether SauceLabs would be able to default to the resolution defined in Selenium if we don't explicitly specify a resolution in sauce:options.

Would it be crazy to add another method to SeleniumBrowser called SeleniumBrowser.remote_saucelabs [...]

When I was first putting this together, I spent a while standing at the precipice of the $browser_config / $http_client rabbit hole, debating diving in.

The main reason I decided not to for $browser_config is that I'd then also have to untangle the implementation and use of the single_session? helper method, which felt like too much for this PR. I'd be strongly in favor of us doing some cleanup there as a follow-up, but I wanted to keep this one at least somewhat focused. I also liked the idea of keeping all code that referenced ENV variables in one place, and keeping the code in selenium_browser.rb a bit more environment-agnostic.

I could be much more easily convinced to fold some $http_client improvements in here, though; it was a little annoying to accommodate in this refactor, and I didn't realize it was only externally referenced from a single place 👀