karma-runner / karma-sauce-launcher

A Karma plugin. Launch any browser on SauceLabs!
MIT License
199 stars 103 forks source link

Error in heartbeat function #198

Closed rcebulko closed 3 years ago

rcebulko commented 4 years ago

When using the launcher, we encounter an error in the heartbeat function

[20:01:01] TypeError: Cannot read property 'title' of undefined
    at Timeout._onTimeout (/home/travis/build/ampproject/amphtml/node_modules/karma-sauce-launcher/dist/launcher/launcher.js:36:20)
    at listOnTimeout (internal/timers.js:549:17)
    at processTimers (internal/timers.js:492:7)

Relevant snippet:

  // Heartbeat function to keep alive sessions on Sauce Labs via webdriver JSON wire calls
  const heartbeat = () => {
    pendingHeartBeat = setTimeout( (driver) => {
      log.debug('Heartbeat to Sauce Labs (%s) - fetching title', browserName)

      driver.title()
        .then(null, (err) => {
          log.error('Heartbeat to %s failed\n  %s', browserName, formatSauceError(err))

          clearTimeout(pendingHeartBeat)
          return this._done('failure')
      });

      heartbeat()
      }, 60000);
}

I'm not sure how driver could get set; I'm not familiar with setTimeout providing any arguments to its callback. Should this be trying to read from connectedDrivers or something similar?

/cc @joshmgrant

joshmgrant commented 4 years ago

@rcebulko Hmm, interesting issue. Do you have a CI build or Sauce job link along with this?

rcebulko commented 4 years ago

https://travis-ci.org/github/ampproject/amphtml/jobs/668922609?utm_medium=notification&utm_source=github_status LMK if you can't access that log

triadiprabowo commented 4 years ago

I am experiencing same problem in version 4.1.2

tdalbo92 commented 4 years ago

Took me awhile to dig down into this, but it's affecting us as well. I've got it reproducing locally on my system, and I'd be happy to debug if possible.

I almost wonder if selenium-webdriver changed their public API, this should be using getTitle() instead of .title()

rcebulko commented 4 years ago

I almost wonder if selenium-webdriver changed their public API, this should be using getTitle() instead of .title()

A lot of this code hasn't been updated in a couple years, it wouldn't surprise me if some of the package dependencies that were updated in those years between releases might have included API changes

joshmgrant commented 4 years ago

It looks like @tdalbo92 is correct! The WebdriverJS API has changed driver.title() to driver.getTitle(). I'll submit a PR soon which includes the null check and this updated call to get the page title.

rcebulko commented 4 years ago

Thanks for debugging this @tdalbo92 anf @joshmgrant for the fix! Created https://github.com/karma-runner/karma-sauce-launcher/pull/203 from Josh's PR so tests can run. If all is green, I'll merge and hopefully that fixes it

rcebulko commented 4 years ago

Seeing a new error: Heartbeat to ${browserName} failed: driver is null\n%s. This looks like it's happening here: https://github.com/karma-runner/karma-sauce-launcher/blob/master/src/launcher/launcher.ts#L50

@joshmgrant Where is driver supposed to come from here? Why is setTimeout providing it as an argument?

rcebulko commented 4 years ago

@joshmgrant I think we should probably roll back https://github.com/karma-runner/karma-sauce-launcher/commit/74eea9247f0333ba0fc352c6fe22a0f820d07831 until it's fully working, master has been borked for a while now :disappointed:

tdalbo92 commented 4 years ago

This is definitely causing chaos for our longer running test in CI. I did a bit more debugging, and some really weird concurrency issues are happening when it hiccups - both before and after #203 . Existing tests want to keep running even after the exception is thrown, but the runner has already started to kill the browser and try again.

By the time it starts the new tests, the old ones finish up and close the session, causing the entire thing to fail.

It's Schrodinger's Karma - it's passing and failing at the same time, so either way the cat is dead.

joshmgrant commented 4 years ago

@rcebulko that sounds reasonable to roll back. I'm going to look into this though, try to get some tests working in this project and see where to go from there.

rcebulko commented 4 years ago

@joshmgrant Have you had any luck with a fix for the heartbeat approach?

joshmgrant commented 4 years ago

@rcebulko not yet but I'm going to see if I can get something together in the next few days

wswebcreation commented 3 years ago

Re-implemented the heartbeat and released it in version https://github.com/karma-runner/karma-sauce-launcher/releases/tag/v4.2.0, closing this for now