jasmine / jasmine-browser-runner

Serve and run your Jasmine specs in a browser
49 stars 23 forks source link

support any remote selenium grid #32

Closed bicarbon8 closed 1 year ago

bicarbon8 commented 1 year ago

Changes:

Justification:

this project is listed on the Karma repo as a viable alternative to Karma now that it is no longer being updated. given this, there is a need to support more than just Saucelabs integration and since the landscape of Selenium-As-A-Service providers vastly differs in their capabilities requirements it makes more sense to take an approach of letting the user specify the required capabilities and hub url rather than trying to keep up with all the possible service providers by building direct integrations for each.

Additional Notes:

sgravrock commented 1 year ago

I appreciate the PR. Although feature parity with Karma is a non-goal, I agree that Selenium Grid support would be a good feature to have. However, I can't merge this in its current state:

I tried this out locally by patching jasmine-core as follows, based on your changes to spec/sauceIntegrationSpec.js:

diff --git a/spec/support/jasmine-browser.js b/spec/support/jasmine-browser.js
index 334efcf1..78511554 100644
--- a/spec/support/jasmine-browser.js
+++ b/spec/support/jasmine-browser.js
@@ -27,15 +27,12 @@ module.exports = {
   ],
   random: true,
   browser: {
-    name: process.env.JASMINE_BROWSER || 'firefox',
-    useSauce: process.env.USE_SAUCE === 'true',
-    sauce: {
-      name: `jasmine-core ${new Date().toISOString()}`,
-      os: process.env.SAUCE_OS,
-      browserVersion: process.env.SAUCE_BROWSER_VERSION,
-      build: `Core ${process.env.TRAVIS_BUILD_NUMBER || 'Ran locally'}`,
-      tags: ['Jasmine-Core'],
-      tunnelIdentifier: process.env.SAUCE_TUNNEL_IDENTIFIER,
+    browserName: process.env.JASMINE_BROWSER || 'firefox',
+    url: 'https://ondemand.saucelabs.com/wd/hub',
+    browserVersion: process.env.SAUCE_BROWSER_VERSION,
+    platformName: process.env.SAUCE_OS,
+    "sauce:options": {
+      "tunnel-identifier": process.env.SAUCE_TUNNEL_IDENTIFIER,
       username: process.env.SAUCE_USERNAME,
       accessKey: process.env.SAUCE_ACCESS_KEY
     }

With that change applied, when I run jasmine-core's scripts/start-sauce-connect followed by scripts/run-all-browsers, all browsers fail with connection failures of one sort or another. That's similar to what I see when I run jasmine-browser-runner's spec/sauceIntegrationSpec.js.

I have some nits to pick about how the config is set up, but the bigger issues are that this would need to be done in a way that keeps existing Saucelabs and local configs working, and we'd need to figure out a way to test the Selenium Grid support on CI.

bicarbon8 commented 1 year ago

thanks for the detailed feedback and investigation into the PR. I'd be happy to modify the PR such that it constitutes only a minor version upgrade and retains full backwards compatibility with the previous configuration structure while still allowing for the new structure as well. I don't have Saucelabs credentials so I had to base my config examples on their own capabilities generator, but maybe I can get a demo account and investigate the issues. for the comment around testing against an actual grid hub as part of the CI suite of tests, would you prefer an actual running grid or simply testing against the standard that a selenium grid should conform to?

oh, and as to breaking local browser functionality. when running the tests locally all local Firefox instances launched successfully; the node 18 and 20 errors looked like problems accessing the Firefox binary on the test agent which I assume is a problem on the agent image and not with the changes being proposed

sgravrock commented 1 year ago

I don't have Saucelabs credentials so I had to base my config examples on their own capabilities generator, but maybe I can get a demo account and investigate the issues

I think that's a good idea. My experience has been that just following the docs isn't always enough to arrive at working code (both in general and for Saucelabs in particular). While I can't give you access to Jasmine's Saucelabs account, the test results themselves are public so I can share a couple of links.

On main: https://app.saucelabs.com/tests/909e7d1b2dc8401b80f8c58344af68e7 On your PR branch: https://app.saucelabs.com/tests/9eed744a3fb741a3bdc6503cc0821bf0#1

If you look a few seconds before the end of the video on the second link, you can see the browser displaying a connection error. I'm not sure what's causing it. The metadata looks the same between the two except for how the username and tags are passed. I've confirmed that neither of those is the problem. Notably, the Assigned tunnel id is the same in both cases, but for some reason the browser isn't able to connect back through the tunnel in the second case.

would you prefer an actual running grid or simply testing against the standard that a selenium grid should conform to?

Testing against the standard already didn't work, so I think we need an actual running grid. That'll also give a working reference implementation for users (and for me, when I need to help people with it later on). I think either Saucelabs or BrowserStack would be fine for that.

oh, and as to breaking local browser functionality. when running the tests locally all local Firefox instances launched successfully; the node 18 and 20 errors looked like problems accessing the Firefox binary on the test agent which I assume is a problem on the agent image and not with the changes being proposed

That's a strange assumption. The only difference between the passing builds and the failing builds are the code changes in this PR. The CI infrastructure is the same. I'd be starting from the assumption that the problem was caused by the code changes, not the infrastructure that didn't change. All the more so since the code changes include both bugs and intentional breaking changes.

I was able to reproduce the same 9 test failures on a local Linux box, albeit with timeouts instead of the error messages shown on CI. I also noticed, not just on Linux, that ~10 tests that previously ran headless Chrome are now running Firefox. (It's really noticeable because each Firefox instance grabs focus, effectively making the computer unusable for the duration of npm test.) So it looks like these changes broke local browser selection as well.

bicarbon8 commented 1 year ago

@sgravrock I believe I've completed all your requested changes as well as fixing the issues that you saw in the Saucelabs tests. most of it turned out to be a misunderstanding on my part with how the tests were running as the actual configuration based on Saucelabs' documentation was correct.

Testing against the standard already didn't work, so I think we need an actual running grid. That'll also give a working reference implementation for users (and for me, when I need to help people with it later on). I think either Saucelabs or BrowserStack would be fine for that.

for the above, I added integration tests that follow the design of the already existing sauceIntegrataionSpec called remoteGridIntegrationSpec that runs using the new, more flexible, configuration format against Saucelabs. during this I discovered that Saucelabs doesn't automatically route requests based on geographical zone like BrowserStack does meaning that the useSauce with a sauce object implemetation (v2.0.0) would not work for users outside the US since a region-specific URL is required to connect to Saucelabs' grid hub.

That's a strange assumption. The only difference between the passing builds and the failing builds are the code changes in this PR. The CI infrastructure is the same. I'd be starting from the assumption that the problem was caused by the code changes, not the infrastructure that didn't change. All the more so since the code changes include both bugs and intentional breaking changes.

apologies for my incorrect assumption. our work build agents sometimes are updated between builds causing similar failures so that was my basis, but you were correct. I didn't realise that the local tests launching actual Firefox instances wasn't how it was meant to work. after making the changes backwards compatible, this resolved the issue and now the headless browsers are used instead.

as I'm based in the EU, I was forced to locally update the URL used for connecting to Saucelabs' grid hub to the EU URL when running the useSauce tests and confirmed that the tests all passed, but I would appreciate if you could run in your integration environment to confirm the tests work as expected. if you have any additional feedback, please let me know (I know you mentioned something about the config structure previously, but without a proposal I kept this the same as it's something I've used successfully in other projects when connecting to multiple grid hub providers). Thanks again for your consideration of this change.

sgravrock commented 1 year ago

Thanks. This looks pretty good.

I discovered that Saucelabs doesn't automatically route requests based on geographical zone like BrowserStack does meaning that the useSauce with a sauce object implemetation (v2.0.0) would not work for users outside the US since a region-specific URL is required to connect to Saucelabs' grid hub.

It's interesting that you're the first person to run into this. That suggests that the existing Saucelabs support isn't very widely used.

I would appreciate if you could run in your integration environment to confirm the tests work as expected

Looks good. Here's a successful run. I also see the right set of browsers showing up in the Saucelabs UI. jasmine-core tests also work in Saucelabs when using this branch.

I'll follow up later (today hopefully) with some notes about the config format and any other details.

bicarbon8 commented 1 year ago

@sgravrock thanks again for the feedback. I've completed the configuration changes you've requested and allowed for fallback to a local instance of a Selenium Grid when no URL is provided. If no capabilities are specified then only the browserName capability is passed to the WebDriver builder. I was able to test that the integration tests all passed on my side, but again I would appreciate if you could confirm the same for your side because I have to manually modify the Saucelabs URL since I'm running them from the EU and they don't route automatically based on geographical location. I've also reverted the version change; apologies for changing it as I didn't realise your release process will automatically update that value.

sgravrock commented 1 year ago

Looks good. Thank you!