From 305a1ac57dcc7a37767353febec9ef76a5e66dd8 Mon Sep 17 00:00:00 2001 From: Priyank Patel Date: Wed, 19 Aug 2020 15:35:27 +0000 Subject: [PATCH] node_tests: Don't remove require cache of module in zrequire. There is good reason to do this (explanation is bit long!). With the TypeScript migration, and the require and ES6 migrations that come with it, we use require instead of set_global which loads the entire module. Suppose we have a util module, which is used by some other module, say message_store, and util is being required in message_store since it is removed from window. Then, if a test zrequires message_store first, and then zrequires the util module qand mocks one of its methods, it will not be mocked for the message_store module. The reason is: 1. zrequire('message_store') leads to require('util'). 2. zrequire('util') removes the util module from cache and it is reloaded. Now the util module in message_store and the one in the test will be different and any updates to it in tests won't be reflected in the actual code. Which can lead to confusion for folks writing tests. I'll mention this can be avoided doing zrequire('util') first but...that is not ideal. And, since there was one outlier test that relied on this behavior, we add the namespace.reset_module function. --- .eslintrc.json | 1 + frontend_tests/node_tests/recent_topics.js | 23 +++++++++++----------- frontend_tests/zjsunit/index.js | 1 + frontend_tests/zjsunit/namespace.js | 15 ++++++++++++-- 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index fbb39318c1..64e825e1b3 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -347,6 +347,7 @@ "globals": { "assert": false, "document": false, + "reset_module": false, "set_global": false, "window": false, "with_field": false, diff --git a/frontend_tests/node_tests/recent_topics.js b/frontend_tests/node_tests/recent_topics.js index 7211328863..24b49f6223 100644 --- a/frontend_tests/node_tests/recent_topics.js +++ b/frontend_tests/node_tests/recent_topics.js @@ -293,6 +293,8 @@ function verify_topic_data(all_topics, stream, topic, last_msg_id, participated) assert.equal(topic_data.participated, participated); } +let rt = zrequire("recent_topics"); + run_test("test_recent_topics_launch", () => { // Note: unread count and urls are fake, // since they are generated in external libraries @@ -314,7 +316,7 @@ run_test("test_recent_topics_launch", () => { return ""; }); - const rt = zrequire("recent_topics"); + rt = reset_module("recent_topics"); rt.process_messages(messages); rt.launch(); @@ -351,7 +353,7 @@ run_test("test_filter_all", () => { // topic is not muted row_data = generate_topic_data([[1, "topic-1", 0, false, true]]); i = row_data.length; - const rt = zrequire("recent_topics"); + rt = reset_module("recent_topics"); rt.set_filter("all"); rt.process_messages([messages[0]]); @@ -391,7 +393,7 @@ run_test("test_filter_unread", () => { ]); let i = 0; - const rt = zrequire("recent_topics"); + rt = reset_module("recent_topics"); global.stub_templates(() => ""); rt.process_messages(messages); @@ -455,8 +457,7 @@ run_test("test_filter_participated", () => { ]); let i = 0; - const rt = zrequire("recent_topics"); - + rt = reset_module("recent_topics"); global.stub_templates(() => ""); rt.process_messages(messages); assert.equal(rt.inplace_rerender("1:topic-4"), true); @@ -495,7 +496,7 @@ run_test("test_filter_participated", () => { }); run_test("test_update_unread_count", () => { - const rt = zrequire("recent_topics"); + rt = reset_module("recent_topics"); rt.set_filter("all"); global.stub_templates(() => ""); rt.process_messages(messages); @@ -509,7 +510,7 @@ run_test("test_update_unread_count", () => { global.stub_templates(() => ""); run_test("basic assertions", () => { - const rt = zrequire("recent_topics"); + rt = reset_module("recent_topics"); rt.set_filter("all"); rt.process_messages(messages); let all_topics = rt.get(); @@ -586,7 +587,7 @@ run_test("basic assertions", () => { }); run_test("test_reify_local_echo_message", () => { - const rt = zrequire("recent_topics"); + rt = reset_module("recent_topics"); rt.set_filter("all"); rt.process_messages(messages); @@ -633,7 +634,7 @@ run_test("test_reify_local_echo_message", () => { }); run_test("test_delete_messages", () => { - const rt = zrequire("recent_topics"); + rt = reset_module("recent_topics"); rt.set_filter("all"); rt.process_messages(messages); @@ -690,7 +691,7 @@ run_test("test_topic_edit", () => { }, }); // NOTE: This test should always run in the end as it modified the messages data. - const rt = zrequire("recent_topics"); + rt = reset_module("recent_topics"); rt.set_filter("all"); rt.process_messages(messages); @@ -769,7 +770,7 @@ run_test("test_topic_edit", () => { }); run_test("test_search", () => { - const rt = zrequire("recent_topics"); + rt = reset_module("recent_topics"); assert.equal(rt.topic_in_search_results("t", "general", "Recent Topic"), true); assert.equal(rt.topic_in_search_results("T", "general", "Recent Topic"), true); assert.equal(rt.topic_in_search_results("to", "general", "Recent Topic"), true); diff --git a/frontend_tests/zjsunit/index.js b/frontend_tests/zjsunit/index.js index e5133712e8..fbc685cccd 100644 --- a/frontend_tests/zjsunit/index.js +++ b/frontend_tests/zjsunit/index.js @@ -44,6 +44,7 @@ global.with_field = namespace.with_field; global.set_global = namespace.set_global; global.patch_builtin = namespace.set_global; global.zrequire = namespace.zrequire; +global.reset_module = namespace.reset_module; global.stub_out_jquery = namespace.stub_out_jquery; global.with_overrides = namespace.with_overrides; diff --git a/frontend_tests/zjsunit/namespace.js b/frontend_tests/zjsunit/namespace.js index 119cc6d2b7..baa61022cc 100644 --- a/frontend_tests/zjsunit/namespace.js +++ b/frontend_tests/zjsunit/namespace.js @@ -17,18 +17,29 @@ exports.set_global = function (name, val) { return val; }; -exports.zrequire = function (name, fn) { +function require_path(name, fn) { if (fn === undefined) { fn = "../../static/js/" + name; } else if (/^generated\/|^js\/|^shared\/|^third\//.test(fn)) { // FIXME: Stealing part of the NPM namespace is confusing. fn = "../../static/" + fn; } - delete require.cache[require.resolve(fn)]; + + return fn; +} + +exports.zrequire = function (name, fn) { + fn = require_path(name, fn); requires.push(fn); return require(fn); }; +exports.reset_module = function (name, fn) { + fn = require_path(name, fn); + delete require.cache[require.resolve(fn)]; + return require(fn); +}; + exports.clear_zulip_refs = function () { /* This is a big hammer to make sure