sclevine / agouti

A WebDriver client and acceptance testing library for Go
MIT License
822 stars 105 forks source link

`Click` on invisible element fails in Chrome but Agouti ignores error and returns success #186

Open hasryan opened 5 years ago

hasryan commented 5 years ago

I wrote a test that attempted to click on an invisible element, and was surprised to find that the click did not return an error.

Example:

With this html:

<html><a id="foo" style="display:none">Invisible link</a></html>

This passes:

var page *agouti.Page
...
Expect(page.FindById("foo").Click()).To(Succeed())

The issue appears to be that Agouti expects ChromeDriver to indicate errors with a non-2XX status code, while (at least my version of ChromeDriver) returns HTTP 200 even when a failure occurred.

I added some debug statements and found that the response from ChromeDriver does report this click error using the status property of the response as described here: https://github.com/SeleniumHQ/selenium/wiki/JsonWireProtocol#response-status-codes

For the above example, it responded with the body:

{
  "sessionId": "...",
  "status": 11,
  "value": {"message":"element not interactable\\n (Session info: ..."}
}

My analysis:

When the click request is sent here: https://github.com/sclevine/agouti/blob/96599c91888f1b1cf2dccc7f1776ba7f511909e5/api/element.go#L80 It does not provide a pointer for the response. Which results in the client not attempting to parse the response (aside from checking HTTP status code): https://github.com/sclevine/agouti/blob/96599c91888f1b1cf2dccc7f1776ba7f511909e5/api/internal/bus/client.go#L29

My fix replaced the following block of code from https://github.com/sclevine/agouti/blob/96599c91888f1b1cf2dccc7f1776ba7f511909e5/api/internal/bus/client.go#L29-L34

With this in its place:

bodyValue := struct{
    Status int `json:"status"`
    Value interface{} `json:"value"`
}{}
if result != nil {
    bodyValue.Value = result
}

if err := json.Unmarshal(responseBody, &bodyValue); err != nil {
    return fmt.Errorf("unexpected response: %s", responseBody)
} else if bodyValue.Status != 0 {
    return fmt.Errorf("unexpected response (nonzero status %d): %s", bodyValue.Status, responseBody)
}

I am not sure if this is an issue with ChromeDriver not not returning the correct HTTP status code, or if it's reporting errors in a valid way. For this reason I am not sure if it's safe to make this change or if it could cause issues with other browser drivers.

I am using ChromeDriver v 2.46 on macOS