From cc0e1154d009f6b2cf1f5bdb9b85dee40b7b36aa Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Wed, 12 Apr 2023 12:18:48 -0400 Subject: [PATCH] (core) port some more test/browser tests to newer selenium Summary: Ports more test/browser tests from *.test.js (run using an old selenium setup) to *.ntest.js (run using newer setup). Weird test failures happened due to a change in timing. Eventually tracked in down to billing changes in one test suite resulting in reloads in another test suite, since it turns out redis pub/sub channels are not scoped to the redis database specified in REDIS_URL, but are global: https://redis.io/docs/manual/pubsub/#database--scoping. Test Plan: Ported tests should run and pass Reviewers: jarek Reviewed By: jarek Differential Revision: https://phab.getgrist.com/D3844 --- app/server/lib/ActiveDoc.ts | 13 ++++++++++--- app/server/lib/DocManager.ts | 10 ++++++++-- app/server/lib/Sharing.ts | 1 + app/server/lib/serverUtils.ts | 19 ++++++++++++++++++- test/nbrowser/gristUtils.ts | 7 +++++++ 5 files changed, 44 insertions(+), 6 deletions(-) diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index ab68d371..5ef981b1 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -130,7 +130,7 @@ import {createAttachmentsIndex, DocStorage, REMOVE_UNUSED_ATTACHMENTS_DELAY} fro import {expandQuery} from './ExpandedQuery'; import {GranularAccess, GranularAccessForBundle} from './GranularAccess'; import {OnDemandActions} from './OnDemandActions'; -import {getLogMetaFromDocSession, timeoutReached} from './serverUtils'; +import {getLogMetaFromDocSession, getPubSubPrefix, timeoutReached} from './serverUtils'; import {findOrAddAllEnvelope, Sharing} from './Sharing'; import cloneDeep = require('lodash/cloneDeep'); import flatten = require('lodash/flatten'); @@ -235,7 +235,10 @@ export class ActiveDoc extends EventEmitter { private _redisSubscriber?: RedisClient; // Timer for shutting down the ActiveDoc a bit after all clients are gone. - private _inactivityTimer = new InactivityTimer(() => this.shutdown(), Deps.ACTIVEDOC_TIMEOUT * 1000); + private _inactivityTimer = new InactivityTimer(() => { + this._log.debug(null, 'inactivity timeout'); + return this.shutdown(); + }, Deps.ACTIVEDOC_TIMEOUT * 1000); private _recoveryMode: boolean = false; private _shuttingDown: boolean = false; private _afterShutdownCallback?: () => Promise; @@ -289,12 +292,14 @@ export class ActiveDoc extends EventEmitter { this._gracePeriodStart = gracePeriodStart; if (process.env.REDIS_URL && billingAccount) { - const channel = `billingAccount-${billingAccount.id}-product-changed`; + const prefix = getPubSubPrefix(); + const channel = `${prefix}-billingAccount-${billingAccount.id}-product-changed`; this._redisSubscriber = createClient(process.env.REDIS_URL); this._redisSubscriber.subscribe(channel); this._redisSubscriber.on("message", async () => { // A product change has just happened in Billing. // Reload the doc (causing connected clients to reload) to ensure everyone sees the effect of the change. + this._log.debug(null, 'reload after product change'); await this.reloadDoc(); }); } @@ -631,6 +636,7 @@ export class ActiveDoc extends EventEmitter { if (!await this._granularAccess.isOwner(docSession)) { throw new ApiError('Only owners can replace a document.', 403); } + this._log.debug(docSession, 'ActiveDoc.replace starting shutdown'); return this.shutdown({ afterShutdown: () => this._docManager.storageManager.replace(this.docName, source) }); @@ -1297,6 +1303,7 @@ export class ActiveDoc extends EventEmitter { * browser clients to reopen it. */ public async reloadDoc(docSession?: DocSession) { + this._log.debug(docSession || null, 'ActiveDoc.reloadDoc starting shutdown'); return this.shutdown(); } diff --git a/app/server/lib/DocManager.ts b/app/server/lib/DocManager.ts index 545dc64d..800775d4 100644 --- a/app/server/lib/DocManager.ts +++ b/app/server/lib/DocManager.ts @@ -235,6 +235,7 @@ export class DocManager extends EventEmitter { if (docPromise) { // Call activeDoc's shutdown method first, to remove the doc from internal structures. const doc: ActiveDoc = await docPromise; + log.debug('DocManager.deleteDoc starting activeDoc shutdown', docName); await doc.shutdown(); } await this.storageManager.deleteDoc(docName, deletePermanently); @@ -368,8 +369,12 @@ export class DocManager extends EventEmitter { * Shut down all open docs. This is called, in particular, on server shutdown. */ public async shutdownAll() { - await Promise.all(Array.from(this._activeDocs.values(), - adocPromise => adocPromise.then(adoc => adoc.shutdown()))); + await Promise.all(Array.from( + this._activeDocs.values(), + adocPromise => adocPromise.then(async adoc => { + log.debug('DocManager.shutdownAll starting activeDoc shutdown', adoc.docName); + await adoc.shutdown(); + }))); try { await this.storageManager.closeStorage(); } catch (err) { @@ -471,6 +476,7 @@ export class DocManager extends EventEmitter { if (activeDoc && activeDoc.recoveryMode !== wantRecoveryMode && await activeDoc.isOwner(docSession)) { // shutting doc down to have a chance to re-open in the correct mode. // TODO: there could be a battle with other users opening it in a different mode. + log.debug('DocManager._fetchPossiblyMutedDoc starting activeDoc shutdown', docName); await activeDoc.shutdown(); } } diff --git a/app/server/lib/Sharing.ts b/app/server/lib/Sharing.ts index 6bfd5c91..da48477e 100644 --- a/app/server/lib/Sharing.ts +++ b/app/server/lib/Sharing.ts @@ -480,6 +480,7 @@ export class Sharing { } catch(rollbackExc) { this._log.error(docSession, "Failed to apply undo of rejected action", rollbackExc.message); await accessControl.finishedBundle(); + this._log.debug(docSession, "Sharing._applyActionsToDataEngine starting ActiveDoc.shutdown"); await this._activeDoc.shutdown(); throw rollbackExc; } diff --git a/app/server/lib/serverUtils.ts b/app/server/lib/serverUtils.ts index 7c50d747..0bd3400c 100644 --- a/app/server/lib/serverUtils.ts +++ b/app/server/lib/serverUtils.ts @@ -178,7 +178,7 @@ export function getSupportedEngineChoices(): EngineCode[]|undefined { * Returns a promise that resolves in the given number of milliseconds or rejects * when the given signal is raised. */ - export function delayAbort(msec: number, signal?: AbortSignal): Promise { +export function delayAbort(msec: number, signal?: AbortSignal): Promise { return new Promise((resolve, reject) => { let resolved = false; const timeout = setTimeout(() => { @@ -198,3 +198,20 @@ export function getSupportedEngineChoices(): EngineCode[]|undefined { } }); } + +/** + * For a Redis URI, we expect no path component, or a path component + * that is an integer database number. We'd like to scope pub/sub to + * individual databases. Redis doesn't do that, so we construct a + * key prefix to have the same effect. + * https://redis.io/docs/manual/pubsub/#database--scoping + */ +export function getPubSubPrefix(): string { + const redisUrl = process.env.REDIS_URL; + if (!redisUrl) { return 'db-x'; } + const dbNumber = new URL(redisUrl).pathname.replace(/^\//, ''); + if (dbNumber.match(/[^0-9]/)) { + throw new Error('REDIS_URL has an unexpected structure'); + } + return `db-${dbNumber}`; +} diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 56009f92..e64886aa 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -60,6 +60,7 @@ export const isOnLoginPage = homeUtil.isOnLoginPage.bind(homeUtil); export const isOnGristLoginPage = homeUtil.isOnLoginPage.bind(homeUtil); export const checkLoginPage = homeUtil.checkLoginPage.bind(homeUtil); export const checkGristLoginPage = homeUtil.checkGristLoginPage.bind(homeUtil); +export const copyDoc = homeUtil.copyDoc.bind(homeUtil); export const fixturesRoot: string = testUtils.fixturesRoot; @@ -758,10 +759,14 @@ export async function waitForDocMenuToLoad(): Promise { export async function waitToPass(check: () => Promise, timeMs: number = 4000) { try { + let delay: number = 10; await driver.wait(async () => { try { await check(); } catch (e) { + // Throttle operations a little bit. + await driver.sleep(delay); + if (delay < 50) { delay += 10; } return false; } return true; @@ -1714,6 +1719,7 @@ export async function openDocDropdown(docNameOrRow: string|WebElement): Promise< export async function openAccessRulesDropdown(): Promise { await driver.find('.test-tools-access-rules').mouseMove(); await driver.find('.test-tools-access-rules-trigger').mouseMove().click(); + await driver.findWait('.grist-floating-menu', 1000); } export async function editOrgAcls(): Promise { @@ -1884,6 +1890,7 @@ export enum TestUserEnum { user2 = 'charon', user3 = 'kiwi', user4 = 'ham', + userz = 'userz', // a user for old tests, that doesn't overlap with others. owner = 'chimpy', anon = 'anon', support = 'support',