Bug in Execute

[GCuser99]

As discussed here , I do think there is a bug in Execute. I suspect that the sessionId and other path parameters such as (element)Id should not be keys of the "parameters" dictionary that is passed to SendRequest(method, UrlBase + path, parameters).

That applies in general to ALL commands, however with my limited testing experience it only seems to cause a fatal error when setting timeouts. The key sessionId seems to be redundant info in "parameters" passed to SendRequest as sessionId is already encoded in "path" by the time SendRequest is invoked.

Why this seems to only blow up for timeouts and not for other commands I tested is a mystery, but the commands that I have tried work fine WITHOUT the path keys sessionId and (element)Id in the parameter set passed to SendRequest. So... the most immediate (and kind of ugly!) fix is to only pass a parameter set to SendRequest that contains non-path parameters as in the following version of Execute (with optional raise input parameter):

Public Function Execute(driverCommand, Optional parameters As Dictionary = Nothing, Optional ByVal raise As Boolean = True)
    Dim method As String: method = driverCommand(0)
    Dim path As String: path = driverCommand(1)
    
    If parameters Is Nothing Then
        Set parameters = New Dictionary
    End If
    
    ' Set default session id if session id is missing
    If Not parameters.Exists("sessionId") Then
        parameters.Add "sessionId", DefaultSessionId
    End If
    
    'Set path params to path, and save non-path params to nonPathParameters
    Dim paramKey As Variant, nonPathParameters As New Dictionary
    For Each paramKey In parameters
        If InStr(path, "$" & paramKey) > 0 Then 'path parameter
            path = Replace(path, "$" & paramKey, parameters(paramKey))
        Else 'non-path parameter
            nonPathParameters.Add paramKey, parameters(paramKey)
        End If
    Next paramKey
    
    ' Send request to selenium server
    Dim resp As Dictionary
    Set resp = SendRequest(method, UrlBase + path, nonPathParameters)
    
    ' Return value(s)
    If IsNull(resp("value")) Then
        Set Execute = New Dictionary 'executescript returns vbnullstring
    ElseIf TypeName(resp("value")) = "Collection" Then
        Set Execute = resp("value")
    ElseIf VarType(resp("value")) = vbObject Then
        If resp("value").Exists("error") And raise Then
            'make this user optional
            Err.raise 513, "WebDriver.Execute", JsonConverter.ConvertToJson(resp("value"))
        Else
            Set Execute = resp("value")
        End If
    Else
        Execute = resp("value")
    End If
      
End Function

After making the above change to Execute, I tested with the following:

Sub testfortimeouts()
    Dim Driver As New WebDriver
    
    Driver.Chrome
    
    sesid1 = Driver.OpenBrowser
    sesid2 = Driver.OpenBrowser
    
    Driver.SetPageLoadTimeout 1000, sesid1
    Driver.SetPageLoadTimeout 2000, sesid2
    
    Debug.Print "s1", Driver.GetPageLoadTimeout(sesid1)
    Debug.Print "s2",  Driver.GetPageLoadTimeout(sesid2)
    
    Driver.CloseBrowser sesid1
    Driver.CloseBrowser sesid2
    
    Driver.Shutdown
    
End Sub

The following is printed to immediate window:

s1             1000 
s2             2000 

I then ran a battery of different automation test subs and they all ran as expected.

Notes:

Maybe a cleaner more elegant fix to this bug is to pass two dictionaries to Execute - a path parameter dictionary and a non-path parameter dictionary.

This problem should also be corrected in the @ezagdd versions of ExecuteScript and IsElementPresent, although I have offered simplified versions of those (ExecuteScript and IsElementPresent) which call the raise-enabled Execute directly to reduce code redundancy and so, as a result, would not need to be corrected.

ありがとうございます。
Python 3.10を使って、SendRequest(method, UrlBase + path, parameters) のparametersにsessionIdが含まれない事を確認しました。

ExecuteScript から呼び出された場合は、vbNullString を返すようにしました。

Public Function Execute(driverCommand, _
                        Optional parameters As Dictionary = Nothing, _
                        Optional ByVal raise As Boolean = True)
    Dim method As String: method = driverCommand(0)
    Dim Path As String: Path = driverCommand(1)

    If parameters Is Nothing Then
        Set parameters = New Dictionary
    End If

    ' Set default session id if session id is missing
    If Not parameters.Exists("sessionId") Then
        parameters.Add "sessionId", DefaultSessionId
    End If

    ' Set path params to path, and save non-path params to nonPathParameters
    Dim paramKey As Variant
    Dim nonPathParameters As New Dictionary
    For Each paramKey In parameters
        If InStr(Path, "$" & paramKey) > 0 Then 'Path parameter
            Path = Replace(Path, "$" & paramKey, parameters(paramKey))
        Else 'non-path parameter
            nonPathParameters.Add paramKey, parameters(paramKey)
        End If
    Next paramKey

    ' Send request to selenium server
    Dim Resp As Dictionary
    Set Resp = SendRequest(method, UrlBase + Path, nonPathParameters)

    ' Return value(s)
    If IsNull(Resp("value")) Then
        If InStr(Path, "execute") = 0 Then
            Set Execute = New Dictionary
        Else
            Execute = vbNullString       'ExecuteScript Returns vbNullString
        End If
    ElseIf TypeName(Resp("value")) = "Collection" Then
        Set Execute = Resp("value")
    ElseIf VarType(Resp("value")) = vbObject Then
        If Resp("value").Exists("error") And raise Then
            'make this user optional
            Err.raise 513, "WebDriver.Execute", JsonConverter.ConvertToJson(Resp("value"))
        Else
            Set Execute = Resp("value")
        End If
    Else
        Execute = Resp("value")
    End If
      
End Function

[変更前]

    If IsNull(resp("value")) Then
        Set Execute = New Dictionary 'executescript returns vbnullstring

[変更後]

    If IsNull(Resp("value")) Then
        If InStr(Path, "execute") = 0 Then
            Set Execute = New Dictionary
        Else
            Execute = vbNullString       'ExecuteScript Returns vbNullString
        End If

ありがとうございます。
以下のコーディングだと raise = False の時、WebDriverからの本来のエラーを隠蔽してしまう事になります。
Executeの呼出し元関数で、raiseの処置を行うべきではないでしょうか。

Thank you.
With the following coding, when raise = False, the original error from WebDriver will be hidden.
Shouldn't the raise action be taken in the calling function of Execute?

        If Resp("value").Exists("error") And raise Then    ' ←←←←←
            'make this user optional
            Err.raise 513, "WebDriver.Execute", JsonConverter.ConvertToJson(Resp("value"))
        Else
            Set Execute = Resp("value")
        End If

[GCuser99]

@ezagdd, I am not sure I understand what you are asking... From above If raise=False then Set Execute = Resp("value"). So can't the caller take action from that? An example would be Navigate.

If I am misunderstanding what you mean, then please elaborate more.

説明不足でした。以下に raise = Ture と raise = False の結果を記します。
The explanation was insufficient. Below are the results of raise = True and raise = False.

① ScriptTimeout = 1ms, raise = True で、javascript を動かします。

Sub TestforScript()
Dim Driver As WebDriver
Dim height As Long

    Set Driver = New WebDriver
    Driver.Chrome "chromedriver.exe"
    Driver.OpenBrowser

    Driver.Navigate "https://www.yahoo.co.jp/"
    height = Driver.ExecuteScript("return document.body.scrollHeight", , , , 1, True)

    Driver.CloseBrowser
    Driver.Shutdown
    Set Driver = Nothing
End Sub

結果から script timeout となったことが分かります。
You can see that the result is a script timeout.

　
　
② ScriptTimeoutScriptTimeout = 1ms, raise = False で、javascript を動かします。

Sub TestforScript()
Dim Driver As WebDriver
Dim height As Long

    Set Driver = New WebDriver
    Driver.Chrome "chromedriver.exe"
    Driver.OpenBrowser

    Driver.Navigate "https://www.yahoo.co.jp/"
    height = Driver.ExecuteScript("return document.body.scrollHeight", , , , 1, False)

    Driver.CloseBrowser
    Driver.Shutdown
    Set Driver = Nothing
End Sub

結果から script timeout であることが分かりません。ExecuteScript の呼出し方が誤っていると誤認する恐れがあります。
　　実行時エラー '450':
　　引数の数が一致していません。または不正なプロパティを指定しています。
I don't know from the result that it is script timeout. You may mistakenly think that the ExecuteScript is called incorrectly.

　
　
使用した ExecuteScript と Execute です。

Public Function ExecuteScript(ByVal Script As String, _
                              Optional ScriptArgs As Variant = vbNullString, _
                              Optional ByVal ElementId As String = vbNullString, _
                              Optional ByVal SessionId As String = vbNullString, _
                              Optional ByVal timeOutms, _
                              Optional ByVal raise As Boolean = False)
    Dim Data As New Dictionary
    Dim ElmData As New Dictionary
    Dim args As Variant
    Dim savtimeOutms As Double

    Data.Add "script", Script

    ' Set ElementID and Args
    If ElementId <> vbNullString Then
        ElmData.Add "ELEMENT", ElementId
        ElmData.Add ELEMENT_KEY, ElementId
        args = Array(ElmData)
        If VarType(ScriptArgs) = vbVariant + vbArray Then    'Scriptargs is Array
            Dim i As Integer
            For i = 0 To UBound(ScriptArgs)
                ReDim Preserve args(i + 1)
                args(i + 1) = ScriptArgs(i)
            Next
        Else
            args = Array(ElmData, ScriptArgs)
        End If
    Else
        args = Array()
    End If
    Data.Add "args", args

    ' Set sessionId
    If SessionId = vbNullString Then
        Data.Add "sessionId", DefaultSessionId
    Else
        Data.Add "sessionId", SessionId
    End If

    If Not IsMissing(timeOutms) Then
        savtimeOutms = Me.GetScriptTimeout(SessionId)
        If savtimeOutms <> timeOutms Then Me.SetScriptTimeout timeOutms, SessionId
    End If

    ExecuteScript = Execute(CMD_W3C_EXECUTE_SCRIPT, Data, raise)

    If Not IsMissing(timeOutms) Then
        If savtimeOutms <> timeOutms Then Me.SetScriptTimeout savtimeOutms, SessionId
    End If

End Function

Public Function Execute(driverCommand, _
                        Optional parameters As Dictionary = Nothing, _
                        Optional ByVal raise As Boolean = True)
    Dim method As String: method = driverCommand(0)
    Dim Path As String: Path = driverCommand(1)

    If parameters Is Nothing Then
        Set parameters = New Dictionary
    End If

    ' Set default session id if session id is missing
    If Not parameters.Exists("sessionId") Then
        parameters.Add "sessionId", DefaultSessionId
    End If

    ' Set path params to path, and save non-path params to nonPathParameters
    Dim paramKey As Variant
    Dim nonPathParameters As New Dictionary
    For Each paramKey In parameters
        If InStr(Path, "$" & paramKey) > 0 Then 'Path parameter
            Path = Replace(Path, "$" & paramKey, parameters(paramKey))
        Else 'non-path parameter
            nonPathParameters.Add paramKey, parameters(paramKey)
        End If
    Next paramKey

    ' Send request to selenium server
    Dim resp As Dictionary
    Set resp = SendRequest(method, UrlBase + Path, nonPathParameters)

    ' Return value(s)
    If IsNull(resp("value")) Then
        If InStr(Path, "execute") = 0 Then
            Set Execute = New Dictionary
        Else
            Execute = vbNullString       'ExecuteScript Returns vbNullString
        End If
    ElseIf TypeName(resp("value")) = "Collection" Then
        Set Execute = resp("value")
    ElseIf VarType(resp("value")) = vbObject Then
        If resp("value").Exists("error") And raise Then
            'make this user optional
            Err.raise 513, "WebDriver.Execute", JsonConverter.ConvertToJson(resp("value"))
        Else
            Set Execute = resp("value")
        End If
    Else
        Execute = resp("value")
    End If
      
End Function

[GCuser99]

Ok @ezagdd I see now!

Proposed Lazy and Ugly Solution 1:
Assuming that a successful result passed to user from CMD_W3C_EXECUTE_SCRIPT is always a non-object (?), the easy solution is to modify above so that ExecuteScript never receives an object from Execute:

    If IsNull(resp("value")) Then
        If InStr(path, "execute") = 0 Then
            Set Execute = New Dictionary
        Else 'ExecuteScript called and needs a non-object type
            Execute = vbNullString 'ExecuteScript Returns vbNullString
        End If
    ElseIf TypeName(resp("value")) = "Collection" Then
        Set Execute = resp("value")
    ElseIf VarType(resp("value")) = vbObject Then
        If resp("value").Exists("error") Then
            If raise Then
                Err.raise 513, "WebDriver.Execute", JsonConverter.ConvertToJson(resp("value"))
            Else 'let caller handle error
                If InStr(path, "execute") = 0 Then
                    Set Execute = resp("value")
                Else 'ExecuteScript called and needs a non-object type
                    Execute = JsonConverter.ConvertToJson(resp("value"))
                End If
            End If
        Else
            Set Execute = resp("value")
        End If
    Else
        Execute = resp("value")
    End If

Now, in your test example, as you suggested earlier, since you assigned raise=false, then you the caller, are responsible for error handling:

Sub TestforScript()
    Dim Driver As WebDriver
    
    Set Driver = New WebDriver
    Driver.Chrome "chromedriver.exe"
    Driver.OpenBrowser

    Driver.Navigate "https://www.yahoo.co.jp/"
    
    'Since I am the caller, and raise=False, then it's my responsibility to handle error
    Dim height As Variant
    height = Driver.ExecuteScript("return document.body.scrollHeight", , , , 1, False)
    If VarType(height) = vbString Then
        'handle error
        Debug.Print height
    End If

    Driver.CloseBrowser
    Driver.Shutdown
    Set Driver = Nothing
End Sub

It's not pretty, but the above works on everything that I tested...

To your point "Shouldn't the raise action be taken in the function that is calling Execute?". I think agree with you. In fact, I'm wondering if caller should not only handle errors, but also how to parse the results of Execute. Why should Execute have to anticipate what the caller needs in terms of return-type, in addition to how to handle errors?

I will give this some thought and get back to you... Thanks for your patience in explaining the issue you raised above!

[GCuser99]

Here is my "prettier" solution @ezagdd for your review...

1. Execute no longer tries to anticipate the return type for the caller - that is the caller's responsibility based on the caller context (what the caller wants to do with the response). So Execute() always passes along the response dictionary from SendRequest().
2. I created two private functions GetResponseErrorMessage() and IsResponseError() to be used by Execute() and callers for error processing.
3. I went through the entire code base and made the contextual modifications to parse output from Execute. As a first pass, it was pretty easy actually. In most cases, when the result of Execute was being assigned to an object or variable, I just appended ("value") to the Execute() as in GetText = Execute(CMD_GET_ELEMENT_TEXT, Data)("value"). So the caller decides what variable type to assign the Execute results to, and how to handle errors if raise=false.
4. For Navigate, IsElementPresent, ExecuteScript, and IsAlertPresent, it was a little more involved, as my version of those methods already had some error handling.

Please see this discussion for more details.

ありがとうございます！