From 132e67cb28ae4280db0b1c75e4c1eceedf813f4d Mon Sep 17 00:00:00 2001
From: Steve Howell <showell@zulip.com>
Date: Mon, 22 Feb 2021 12:55:34 +0000
Subject: [PATCH] zjquery: Prohibit extensions to $.fn.

We make an exception for the popovers code.

Luckily it's pretty rare that we extend $.fn
in our real code.
---
 frontend_tests/node_tests/popovers.js | 17 +++++++++----
 frontend_tests/node_tests/zjquery.js  | 23 -----------------
 frontend_tests/zjsunit/namespace.js   |  3 ++-
 frontend_tests/zjsunit/zjquery.js     | 36 ++++++++++++++++++++++++++-
 4 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/frontend_tests/node_tests/popovers.js b/frontend_tests/node_tests/popovers.js
index bbb8ecab0f..5fb606b7df 100644
--- a/frontend_tests/node_tests/popovers.js
+++ b/frontend_tests/node_tests/popovers.js
@@ -5,7 +5,7 @@ const {strict: assert} = require("assert");
 const rewiremock = require("rewiremock/node");
 
 const {stub_templates} = require("../zjsunit/handlebars");
-const {set_global, zrequire} = require("../zjsunit/namespace");
+const {set_global, with_field, zrequire} = require("../zjsunit/namespace");
 const {run_test} = require("../zjsunit/test");
 const $ = require("../zjsunit/zjquery");
 
@@ -19,7 +19,6 @@ const user_status = zrequire("user_status");
 const message_edit = zrequire("message_edit");
 
 const noop = function () {};
-$.fn.popover = noop; // this will get wrapped by our code
 
 set_global("current_msg_list", {});
 set_global("page_params", {
@@ -49,9 +48,14 @@ const stream_data = set_global("stream_data", {});
 
 const ClipboardJS = noop;
 
-const popovers = rewiremock.proxy(() => zrequire("popovers"), {
-    clipboard: ClipboardJS,
-});
+function import_popovers() {
+    return rewiremock.proxy(() => zrequire("popovers"), {
+        clipboard: ClipboardJS,
+    });
+}
+
+// Bypass some scary code that runs when we import the module.
+const popovers = with_field($.fn, "popover", noop, import_popovers);
 
 const alice = {
     email: "alice@example.com",
@@ -114,6 +118,7 @@ function test_ui(label, f) {
 
 test_ui("sender_hover", (override) => {
     override(popovers, "hide_user_profile", noop);
+    override($.fn, "popover", noop);
 
     const selection = ".sender_name, .sender_name-in-status, .inline_profile_picture";
     const handler = $("#main_div").get_on_handler("click", selection);
@@ -208,6 +213,8 @@ test_ui("sender_hover", (override) => {
 });
 
 test_ui("actions_popover", (override) => {
+    override($.fn, "popover", noop);
+
     const target = $.create("click target");
 
     override(popovers, "hide_user_profile", noop);
diff --git a/frontend_tests/node_tests/zjquery.js b/frontend_tests/node_tests/zjquery.js
index 8263656f6b..f9d1e0c0f5 100644
--- a/frontend_tests/node_tests/zjquery.js
+++ b/frontend_tests/node_tests/zjquery.js
@@ -197,26 +197,3 @@ run_test("create", () => {
     obj2.addClass(".striped");
     assert(obj2.hasClass(".striped"));
 });
-
-run_test("extensions", () => {
-    // You can extend $.fn so that all subsequent objects
-    // we create get a new function.
-
-    $.fn.area = function () {
-        return this.width() * this.height();
-    };
-
-    // Before we use area, though, let's illustrate that
-    // the predominant Zulip testing style is to stub objects
-    // using direct syntax:
-
-    const rect = $.create("rectangle");
-    rect.width = () => 5;
-    rect.height = () => 7;
-
-    assert.equal(rect.width(), 5);
-    assert.equal(rect.height(), 7);
-
-    // But we also have area available from general extension.
-    assert.equal(rect.area(), 35);
-});
diff --git a/frontend_tests/zjsunit/namespace.js b/frontend_tests/zjsunit/namespace.js
index fe54c5793a..40fff8db7e 100644
--- a/frontend_tests/zjsunit/namespace.js
+++ b/frontend_tests/zjsunit/namespace.js
@@ -75,8 +75,9 @@ exports.restore = function () {
 exports.with_field = function (obj, field, val, f) {
     const old_val = obj[field];
     obj[field] = val;
-    f();
+    const result = f();
     obj[field] = old_val;
+    return result;
 };
 
 exports.with_overrides = function (test_function) {
diff --git a/frontend_tests/zjsunit/zjquery.js b/frontend_tests/zjsunit/zjquery.js
index a5c1196db2..3e5eb03a6f 100644
--- a/frontend_tests/zjsunit/zjquery.js
+++ b/frontend_tests/zjsunit/zjquery.js
@@ -408,6 +408,7 @@ function make_zjquery() {
     const elems = new Map();
 
     // Our fn structure helps us simulate extending jQuery.
+    // Use this with extreme caution.
     const fn = {};
 
     function new_elem(selector, create_opts) {
@@ -543,7 +544,40 @@ function make_zjquery() {
         return s;
     };
 
-    zjquery.fn = fn;
+    fn.popover = () => {
+        throw new Error(`
+            Do not try to test $.fn.popover code unless
+            you really know what you are doing.
+        `);
+    };
+
+    zjquery.fn = new Proxy(fn, {
+        set(obj, prop, value) {
+            if (prop === "popover") {
+                // We allow our popovers test to modify
+                // $.fn so we can bypass a gruesome hack
+                // in our popovers.js module.
+                obj[prop] = value;
+                return true;
+            }
+
+            throw new Error(`
+                Please don't use node tests to test code
+                that extends $.fn unless you really know
+                what you are doing.
+
+                It's likely that you are better off testing
+                end-to-end behavior with puppeteer tests.
+
+                If you are trying to get coverage on a module
+                that extends $.fn, and you just want to skip
+                over that aspect of the module for the purpose
+                of testing, see if you can wrap the code
+                that extends $.fn and use override() to
+                replace the wrapper with () => {}.
+            `);
+        },
+    });
 
     zjquery.clear_all_elements = function () {
         elems.clear();