Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async errors in executeAsync are not getting caught by Selenium #999

Closed
georgecrawford opened this issue Jan 8, 2016 · 21 comments
Closed

Comments

@georgecrawford
Copy link

In my tests, I'm using this pattern when I need to run browser.executeAsync:

const result = browser.executeAsync(function(param, done) {
    setTimeout(function() {
        const result = getResult();
        if (result) {
            done([null, result]);
        } else {
            done(['getResult() failed because...']);
        }
    }, 1000);
});
return checkAsyncResult(result)

function checkAsyncResult(result) {
    if (!result || !Array.isArray(result.value)) {
        throw Error(`Unexpected reponse from executeAsync: ${JSON.stringify(result.value)}`);
    }

    if (result.value[0]) {
        throw result.value[0];
    }

    return result.value[1];
};

It'd be quite nice if webdriverio could do this automatically, and we could standardise on the [(err), res] format for async responses. It's somewhat related to the idea of making WebElement JSON objects first-class citizens.

@monolithed
Copy link

+1

@christian-bromann
Copy link
Member

@georgecrawford I am not sure what you mean by standardise on the [(err), res] format. Do you mean that if the first argument is an object we should throw an error? Why not just do:

const result = browser.executeAsync(function(param, done) {
    setTimeout(function() {
        const result = getResult();
        if (result) {
            done([null, result]);
        } else {
            throw new Error('getResult() failed because...']);
        }
    }, 1000);
});

@georgecrawford
Copy link
Author

Throwing an error in the browser's context? I haven't tried that, but had assumed Selenium wouldn't catch and return it. Am I wrong?

@monolithed
Copy link

I haven't tried that, but had assumed Selenium wouldn't catch and return it.

It's a good question, throw new Error can produce some side effects.

window.onerror = error => {
  console.log(error); // Uncaught Error: error!
}

throw new Error('error!');
window.onerror = error => {
  console.log(error); // Nothing
}

try {
  throw new Error('error!');
}
catch (error) {
  console.log(error); //Uncaught Error: error!
}

And

new Promise((resolve, reject) => {
  try {
    throw new Error('error!');
  }
  catch (error) {
      resolve(error);
  }
})
.then(result => {
  console.log(result); // throw new Error('error!');
})
.catch(error => {
  console.log(error); // Nothing
})

Unlike a local try/catch block, the window.onerror handler doesn’t have direct access to the exception object, and is executed in the global context rather than locally where the error occurred.

So, we have to implement resolve and reject or return

@georgecrawford
Copy link
Author

I'm going to bow out of this thread, because @christian-bromann is right - throwing an error in the browser context is picked up by Selenium, and reported to webdriverio. I hadn't realised this. So I have what I need.

@christian-bromann
Copy link
Member

👍

@monolithed
Copy link

window.onerror = error => {
    // But here is a place where you can change the behavior on the site 
    // and exception from WD is not expected.
}

@christian-bromann
Copy link
Member

What do you mean?

@monolithed
Copy link

// Index.js
window.onerror = function (error) {
     document.body.backgroundColor = 'red';
};
// Tests.js
it ('do something', function () {
    ...
    browser.executeAsync(function (param, done) {
          throw new Error('failed because...']);
    });
});

it ('check background color', function () {
    ...
    browser.getCssProperty('body', 'color').then(function (color) {
        return color.value;
    })
   .should.be.eventually.equal('white'); // "red" is not expected for this test
})

Do you see what I mean, now? )

@monolithed
Copy link

One exception in a single test may affect other tests, I don't like it.
What is the problem to implement reject?

@georgecrawford
Copy link
Author

Damn, I've just realised that this doesn't get caught by Selenium, so I just get a timeout instead:

        browser.executeAsync(function(done) {
            setTimeout(function() {
                throw Error('George error')
            }, 10)
        })

As that's a primary use-case for me (async errors), I now think once again that we should have a better system for reporting async browser-land errors.

@christian-bromann
Copy link
Member

I think we can build that in and catch errors as they occur

@georgecrawford
Copy link
Author

How?

@christian-bromann
Copy link
Member

We could register error handler here

@georgecrawford
Copy link
Author

I think you misunderstood me - in my example above, there is no HTTP response from Selenium when the error is thrown in the browser, so there's nothing webdriverio can do to catch it. Instead, there's just an async script execution timeout. At least, that's what I see with Chromedriver. Can you reproduce?

I can't find a spec of any kind for what the defined behaviour of executeAsyncScript should be wrt errors. Do you know of one?

@christian-bromann christian-bromann added this to the Upcoming milestone Apr 6, 2016
@christian-bromann christian-bromann changed the title Standardise return value for browser.executeAsync async errors in executeAsync are not getting caught by Selenium Apr 6, 2016
@nikolmarku1
Copy link

On my test running on real device Ipad Mini the test hangs forever after appium try to execute async script: using appium with selenium grid ... appium server exe the comand but gets no response and does not communicate with grid to stop the test ....

[debug] [MJSONWP] Responding to client with driver.getStatus() result: {"build":{"version":"1.6.3","revision":null}}
[HTTP] <-- GET /wd/hub/status 200 8 ms - 83
[HTTP] --> POST /wd/hub/session/64d8740f-9430-49b3-969c-cac4ab4db649/execute_async {"script":"var callback = arguments[arguments.length - 1];window.et = window.et || {}; window.et.state = window.et.state || [];et.state.push(null, function(){et.state.get('',function( v ){if (v) { callback(JSON.stringify(v)) } else { callback('error'); } })});","args":[]}
[debug] [MJSONWP] Calling AppiumDriver.executeAsync() with args: ["var callback = arguments[arguments.length - 1];window.et = window.et || {}; window.et.state = window.et.state || [];et.state.push(null, function(){et.state.get('',function( v ){if (v) { callback(JSON.stringify(v)) } else { callback('error'); } })});",[],"64d8740f-9430-49b3-969c-cac4ab4db649"]
[debug] [XCUITest] Executing command 'executeAsync'
[HTTP] --> GET /wd/hub/status {}

@nikolmarku1
Copy link

and the same happen with another script ....
[debug] [MJSONWP] Calling AppiumDriver.executeAsync() with args: ["var c = arguments[0];alert(c);c(1);",[],"c9915bfc-adaa-438d-8574-5cba4c1963a5"]
[debug] [XCUITest] Executing command 'executeAsync'
[debug] [iOS] Real device safari test and no custom callback address set, changing callback address to local ip.
[debug] [iOS] Response url for executeAsync: http://10.24.209.41:4726/wd/hub/session/c9915bfc-adaa-438d-8574-5cba4c1963a5/receive_async_response
[debug] [RemoteDebugger] Executing 'execute_async_script' atom in default context
[debug] [RemoteDebugger] Sending javascript command (function(){return function(){var e=this;
funct...
[debug] [RemoteDebugger] Sending WebKit data: {"method":"Runtime.evaluate...
[HTTP] --> GET /wd/hub/status {}
[debug] [MJSONWP] Calling AppiumDriver.getStatus() with args: []
[debug] [MJSONWP] Responding to client with driver.getStatus() result: {"build":{"version":"1.6.3","revision":null}}
[HTTP] <-- GET /wd/hub/status 200 9 ms - 83
[MJSONWP] Encountered internal error running command: TimeoutError: operation timed out
at afterTimeout (/usr/local/lib/node_modules/appium/node_modules/bluebird/js/main/timers.js:16:15)
at Timeout.timeoutTimeout [as _onTimeout] (/usr/local/lib/node_modules/appium/node_modules/bluebird/js/main/timers.js:59:9)
at ontimeout (timers.js:365:14)
at tryOnTimeout (timers.js:237:5)
at Timer.listOnTimeout (timers.js:207:5)
[HTTP] <-- POST /wd/hub/session/c9915bfc-adaa-438d-8574-5cba4c1963a5/execute_async 500 5011 ms - 190

@glukki
Copy link

glukki commented Apr 24, 2018

I was just reading specs, and I think you can return promise from a passed function.. But you still will have to call callback in the end: https://www.w3.org/TR/webdriver/#h-execute-async-script
I haven't experimented with it yet..

my current understanding of spec (timeout handling skipped):

// request.script === 'return (<function>).apply(null, arguments)' // wdio wrapper
// request.args === whatever

// Execute Async Script
const f = new Function(request.script);
return Promise((resolve, reject) => {
  const result = promiseCall(f, ...(args.concat(resolve)));
  result.catch(reject);
  // but `result`'s resolved value is never used
})

// Promise-Calling
function promiseCall(f, ...args) {
  try {
    return Promise.resolve(f(...args));
  } catch (e) {
    return Promise.reject(e);
  }
}

So if that's true, at least we can return promise from function, and reject it to pass error back to wdio.

Unfortunately, Chrome implementation is different: https://github.com/bayandin/chromedriver/blob/37323a96c0ba20c1eeb6bcccd44bc59bcd800e67/js/execute_async_script.js#L53

@glukki
Copy link

glukki commented Apr 24, 2018

I'm almost sure in my understanding of how this should work, according to spec.
If you agree with me, please star this issue: https://bugs.chromium.org/p/chromedriver/issues/detail?id=2398

@christian-bromann
Copy link
Member

@glukki Chromedriver has a lot of things not implemented according to the WebDriver spec. We will have to wait until they catch up with the development. I am sure once they release a new version it will be working like that.

@christian-bromann
Copy link
Member

This behavior can not be influenced by WebdriverIO and needs to get fixed on the protocol level. There is already activity on this by the browser vendors: web-platform-tests/wpt#13781

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants