refactor(test): improve test scenario reliability and maintainability (#3077)

* refactor(test): improve test scenario reliability and maintainability

* tests: add resp3 check test (#1)

* test: refactor connection handoff tests with enhanced spy utility (#2)

* test: add comprehensive push notification disabled scenarios (#3)

* tests: add params config tests (#4)

* tests: add feature enablement tests (#5)

---------

Co-authored-by: Nikolay Karadzhov <nikolay.karadzhov@redis.com>

Author: PavelPashov

packages/client/lib/client/enterprise-maintenance-manager.ts

@@ -51,9 +51,10 @@ interface Client {
   _pause: () => void;
   _unpause: () => void;
   _maintenanceUpdate: (update: MaintenanceUpdate) => void;
-  duplicate: (options: RedisClientOptions) => Client;
+  duplicate: () => Client;
   connect: () => Promise<Client>;
   destroy: () => void;
+  on: (event: string, callback: (value: unknown) => void) => void;
 }
 
 export default class EnterpriseMaintenanceManager {
@@ -211,21 +212,25 @@ export default class EnterpriseMaintenanceManager {
     dbgMaintenance("Creating new tmp client");
     let start = performance.now();
 
-    const tmpOptions = this.#options;
     // If the URL is provided, it takes precedense
-    if(tmpOptions.url) {
-      const u = new URL(tmpOptions.url);
+    // the options object could just be mutated
+    if(this.#options.url) {
+      const u = new URL(this.#options.url);
       u.hostname = host;
       u.port = String(port);
-      tmpOptions.url = u.toString();
+      this.#options.url = u.toString();
     } else {
-      tmpOptions.socket = {
-        ...tmpOptions.socket,
+      this.#options.socket = {
+        ...this.#options.socket,
         host,
         port
       }
     }
-    const tmpClient = this.#client.duplicate(tmpOptions);
+    const tmpClient = this.#client.duplicate();
+    tmpClient.on('error', (error: unknown) => {
+      //We dont know how to handle tmp client errors
+      dbgMaintenance(`[ERR]`, error)
+    });
     dbgMaintenance(`Tmp client created in ${( performance.now() - start ).toFixed(2)}ms`);
     dbgMaintenance(
       `Set timeout for tmp client to ${this.#options.maintRelaxedSocketTimeout}`,

packages/client/lib/tests/test-scenario/configuration.e2e.ts

@@ -0,0 +1,201 @@
+import assert from "node:assert";
+import diagnostics_channel from "node:diagnostics_channel";
+import { DiagnosticsEvent } from "../../client/enterprise-maintenance-manager";
+
+import {
+  RedisConnectionConfig,
+  createTestClient,
+  getDatabaseConfig,
+  getDatabaseConfigFromEnv,
+  getEnvConfig,
+} from "./test-scenario.util";
+import { createClient } from "../../..";
+import { FaultInjectorClient } from "./fault-injector-client";
+import { MovingEndpointType } from "../../../dist/lib/client/enterprise-maintenance-manager";
+import { RedisTcpSocketOptions } from "../../client/socket";
+
+describe("Client Configuration and Handshake", () => {
+  let clientConfig: RedisConnectionConfig;
+  let client: ReturnType<typeof createClient<any, any, any, any>>;
+  let faultInjectorClient: FaultInjectorClient;
+  let log: DiagnosticsEvent[] = [];
+
+  before(() => {
+    const envConfig = getEnvConfig();
+    const redisConfig = getDatabaseConfigFromEnv(
+      envConfig.redisEndpointsConfigPath,
+    );
+
+    faultInjectorClient = new FaultInjectorClient(envConfig.faultInjectorUrl);
+    clientConfig = getDatabaseConfig(redisConfig);
+
+    diagnostics_channel.subscribe("redis.maintenance", (event) => {
+      log.push(event as DiagnosticsEvent);
+    });
+  });
+
+  beforeEach(() => {
+    log.length = 0;
+  });
+
+  afterEach(async () => {
+    if (client && client.isOpen) {
+      await client.flushAll();
+      client.destroy();
+    }
+  });
+
+  describe("Parameter Configuration", () => {
+    const endpoints: MovingEndpointType[] = [
+      "auto",
+      // "internal-ip",
+      // "internal-fqdn",
+      "external-ip",
+      "external-fqdn",
+      "none",
+    ];
+
+    for (const endpointType of endpoints) {
+      it(`clientHandshakeWithEndpointType '${endpointType}'`, async () => {
+        try {
+          client = await createTestClient(clientConfig, {
+            maintMovingEndpointType: endpointType,
+          });
+          client.on("error", () => {});
+
+          //need to copy those because they will be mutated later
+          const oldOptions = JSON.parse(JSON.stringify(client.options));
+          assert.ok(oldOptions);
+
+          const { action_id } = await faultInjectorClient.migrateAndBindAction({
+            bdbId: clientConfig.bdbId,
+            clusterIndex: 0,
+          });
+
+          await faultInjectorClient.waitForAction(action_id);
+
+          const movingEvent = log.find((event) => event.type === "MOVING");
+          assert(!!movingEvent, "Didnt receive moving PN");
+
+          let endpoint: string | undefined;
+          try {
+            //@ts-ignore
+            endpoint = movingEvent.data.push[3];
+          } catch (err) {
+            assert(
+              false,
+              `couldnt get endpoint from event ${JSON.stringify(movingEvent)}`,
+            );
+          }
+
+          assert(endpoint !== undefined, "no endpoint");
+
+          const newOptions = client.options;
+          assert.ok(newOptions);
+
+          if (oldOptions?.url) {
+            if (endpointType === "none") {
+              assert.equal(
+                newOptions!.url,
+                oldOptions.url,
+                "For movingEndpointTpe 'none', we expect old and new url to be the same",
+              );
+            } else {
+              assert.equal(
+                newOptions.url,
+                endpoint,
+                "Expected what came through the wire to be set in the new client",
+              );
+              assert.notEqual(
+                newOptions!.url,
+                oldOptions.url,
+                `For movingEndpointTpe ${endpointType}, we expect old and new url to be different`,
+              );
+            }
+          } else {
+            const oldSocket = oldOptions.socket as RedisTcpSocketOptions;
+            const newSocket = newOptions.socket as RedisTcpSocketOptions;
+            assert.ok(oldSocket);
+            assert.ok(newSocket);
+
+            if (endpointType === "none") {
+              assert.equal(
+                newSocket.host,
+                oldSocket.host,
+                "For movingEndpointTpe 'none', we expect old and new host to be the same",
+              );
+            } else {
+              assert.equal(
+                newSocket.host + ":" + newSocket.port,
+                endpoint,
+                "Expected what came through the wire to be set in the new client",
+              );
+              assert.notEqual(
+                newSocket.host,
+                oldSocket.host,
+                `For movingEndpointTpe ${endpointType}, we expect old and new host to be different`,
+              );
+            }
+          }
+        } catch (error: any) {
+          if (
+            endpointType === "internal-fqdn" ||
+            endpointType === "internal-ip"
+          ) {
+            // errors are expected here, because we cannot connect to internal endpoints unless we are deployed in the same place as the server
+          } else {
+            assert(false, error);
+          }
+        }
+      });
+    }
+  });
+
+  describe("Feature Enablement", () => {
+    it("connectionHandshakeIncludesEnablingNotifications", async () => {
+      client = await createTestClient(clientConfig, {
+        maintPushNotifications: "enabled",
+      });
+
+      const { action_id } = await faultInjectorClient.migrateAndBindAction({
+        bdbId: clientConfig.bdbId,
+        clusterIndex: 0,
+      });
+
+      await faultInjectorClient.waitForAction(action_id);
+
+      let movingEvent = false;
+      let migratingEvent = false;
+      let migratedEvent = false;
+      for (const event of log) {
+        if (event.type === "MOVING") movingEvent = true;
+        if (event.type === "MIGRATING") migratingEvent = true;
+        if (event.type === "MIGRATED") migratedEvent = true;
+      }
+      assert.ok(movingEvent, "didnt receive MOVING PN");
+      assert.ok(migratingEvent, "didnt receive MIGRATING PN");
+      assert.ok(migratedEvent, "didnt receive MIGRATED PN");
+    });
+
+    it("disabledDontReceiveNotifications", async () => {
+      try {
+        client = await createTestClient(clientConfig, {
+          maintPushNotifications: "disabled",
+          socket: {
+            reconnectStrategy: false
+          }
+        });
+        client.on('error', console.log.bind(console))
+
+        const { action_id } = await faultInjectorClient.migrateAndBindAction({
+          bdbId: clientConfig.bdbId,
+          clusterIndex: 0,
+        });
+
+        await faultInjectorClient.waitForAction(action_id);
+
+        assert.equal(log.length, 0, "received a PN while feature is disabled");
+      } catch (error: any) { }
+    });
+  });
+});