Alert disappears instantly leads to org.openqa.selenium.NoAlertPresentException

[punkratz312]

by clicking a link which triggers and alter dialog it instantly disappears and code does break due to NoAlertPresentException. when clicking the link by hand it works fine, so maybe there is an issue...

working example (with manual clicking workaround)

https://user-images.githubusercontent.com/8830888/119782533-9659b780-becc-11eb-8a6a-1c2ded97564c.mov

failing test due to instant disappear of alert dialog:

https://user-images.githubusercontent.com/8830888/119784351-66131880-bece-11eb-9b55-4d472f1507e9.mov

Test:

Clicking action: (find correct element but instant disappear)

chrome.switches:

[wakaleo]

Dig into the code but I don't think Serenity (or Selenium, for that matter) does anything special to clear an alert dialog automatically - it looks like maybe a chrome configuration thing.

[punkratz312]

chrome works fine, if i click the link by hand all works good if the click happens via serenity id disappears instantly so i dont think its an chrome issue.

[wakaleo]

Possible a chrome driver issue. Have you tried with other browsers?

[wakaleo]

Otherwise, dig into the code and see if there is a point where something the code is doing makes the alert disappear, or if it happens when the WebDriver call is made.

[punkratz312]

Please see attached video, when clicking the link (several times) with serenity its totally not working. the dialog should stay there by default but its every time hiding instantly so please there must be an issue. if i do the action by hand it works perfectly fine, only serenity/selenium cant do the trick...

https://user-images.githubusercontent.com/8830888/119792717-3700a500-bed6-11eb-87b4-17140d2a4811.mov

[wakaleo]

There may well be an issue, but the question is where it is. I've never seen this behaviour before so can't say. It could be something the Serenity action is doing, something the Selenium code is doing, or something the chrome driver is doing. Simulating it by hand doesn't help isolate the problem - you'll need to step into the code with a debugger and find at what point the alert is being closed.

[punkratz312]

all right thanks

[Jacobvu84]

The problem comes from chromedriver. Now alert will be dismissed is default. If you want to handle the alert pop-up, it should be overrided the capability

Try with this. Put in in the serenity.properties or serenity.conf

driver_capabilities.common.unexpectedAlertBehaviour=ignore

[Jacobvu84]

@punkratz312 Pls check and close issue

[punkratz312]

great thank you 🙏

Sent from my iPhone

On 10. Jun 2021, at 11:25, Jacob Vu @.***> wrote:

 The problem comes from chromedriver. Now alert will be dismissed is default. If you want to handle the alert pop-up, it should be overried the capability

Try with this. Put in in the serenity.properties or serenity.conf

driver_capabilities.common.unexpectedAlertBehaviour=ignore — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[alipala-ah]

great. thank you! saved my time a lot.

[Zsar]

Reproduced in Serenity 3.3.1 and 3.3.2 with chromedriver 97.0.4692.71.

Added the setting from above to my serenity.conf to no effect.

The alert is closed in the following call stack:

• RemoteWebDriver#execute(java.lang.String) - this throws an UnhandledAlertException because there is the alert we very much do expect!
• RemoteWebDriver#getScreenshotAs
• WebDriverFacade#getScreenshotAs
• WebDriverScreenShooter#takeScreenshot
• PhotoSession#takeScreenshot
• PhotoSessionBooking#andSaveToDirectory
• BaseStepListener#screenshotFrom
• BaseStepListener#grabScreenshots
• BaseStepListener#take(BaseStepListener.ScreenshotType, TestResult)
• BaseStepListener#takeEndOfStepScreenshotFor
• BaseStepListener#stepFinished
• StepEventBus#stepFinished
• StepInterceptor#notifyStepFinishedFor - this is the immediate follow-up of invokeMethod
• StepInterceptor#executeTestStepMethod - here StepInterceptor#invokeMethod makes the alert appear

So far, so good. We violated a precondition by not "expecting" the alert. That would be a user error, no ill behaviour has happened so far. But from here, sadly, it all starts to go down the drain:

In WebDriverFacade#getScreenshotAs the exception is swallowed via "logger.warn" antipattern / unfinished code / idk and the method returns null instead of correctly communicating violated preconditions, even though this breach of contract has now caused undefined behaviour and corrupted our test case! (We just tried to take a screeenshot, which should be a pure read operation, but actually we have modified the page aka performed a write operation.)

PhotoSession#takeScreenshot then returns ScreenshotPhoto#None after deciding that failure to take a screenshot is retroactive proof that a screenshot should not have been taken in the first place (via PhotoSession#shouldIgnore, which fails-fast if screenshotData is empty).

At this point, we have no alert and no screenshot. The next action (clicking on a button inside the alert) will fail, because the alert no longer exists. We will have no clue why, because manually doing the same steps will work just fine.

I am pretty sure this is a bug in Serenity, not in chromedriver: Expected behaviour would be Serenity failing the test case and informing me that I have to expect the alert.

... Actually, it might even be two bugs: If I wanted a screenshot and cannot get one - surely that is an error!? A program should help the user accomplish their goals, not redefine those goals for them. Even less so after the fact (if "not" had an "even less", that is).

Please advise how to proceed, @Jacobvu84 , @wakaleo . Should I open a new issue? Should this one be re-opened? Can I do more to help?

FWIW: I tried to search for "Serenity expect alert" or "Selenium expect alert" to learn how I would go about turning this "unexpected" alert into an expected one - at least in theory, the alert should then no longer be destroyed and my test case should work. (Then I could inspect the working code in order to form an understanding about how the broken code should work.) Unfortunately I could not unearth pertaining results. References welcome!

Here is the step definition I used. I am not yet experienced with Serenity, so YMMV, but it looks to me reasonable for a simple start.

public interface My {
    Duration PATIENCE = Duration.ofSeconds(5l);
}

public class MainPage extends PageObject {
    static final Target USER_INFO_BUTTON = /* user menu button on the page */;

    public Performable openUserInfo() {
        return Task.where("{0} opens the user menu"
                , Ensure.that(USER_INFO_BUTTON.waitingForNoMoreThan(My.PATIENCE)).isDisplayed()
                , Click.on(USER_INFO_BUTTON)
        );
    }
}

public class Logout extends PageObject {
    static final Target LOGOUT = /* logout button in the user menu, which will display the alert */;

    public Performable logout() {
        return Task.where("{0} clicks on Logout"
                , Ensure.that(LOGOUT.waitingForNoMoreThan(My.PATIENCE)).isDisplayed()
                , Click.on(LOGOUT)
        );
    }
}

public class Alert extends PageObject {
    public Performable clickAlertOK()  {
        return new ClickOnAlert(); // could not yet figure out how to do this with Task.where syntax
    }

    class ClickOnAlert implements Task {
        @Override
        public void performAs(final Actor actor) {
            final var alert = new WebDriverWait(getDriver(), My.PATIENCE)
                    .until(ExpectedConditions.alertIsPresent()); // alert already gone => will fail with TimeoutException
            // unreachable
            alert.accept();
        }
    }
}

public class LogoutDefinitionSteps {
    Alert alert;
    Logout logout;
    MainPage mainPage;

    @Then(/* Cucumber step to logout */)
    public void logout(final Actor actor) {
        actor.attemptsTo(
                this.mainPage.openUserMenu(),
                this.logout.logout(),
                this.alert.clickAlertOK()
        );
    }
}

[Zsar]

More FWIW: I have reproduced the same scenario in Testerra 1.2 and that one handles the logout alert correctly, using the same chromedriver. So yes, looks like a pure Serenity issue.

[wakaleo]

If you can trace this back to Serenity code, can you dig into the code and propose a fix in a PR?

[Zsar]

Mmh. I fear I'd need some direction, as to how the corrected behaviour should look like:

• I have not yet interacted with the screenshot-taking mechanism at all, so it's involvement is at default behaviour.
• I can easily see that "if I want to take a screenshot, I really do want to take a screenshot", so the behaviour as-is is wrong - but how would a correct behaviour look like (that is: several behaviours would be technically more correct , but which of those would fit the design intention of Serenity-BDD?)

... I will try to find out first, how to fix the user error(?) that makes this situation arise. To that end I have posted a StackOverflow question. Once I know how the working test case looks like, maybe I can devise proper error handling for the broken one.

(By my current understanding these might all be valid approaches, depending on constraints I am not yet aware of:

• Can't screenshots be taken in this situation? Fail test execution and notify in failure message: "You must disable screenshots for tasks triggering alerts!"
  • Aka basically do not catch UnhandledAlertException or wrap and rethrow it.
• Can screenshots be taken but Serenity is doing it wrong? Fix Serenity to correctly take screenshots.
  • As the logic is buried pretty far from userland, the detailed solution is probably not quite important then, e.g. "retry different ways until one sticks" might be a valid approach. ... But given that we currently do that and it murders the alert , probably not.
• Extend API in such a way that alert handling can happen before alert-causing action completes? E.g. Click.on(<target>.andHandleAlert(<task>)) or cleaner Click.on(<target>).andHandleAlert(<task>)?
• Something different entirely? Can't tell yet.

)

[Zsar]

... I have found this piece of code in W3CHttpResponseCodec#decode:

// For now, we'll inelegantly special case unhandled alerts.
if ("unexpected alert open".equals(error) &&
    HTTP_INTERNAL_ERROR == encodedResponse.getStatus()) {
  String text = "";
  Object data = obj.get("data");
  if (data != null) {
    Object rawText = ((Map<?, ?>) data).get("text");
    if (rawText instanceof String) {
      text = (String) rawText;
    }
  }
  response.setValue(new UnhandledAlertException(message, text));
}

and further instances of the behaviour here and here. The latter issue also claims (in the labels) that Firefox is just as affected as Chrome.

It would thence seem to me that screenshot taking during visible alerts is an operation generally unsupported by WebDriver. Proposed fix thence:

1. extend API with alert handling methods, such that screenshots will only be taken before triggering and after handling, if these methods have been called
2. propagate UnhandledAlertException to make test fail-fast and inform user that they must do either/or:
   • disable automatic screenshot taking
   • use methods introduced in 1 to prevent automatic screenshot taking from destroying their alert

I do not think we can do better, because at the time we get the WebDriver response in W3CHttpResponseCodec#decode the alert is already gone and the test execution is thence tainted.

(I guess one could try to implement some sort of automatic retry of the current step? But I do not see this working, because at least on the page I used for testing, one would have to replay the last four steps to recreate the alert (the logout button is inside a menu which closes when the alert is triggered, so we have to first reopen the menu, and only then can replay the step whose call stack we are currently in.)

... Does that sound like a proper solution? Then I'll try to create a PR. - Advice on the API welcome (e.g. how the methods should be called and into which classes they should go).

[wakaleo]

We generally don't fail a test if a screenshot cannot be taken, because this in itself doesn't mean that the application logic is broken, just that the framework can't take a screenshot for some reason. The broader problem here is that you don't necessarily know if an alert will be triggered, so a more robust solution would be to avoid taking screenshots at all if an alert is present (presuming this is a Webdriver limitation).

I imagine you could do this either in the PhotoSession class in the takeScreenshot() method (by catching the exception), and/or in the shouldTakeScreenshots() method in BaseStepListener (by checking whether an alert is currently being displayed).

I've implemented a simple approach in the current main branch (https://github.com/serenity-bdd/serenity-core/commit/dce6b5b011ca0618d8c1ed948628a517b6222b42) - see if this approach works for your problem.

[Zsar]

I think that will not work because at the time you have that exception, the alert has already been destroyed and the session is thus corrupted.

But I figured: Can we not in one of the "may I take screenshots" methods - e.g. in WebDriverFacade#driverCanTakeScreenshots- check for alerts and evaluate false if any are present?

E.g. change

private boolean driverCanTakeScreenshots() {
  return (TakesScreenshot.class.isAssignableFrom(getDriverClass()));
}

to

private boolean driverCanTakeScreenshots() {
  final boolean isGenerallyCapable = TakesScreenshot.class.isAssignableFrom(getDriverClass());
  if (isGenerallyCapable) {
    try { // slow but...
      @SuppressWarnings("unused")
      final var unused = this.getProxiedDriver().switchTo().alert();
    }
    catch(final NoAlertPresentException e) { // ... does not destroy alert
      return true;
    }
  }
  return false;
}

(But probably rather in WebDriverFacade#canTakeScreenshots; I optimised for brevity, while it's not clear whether the approach would even be okay.)

... Will test your code and mine and report back.

[wakaleo]

I also thought of checking with switchtTo().alert(), which could work. The only issues are that it will slow down the tests (you have to do it for every screenshot), and it could potentially interfere with the tests if they have already switched to other frames or dialogs etc.

One option might be to check for the alert just before taking the screenshot itself - screenshots are a slow operation anyway.

Do you have a sample project to reproduce this?

[Zsar]

Yupp, tried yours and mine, as expected yours does not change the behaviour while mine does.

Re speed: I think in WebDriverFacade#driverCanTakeScreenshots we are already as close as we can get? The next calls are:

• ((TakesScreenshot) getProxiedDriver()).getScreenshotAs(target)
  • RemoteWebDriver#execute(java.lang.String) <= here starts Selenium code

I still think just failing at the exception and directing the user to use a different API call would be best, but unfortunately that approach seems to be off the table?

Created an MVE.

[wakaleo]

Could you have a look at the latest code on main? I've integrated your approach.

[wakaleo]

The problem with forcing the user to disable screenshots in interactions that trigger alerts is that it could be very hard to know what action might generate an alert, and it would bind the test code very tightly to the UI, which could make the tests fragile.

[Zsar]

It gets worse... just noticed on my MVE. The code I posted up here works afterwards, but there I used Switch.toAlert().andAccept() - and that again manages to somehow kill the alert before it can "find" it.

... Ugh. One thing at a time. Will add a "dumbified" test to the MVE for this issue. Will keep the "too smart" test for the next issue (created #2894 ).

[wakaleo]

Did you have a look at my latest push? I used Switch.toAlert().text() to avoid interacting with the alert

[Zsar]

Did you have a look at my latest push? I used Switch.toAlert().text() to avoid interacting with the alert

Just did. Also works. Given you put it directly on main, I can delete my branch, yes?