Fix alerts on chrome

[aaltat]

When running acceptance test with chrome, it seems that selenium quite often fails on: alert.text or alert.dismiss executions with WebDriverException indicating error on JavaScript execution. This commit simplifies the alert handling and creates a re-try logic if selenium raises WebDriverException when performing actions on alerts.

[pekkaklarck]

Are these constants? If yes, they could be class variables and use capital letters:

class _AlertKeywords(...):
    _ACCEPT = 'accept'
    _DISMISS = 'dismiss'

[aaltat]

Done in next commit

[pekkaklarck]

See my line notes. Otherwise looks good.

[aaltat]

Updated PR based on the comments

[pekkaklarck]

Looks good to me. It might still be a good idea to rename _DISMISS to _DISMISS_ALERT or to __DISMISS to avoid accidental collisions with attribute names. The underlying issue there is the multi-inheritance approach used by S2L. We could try designing a better architecture for larger RF libraries like it, but that is a totally unrelated and larger discussion than this.

[aaltat]

Yeah, multi inheritance is... The _DISMISS_ALERT is better and I think we should change the _ACCEPT to _ACCEPT_ALERT too.

[aaltat]

The better architecture is a good idea. Hopefully it could be something that could be applied for other projects too. But as said, not in the 2.0 release.

I generally don't like multi inheritance, because it makes stuff complicated in places where it should not be. Although there is a use case for multi inheritance but in my mind the inheritance line should not be tree, like in S2L, but it should be line like. Also the inheritance length should not be long. Maximum of two or three objects and I tend to favour only one in many cases.

But that is totally different topic, I will make the changes later and let's see how code looks like.

[aaltat]

Updated PR based on comments.

[pekkaklarck]

Yeah, I meant both DISMISS and ACCEPT in my previous comment. Was using mobile then and wanted to keep the comment short. My point about double underscore version __DISMISS_ALERT was that it would prevent accidental overriding by sub and sibling classes. I doubt it matters here, but it might be a good convention in general unless we want to redesign the whole architecture in somewhat near future. That redesign shouldn't actually be too big a task--I got ideas how it could look like--and we might even consider it in 2.0.

I let you @aaltat decided what you want to do with the naming at this point. Feel also free to merge the PR when you consider it ready.

[aaltat]

Ah, I have forgotten, again, what __ means in Python. I thought it was a typo. Will add it and merge as single commit in to the master.

Only thing with the change is that because alerts are brokenen again, should we create a 1.8.1 release and cherry pick this commit on top of the 1.8.0 tag. Of course it will create a smallish mess for everyone history...

Perhaps if none reports a problem we could wait for the 2.0.0 release.

[emanlove]

On 09/14/2016 10:45 AM, Tatu Aalto wrote:

Ah, I have forgotten, again, what |__| means in Python. I thought it
was a typo. Will add it and merge as single commit in to the master.

Only thing with the change is that because alerts are brokenen again,
should we create a 1.8.1 release and cherry pick this commit on top of
the 1.8.0 tag. Of course it will create a smallish mess for everyone
history...

Perhaps if none reports a problem we could wait for the 2.0.0 release.

Not sure I see the need to do anything that screws with the history
(which is generally a bad thing to do)? Can't it just be added and a
1.8.1 release be made?

Ed

[pekkaklarck]

Messing up history isn't a good idea, but creating a separate 1.8-maintenance branch ought to be OK.If this isn't urgent, we can simply wait for 2.0 too.

[aaltat]

Only person, who I know, that has complained on the problem is one of my colleagues. And for him I made workaround. If none else complain we can wait until 2.0.0.