[dotnet] Solidify nullability of PinnedScript (#14708)

Author: RenderMichael

dotnet/src/support/Events/EventFiringWebDriver.cs

@@ -478,6 +478,7 @@ public object ExecuteScript(string script, params object[] args)
         /// <param name="script">A <see cref="PinnedScript"/> object containing the code to execute.</param>
         /// <param name="args">The arguments to the script.</param>
         /// <returns>The value returned by the script.</returns>
+        /// <exception cref="ArgumentNullException">If <paramref name="script"/> is <see langword="null"/>.</exception>
         /// <remarks>
         /// <para>
         /// The ExecuteScript method executes JavaScript in the context of
@@ -509,6 +510,11 @@ public object ExecuteScript(string script, params object[] args)
         /// </remarks>
         public object ExecuteScript(PinnedScript script, params object[] args)
         {
+            if (script == null)
+            {
+                throw new ArgumentNullException(nameof(script));
+            }
+
             IJavaScriptExecutor javascriptDriver = this.driver as IJavaScriptExecutor;
             if (javascriptDriver == null)
             {

dotnet/src/webdriver/IJavaScriptEngine.cs

@@ -87,13 +87,15 @@ public interface IJavaScriptEngine : IDisposable
         /// <param name="scriptName">The friendly name by which to refer to this initialization script.</param>
         /// <param name="script">The JavaScript to be loaded on every page.</param>
         /// <returns>A task containing an <see cref="InitializationScript"/> object representing the script to be loaded on each page.</returns>
+        /// <exception cref="ArgumentNullException">If <paramref name="scriptName"/> is <see langword="null"/>.</exception>
         Task<InitializationScript> AddInitializationScript(string scriptName, string script);
 
         /// <summary>
         /// Asynchronously removes JavaScript from being loaded on every document load.
         /// </summary>
         /// <param name="scriptName">The friendly name of the initialization script to be removed.</param>
         /// <returns>A task that represents the asynchronous operation.</returns>
+        /// <exception cref="ArgumentNullException">If <paramref name="scriptName"/> is <see langword="null"/>.</exception>
         Task RemoveInitializationScript(string scriptName);
 
         /// <summary>
@@ -109,13 +111,15 @@ public interface IJavaScriptEngine : IDisposable
         /// </summary>
         /// <param name="script">The JavaScript to pin</param>
         /// <returns>A task containing a <see cref="PinnedScript"/> object to use to execute the script.</returns>
+        /// <exception cref="ArgumentNullException">If <paramref name="script"/> is <see langword="null"/>.</exception>
         Task<PinnedScript> PinScript(string script);
 
         /// <summary>
         /// Unpins a previously pinned script from the browser.
         /// </summary>
         /// <param name="script">The <see cref="PinnedScript"/> object to unpin.</param>
         /// <returns>A task that represents the asynchronous operation.</returns>
+        /// <exception cref="ArgumentNullException">If <paramref name="script"/> is <see langword="null"/>.</exception>
         Task UnpinScript(PinnedScript script);
 
         /// <summary>

dotnet/src/webdriver/IJavascriptExecutor.cs

@@ -17,6 +17,7 @@
 // under the License.
 // </copyright>
 
+using System;
 using System.Collections.Generic;
 
 namespace OpenQA.Selenium
@@ -98,6 +99,7 @@ public interface IJavaScriptExecutor
         /// variable, as if the function were called via "Function.apply"
         /// </para>
         /// </remarks>
+        /// <exception cref="ArgumentNullException">If <paramref name="script" /> is <see langword="null"/>.</exception>
         object ExecuteScript(PinnedScript script, params object[] args);
 
         /// <summary>

dotnet/src/webdriver/ISearchContext.cs

@@ -17,6 +17,7 @@
 // under the License.
 // </copyright>
 
+using System;
 using System.Collections.ObjectModel;
 
 namespace OpenQA.Selenium
@@ -31,6 +32,7 @@ public interface ISearchContext
         /// </summary>
         /// <param name="by">The locating mechanism to use.</param>
         /// <returns>The first matching <see cref="IWebElement"/> on the current context.</returns>
+        /// <exception cref="ArgumentNullException">If <paramref name="by" /> is <see langword="null"/>.</exception>
         /// <exception cref="NoSuchElementException">If no element matches the criteria.</exception>
         IWebElement FindElement(By by);
 

dotnet/src/webdriver/JavaScriptEngine.cs

@@ -219,14 +219,25 @@ public async Task ClearInitializationScripts()
         /// </summary>
         /// <param name="script">The JavaScript to pin</param>
         /// <returns>A task containing a <see cref="PinnedScript"/> object to use to execute the script.</returns>
+        /// <exception cref="ArgumentNullException">If <paramref name="script"/> is <see langword="null"/>.</exception>
         public async Task<PinnedScript> PinScript(string script)
         {
+            if (script == null)
+            {
+                throw new ArgumentNullException(nameof(script));
+            }
+
+            string newScriptHandle = Guid.NewGuid().ToString("N");
+
             // We do an "Evaluate" first so as to immediately create the script on the loaded
             // page, then will add it to the initialization of future pages.
-            PinnedScript pinnedScript = new PinnedScript(script);
             await this.EnableDomains().ConfigureAwait(false);
-            await this.session.Value.Domains.JavaScript.Evaluate(pinnedScript.CreationScript).ConfigureAwait(false);
-            pinnedScript.ScriptId = await this.session.Value.Domains.JavaScript.AddScriptToEvaluateOnNewDocument(pinnedScript.CreationScript).ConfigureAwait(false);
+
+            string creationScript = PinnedScript.MakeCreationScript(newScriptHandle, script);
+            await this.session.Value.Domains.JavaScript.Evaluate(creationScript).ConfigureAwait(false);
+            string scriptId = await this.session.Value.Domains.JavaScript.AddScriptToEvaluateOnNewDocument(creationScript).ConfigureAwait(false);
+
+            PinnedScript pinnedScript = new PinnedScript(script, newScriptHandle, scriptId);
             this.pinnedScripts[pinnedScript.Handle] = pinnedScript;
             return pinnedScript;
         }
@@ -236,11 +247,17 @@ public async Task<PinnedScript> PinScript(string script)
         /// </summary>
         /// <param name="script">The <see cref="PinnedScript"/> object to unpin.</param>
         /// <returns>A task that represents the asynchronous operation.</returns>
+        /// <exception cref="ArgumentNullException">If <paramref name="script"/> is <see langword="null"/>.</exception>
         public async Task UnpinScript(PinnedScript script)
         {
+            if (script == null)
+            {
+                throw new ArgumentNullException(nameof(script));
+            }
+
             if (this.pinnedScripts.ContainsKey(script.Handle))
             {
-                await this.session.Value.Domains.JavaScript.Evaluate(script.RemovalScript).ConfigureAwait(false);
+                await this.session.Value.Domains.JavaScript.Evaluate(script.MakeRemovalScript()).ConfigureAwait(false);
                 await this.session.Value.Domains.JavaScript.RemoveScriptToEvaluateOnNewDocument(script.ScriptId).ConfigureAwait(false);
                 this.pinnedScripts.Remove(script.Handle);
             }

dotnet/src/webdriver/PinnedScript.cs

@@ -17,83 +17,70 @@
 // under the License.
 // </copyright>
 
-using System;
 using System.Globalization;
 
+#nullable enable
+
 namespace OpenQA.Selenium
 {
     /// <summary>
     /// A class representing a pinned JavaScript function that can be repeatedly called
     /// without sending the entire script across the wire for every execution.
     /// </summary>
-    public class PinnedScript
+    public sealed class PinnedScript
     {
-        private string scriptSource;
-        private string scriptHandle;
-        private string scriptId;
-
         /// <summary>
         /// Initializes a new instance of the <see cref="PinnedScript"/> class.
         /// </summary>
         /// <param name="script">The body of the JavaScript function to pin.</param>
+        /// <param name="stringHandle">The unique handle for this pinned script.</param>
+        /// <param name="scriptId">The internal ID of this script.</param>
         /// <remarks>
         /// This constructor is explicitly internal. Creation of pinned script objects
         /// is strictly the perview of Selenium, and should not be required by external
         /// libraries.
         /// </remarks>
-        internal PinnedScript(string script)
+        internal PinnedScript(string script, string stringHandle, string scriptId)
         {
-            this.scriptSource = script;
-            this.scriptHandle = Guid.NewGuid().ToString("N");
+            this.Source = script;
+            this.Handle = stringHandle;
+            this.ScriptId = scriptId;
         }
 
         /// <summary>
         /// Gets the unique handle for this pinned script.
         /// </summary>
-        public string Handle
-        {
-            get { return this.scriptHandle; }
-        }
+        public string Handle { get; }
 
         /// <summary>
         /// Gets the source representing the body of the function in the pinned script.
         /// </summary>
-        public string Source
-        {
-            get { return this.scriptSource; }
-        }
+        public string Source { get; }
 
-        /// <summary>
-        /// Gets the script to create the pinned script in the browser.
-        /// </summary>
-        internal string CreationScript
+        internal static string MakeCreationScript(string scriptHandle, string scriptSource)
         {
-            get { return string.Format(CultureInfo.InvariantCulture, "function __webdriver_{0}(arguments) {{ {1} }}", this.scriptHandle, this.scriptSource); }
+            return string.Format(CultureInfo.InvariantCulture, "function __webdriver_{0}(arguments) {{ {1} }}", scriptHandle, scriptSource);
         }
 
         /// <summary>
         /// Gets the script used to execute the pinned script in the browser.
         /// </summary>
-        internal string ExecutionScript
+        internal string MakeExecutionScript()
         {
-            get { return string.Format(CultureInfo.InvariantCulture, "return __webdriver_{0}(arguments)", this.scriptHandle); }
+            return string.Format(CultureInfo.InvariantCulture, "return __webdriver_{0}(arguments)", this.Handle);
         }
 
         /// <summary>
         /// Gets the script used to remove the pinned script from the browser.
         /// </summary>
-        internal string RemovalScript
+        internal string MakeRemovalScript()
         {
-            get { return string.Format(CultureInfo.InvariantCulture, "__webdriver_{0} = undefined", this.scriptHandle); }
+            return string.Format(CultureInfo.InvariantCulture, "__webdriver_{0} = undefined", this.Handle);
         }
 
         /// <summary>
         /// Gets or sets the ID of this script.
         /// </summary>
-        internal string ScriptId
-        {
-            get { return this.scriptId; }
-            set { this.scriptId = value; }
-        }
+        internal string ScriptId { get; }
     }
 }

dotnet/src/webdriver/WebDriver.cs

@@ -280,16 +280,23 @@ public object ExecuteScript(string script, params object[] args)
         /// <param name="script">A <see cref="PinnedScript"/> object containing the JavaScript code to execute.</param>
         /// <param name="args">The arguments to the script.</param>
         /// <returns>The value returned by the script.</returns>
+        /// <exception cref="ArgumentNullException">If <paramref name="script" /> is <see langword="null"/>.</exception>
         public object ExecuteScript(PinnedScript script, params object[] args)
         {
-            return this.ExecuteScript(script.ExecutionScript, args);
+            if (script == null)
+            {
+                throw new ArgumentNullException(nameof(script));
+            }
+
+            return this.ExecuteScript(script.MakeExecutionScript(), args);
         }
 
         /// <summary>
         /// Finds the first element in the page that matches the <see cref="By"/> object
         /// </summary>
         /// <param name="by">By mechanism to find the object</param>
         /// <returns>IWebElement object so that you can interact with that object</returns>
+        /// <exception cref="ArgumentNullException">If <paramref name="by" /> is <see langword="null"/>.</exception>
         /// <example>
         /// <code>
         /// IWebDriver driver = new InternetExplorerDriver();

dotnet/test/common/ExecutingJavascriptTest.cs

@@ -21,6 +21,7 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
+using System.Threading.Tasks;
 
 namespace OpenQA.Selenium
 {
@@ -468,6 +469,33 @@ public void ShouldBeAbleToExecuteABigChunkOfJavascriptCode()
             }
         }
 
+        [Test]
+        [IgnoreBrowser(Selenium.Browser.IE, "IE does not support Chrome DevTools Protocol")]
+        [IgnoreBrowser(Selenium.Browser.Firefox, "Firefox does not support Chrome DevTools Protocol")]
+        [IgnoreBrowser(Selenium.Browser.Safari, "Safari does not support Chrome DevTools Protocol")]
+        public async Task ShouldBeAbleToPinJavascriptCodeAndExecuteRepeatedly()
+        {
+            IJavaScriptEngine jsEngine = new JavaScriptEngine(driver);
+
+            driver.Url = xhtmlTestPage;
+
+            PinnedScript script = await jsEngine.PinScript("return document.title;");
+            for (int i = 0; i < 5; i++)
+            {
+                object result = ((IJavaScriptExecutor)driver).ExecuteScript(script);
+
+                Assert.That(result, Is.InstanceOf<string>());
+                Assert.That(result, Is.EqualTo("XHTML Test Page"));
+            }
+
+            await jsEngine.UnpinScript(script);
+
+            Assert.That(() =>
+            {
+                _ = ((IJavaScriptExecutor)driver).ExecuteScript(script);
+            }, Throws.TypeOf<JavaScriptException>());
+        }
+
         [Test]
         public void ShouldBeAbleToExecuteScriptAndReturnElementsList()
         {