Skip to content

add support for pinning scripts in remote endpoints #1445

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

add support for pinning scripts in remote endpoints #1445

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
74a4120
add support for pinning scripts in remote endpoints
titusfortner Sep 26, 2019
29fed72
add updates based on feedback
titusfortner Oct 15, 2019
dc78a93
fix string comparison term
titusfortner Oct 15, 2019
c4cbfbe
removed the wrong thing
titusfortner Oct 15, 2019
d985ac4
remove 'Executing Script' as a separate section
titusfortner Oct 15, 2019
4a79a5d
move name definition into body instead of endpoint
titusfortner Oct 15, 2019
accf942
change data structure from list/table to ordered map
titusfortner Oct 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 95 additions & 2 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1186,6 +1186,14 @@ <h3>Errors</h3>
could not be satisfied because the window could not be found.
</tr>

<tr>
<td><dfn>no such script</dfn>
<td>404
<td><code>script timeout</code>
<td>No script matching the given script name was found amongst the
<a>list of known scripts</a> of the current session.
</tr>

<tr>
<td><dfn>script timeout error</dfn>
<td>500
Expand Down Expand Up @@ -2111,6 +2119,8 @@ <h2>Sessions</h2>
A <a>session</a> has an associated <a>user prompt handler</a>.
Unless stated otherwise it is in the <a>dismiss and notify state</a>.

<p>A <a>session</a> has an associated <a>executable script map</a>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be something like:

A session has an associated map of executable scripts.

Then you need to add map to the Index under the Infra entry.


<p>A <a>session</a> has an associated list of <a>active input sources</a>.

<p>A <a>session</a> has an associated <a>input state table</a>.
Expand Down Expand Up @@ -5871,7 +5881,38 @@ <h3><dfn>Get Page Source</dfn></h3>
</section> <!-- /Get Page Source -->

<section>
<h3>Executing Script</h3>
<h3>Scripts</h3>

<p>
An <dfn>executable script</dfn> is an abstraction used to
identify a script when it is transported via the
<a href="#protocol">protocol</a>, between <a>remote</a> and
<a>local</a> ends.
Comment on lines +5887 to +5890
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is correct, but I’m not sure it’s a particularly useful description. See my next comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, my thought was to do what "Elements" section does, and try to define "Script" in the context of its usage in the main section, then have subsections that talk about what you do with them.


Each script may include an optional associated <dfn>executable script name</dfn>
that uniquely identifies the script within the <a>current session</a>.

Each <a>session</a> maintains an <a>ordered map</a> <dfn>executable script map</dfn>,
with each entry having a key of an <a>executable script name</a>
and a value of an <a>exeuctable script</a>.
This map is initially empty.
Comment on lines +5895 to +5898
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already defined above under :2122.

Could you drop this paragraph and move the entry descriptions up?

(I think probably it would make more sense fo the various bit of session state to live closer to where they are used, but for the moment we should put it upstairs in the session chapter.)


To add an <a>executable script</a> to the session,
for <a>executable script map</a>, key <a>executable script name</a>, and value <var>script</var>,
set map[key] to value.
Comment on lines +5900 to +5902
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a normative definition because it is never called anywhere. We only ever add something to this map when under the Pin Script command, so it would be sufficient to make this entire paragraph a step under this command.


To remove an <a>executable script</a> from the session,
for <a>executable script map</a>, key <a>executable script name</a>, and value <var>script</var>,
remove map[key].
Comment on lines +5904 to +5906
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly.


To <dfn>get a script</dfn> with argument <var>name</var>, run the following steps:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing <p>.

<ol>
<li>If <var>name</var> is not equal to a key in the <a>executable script map</a>
for the <a>current session</a>, return <a>error</a> with <a>error code</a>
<a>no such script</a>.
<li>Let <var>script</var> be the value in the <a>executable script map</a>
associated with the key <var>name</var>.
Comment on lines +5913 to +5914
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be:

<li>Return <a>executable script map</a>[<var>name</var>].

Or in a slightly longer form:

<li>Return the restult of <a>getting the value of an entry</a> on the <a>executable script map</a> given <var>name</var>.

</ol>

<p>
A <dfn>collection</dfn> is an <a>Object</a>
Expand Down Expand Up @@ -6049,9 +6090,15 @@ <h3>Executing Script</h3>
<a>getting a property</a> named <code>script</code>
from the <var>parameters</var>.

<li><p>If <var>script</var> is set and <var>name</var> is set,
return <a>error</a> with <a>error code</a> <a>invalid argument</a>.

<li><p>If <var>script</var> is not a <a>String</a>,
return <a>error</a> with <a>error code</a> <a>invalid argument</a>.

<li><p>If <var>name</var> is set let <var>script</var> be the result
of <a>trying</a> to <a>get a script</a> with argument <var>name</var>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nitpicky, but I would prefer the slightly more English:

s/with argument/by/


<li><p>Let <var>args</var> be the result of
<a>getting a property</a> named <code>args</code>
from the <var>parameters</var>.
Expand Down Expand Up @@ -6153,6 +6200,10 @@ <h3><dfn>Execute Script</dfn></h3>
<p>The <a>remote end steps</a> are:

<ol>
<li><p>Let <var>name</var> be the result of
<a>getting a property</a> named <var>name</var>
from the <var>parameters</var>.

<li><p>Let <var>body</var> and <var>arguments</var> be the result of
<a>trying</a> to <a>extract the script arguments from a request</a>
with argument <var>parameters</var>.
Comment on lines +6203 to 6209
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And somewhere in there (and below, under Execute Async Script) we need to replace the body with the pinned script, right?

Expand Down Expand Up @@ -6216,6 +6267,10 @@ <h3><dfn>Execute Async Script</dfn></h3>
<p>The <a>remote end steps</a> are:

<ol>
<li><p>Let <var>name</var> be the result of
<a>getting a property</a> named <var>name</var>
from the <var>parameters</var>.

<li><p>Let <var>body</var> and <var>arguments</var> by the result of <a>trying</a> to
<a>extract the script arguments from a request</a> with
argument <var>parameters</var>.
Expand Down Expand Up @@ -6281,7 +6336,45 @@ <h3><dfn>Execute Async Script</dfn></h3>
and data <var>result</var>.
</ol>
</section> <!-- /Execute Async Script -->
</section> <!-- /Executing Script -->
<section>
<h3><dfn>Pin Script</dfn></h3>

<table class="simple jsoncommand">
<tr>
<th>HTTP Method</th>
<th>URI Template</th>
</tr>
<tr>
<td>POST</td>
<td>/session/{<var>session id</var>}/execute/pin</td>
</tr>
</table>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth having a getter here? We don't have a mechanism for executing code currently the only way to know a script is there is one we have placed ourselves. What happens if a user wants to use one that a Selenium SaaS sets? Do we assume they will have access to those and need to code that from the scratch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the value of limiting size, do browser vendors want it?

A getter might be a good idea. I'm not worried about getting a list from SaaS, that should be in documentation before the user writes the code, not something they need an open session to evaluate. A better use case would be the ability to:

pin_script(name, script) unless pinned_scripts.include?(name)

In which case maybe we would want to throw an exception for duplicated script names rather than just overwriting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't include it with this PR thinking that it might be better to implement something first and add it if people want it, but since it might affect how to handle other behavior, maybe I should.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can’t see a use case for getting the script, let’s avoid the complexity altogether.

I do see value in a driver refusing to pin a script for whatever reason deemed necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we discussing getting a list of names or getting the actual script? I was thinking the name list, just wanted to make sure we're discussing the same thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I thought you meant getting the full body of the scripts earlier.

Having an endpoint to get a list of the pinned scripts’ names does seems reasonable to me, for the same reasons I mentioned in #1445 (comment).

If there is no way of knowing which scripts are pinned in the driver, the local end must maintain these references locally. If it for whatever reason loses these references this could lead to a build-up of unused pinned scripts.

So far my thinking is we need:

  • An endpoint for getting a list of the pinned scripts’ names.
  • An endpoint for deleting individual pinned scripts by name.
  • Optionally a shorthand method for deleting all pinned scripts.

What do you think?

Copy link
Contributor Author

@titusfortner titusfortner Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL/DR: Is it ok if the initial implementation does not have these extra commands and we add it if people request the feature? I think if we encourage SaaS & Frameworks to namespace their scripts, these shouldn't be necessary.

If we keep current PR approach of merely overwriting name collisions, then delete is definitely not necessary, and even list might not be necessary. Overwriting the script is the most forgiving thing, and I see a lot of accidental duplicated wire calls that users send to Sauce. Even though pinning a script multiple times would be inefficient, I'm not sure it makes sense for the spec to prevent something just to protect a user from being inefficient.

But even if we do change it to return an error, the use case for delete & list would only be if the user were not in complete control over their own code (e.g. the initialization code is controlled by another team and they force everyone to use a specific implementation of "something" that another team doesn't want to use). I just don't see this happening.

I'm trying to think through scenarios for this feature. And realize vast majority of people probably won't need or use this, and if they do decide to use this to tweak performance, they are more likely to figure out the better ways to do it.

  1. Allow SaaS/Grid to handle it, so no pinning is necessary, delete isn't necessary, and list shouldn't be necessary because they'd be writing code based off of provided documentation or SaaS API endpoint, not trying to figure out what is available from the driver in the middle of a session. Hmm, maybe if Grid implements atoms with this, then it would make sense to get a list because our documentation... lags
driver.execute_script('sauce:visible', element)
driver.execute_script('se:visible', element)
  1. Allow open source test library (or user framework code) to wrap it and manage state so it's completely encapsulated from user
class Scripts
def pin(script_name)
  js = file.read File.expand_path("../scripts/#{script_name}.js", __FILE__)
  driver.pin_script("watir:#{script_name}", js)
  pinned_scripts << script_name
end
end

class Element
def visible?
  scripts.pin(:visible) unless scripts.pinned_scripts.include?(:visible)
  execute_script('watir:visible', element)
end
end

  1. Have user/framework pin ALLTHETHINGS up front, regardless of whether they'll use them. (potentially ouch)

  2. Have the user manage things as needed in their code. This is the only time I can think of when you'd need to be able to get a list, and again I'm not sure why you'd want to delete things.

driver.pin_script('visible', script) unless driver.pinned_scripts.include?('visible')

If the whole point of this feature is to reduce latency and bandwidth for remote interactions, this is going to add an extra wire call every time, which isn't ideal, but maybe would provide the flexibility for teams that can't figure out how to do it in a better way? Then again, users are only going to do this themselves if they figure out it improves performance, otherwise they'll stop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to make a wholesome API that doesn’t just cater to SaaS users of WebDriver. I think it would be an unfortunate piece of design if WebDriver didn’t allow the pinned scripts to be queried.

An intermediary node (SaaS) or a local end (client) might know which scripts it has already pinned and take whatever action it feels compelled to make to prevent/allow overwriting scripts, but these are fundamentally not concerns of the specification. The specification goes to great length to not specify the concerns of the local end, meaning they are free to implement the API as they wish. But it is of course useful and indeed necessary to take into consideration the use cases local ends have.

The problem I have with a setter-only approach is the assumption that local ends are always stateful, or that they in all cases have the correct picture of the remote end’s state. Let me quickly give two examples:

  1. A stateless WebDriver client might not have the ability to store states across invocations if all it knows about on the local system is the WebDriver session ID. This means unless there’s an API that allows querying the list of pinned scripts, it is impossible for it know if it’s modifying/overwriting an existing script.

  2. Even with a stateful client that locally maintains references to the pinned scripts, nothing prevents an intermediary node or another client from making additional requests without the original client’s knowledge. In other words, accumulating and trusting state built up on the local end is not atomic in the sense that other endpoints might’ve manipulated the pinned scripts.

Because of the particular nature of the script pinning API and the way it accumulates data in memory, I think it is important to have primitives that allow the pinned scripts to be queried and cleared. My proposal would be to have:

  • POST /session/{session id}/execute/pin with a JSON body {name: "…", script: "…"} that would overwrite any script by the same name. (Optionally you might consider returning {overwritten: true/false}, but I don’t particularly see a use case for it and it could always be introduced later.)
  • GET /session/{session id}/execute/pin returning a JSON Array of scripts by their names, i.e. ["One", "Two", …].
  • DELETE /session/{session id}/execute/pin/{?name} that deletes a single script by name if name is given, or all pinned scripts.

This mirrors accurately how you might design other RESTful APIs for your web app or whatever.

Of course we can contain this PR to the first API and follow up on the others later, but I feel it is important to have a clear picture in mind of where we are heading.


<p>The <a>remote end steps</a> are:

<ol>
<li><p>Let <var>name</var> be the result of
<a>getting a property</a> named <var>name</var>
from the <var>parameters</var>.

<li><p>Let <var>script</var> be the result of
<a>getting a property</a> named <var>script</var>
from the <var>parameters</var>.

<li><p>If <var>script</var> is <a>undefined</a> or is not a <a>String</a>,
return <a>error</a> with <a>error code</a> <a>invalid argument</a>.

<li><p>If <var>name</var> is <a>undefined</a> or is not a <a>String</a>,
return <a>error</a> with <a>error code</a> <a>invalid argument</a>.

<li><p>If <var>name</var> is equal to a key in the <a>executable script map</a>
remove the <a>executable script</a> from the session

<li><p>add the <a>executable script</a> to the session.
Comment on lines +6370 to +6373
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more in line with how Infra suggests using maps, both of these steps can be replaced with:

<li><p>Set <a>executable script map</a>[<var>name</var>] to <var>script</var>.


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing successful return here.

</ol>
</section> <!-- /Pin Script -->
</section> <!-- /Scripts -->
</section> <!-- /Document -->


Expand Down