Upgrade sequelize and improve tests #2723

tefkah · 2023-06-13T16:14:06Z

Issue(s) Resolved

Test Plan

Make all tests pass, check if pgdump produces same results.

Notes

The main change of this PR is to upgrade sequelize from v5 to v6. The motivation for this being both a routine upgrade and the (possible) typesafety improvements that come with it (more on that later).

This PR seems really big (it's probably too big, see below for suggestions on making it smaller) but most of the changed lines are from package-lock.json and indentation in the model.ts files!.

As a secondary change, this PR also adds (some) typesafety to the testing tools in stubstub, addresses some memory leaks in the tests which caused the tests to fail on my machine, and adds some minor typesafety improvements in other places.

Future work

Since this is the most important I'll mention this first.

Typesafety

This PR does not add any type inference to the sequelize model definitions, as that is not something sequelize can do (at the moment). In a separate PR i will propose a way to do this.

Incorporating #2707

I first want to make sure nothing breaks when upgrading to sequelize v6, and then I will incorporate #2707. I will do this in a separate PR, as this PR is already quite large, and doing this at the same time will obfuscate the changes.

Sequelize upgrade

The main change the upgrade to sequelize v6 induced is the deprecation of sequelize.import.
This means that rather than defining the sequelize instance in server/models.ts and then calling sequelize.import a bunch of times, we now define the sequelize instance in server/sequelize.ts, import it in every model file, and then call sequelize.define using that instance.
We then export the models from server/models.ts and the sequelize instance from server/models.ts, as before.

Result: everything still works!

Why move the sequelize instance to a different file?

I chose to instantiate the model in a different file instead of keeping it in server/models.ts, because otherwise we would be dealing with a circular dependency between server/models.ts and server/<model>/model.ts. I am not sure if this would actually lead to problems, but it seemed like a good idea to avoid it.

Why the `as any` after every model definiton?

sequelize.define does not do any type inference on its own, so without the as any every model would be defined as ModelCtor<Model<any,any>>, which would cause a bunch of type-errors. I chose to include the as any for now as that is what the models were defined as before, but I want to get rid of this as soon as possible.

Why not include #2707 here?

This PR is already wayy to big, I wanted to keep it as minimal as possible. I will include #2707 in a separate PR.

Testing tools

While doing this upgrade I ran into a lot of nasty errors when testing, and I found it hard to tell whether they were due to the upgrade or due to something else I did, or just general errors.

To gain a better understanding of how the tests were run, I added types to most of the files in stubstub. I decided not to remove them for now, because I thought it might be useful to have them in the future. If you do think this is too much to include (in this PR or in general) let me know and I can remove it!

The bugs

Database not being initialized properly

At first, because I separated the sequelize instance from server/models.ts, the database was not initialized properly when running the tests. This was because the models were not imported in the global setup, and thus when sequelize.sync() was called there it did not create any models! But, because in the setup function in stubstub/prepare.ts also does sequelize.sync(), the database was eventually populated, but only after the first test that did that setup was run, which wasn't always the first test.
As a result, depending on the order jest decides to run the tests in, the first few tests would sometimes fail because the database was not initialized properly.

To fix this, I re-export the sequelize instance from server/models.ts (where all the models are also imported/exported from), and then import the sequelize instance from there in stubstub/global/setup.ts. This way all the models are properly loaded.

I added a note that the sequelize instance should only be imported from server/models.ts to avoid this problem in the future.

Missing Feature Flags

The tests for the routes would sometimes randomly fail because the customScript feature flag wasn't set properly.

In short: normally the server/customScript/__test__/api.ts always runs pretty early in the testing suite. This test creates the customScript feature flag in the db, and so the routes (which check for this feature flag) will operate fine.

However, because of the large amount of changes in this PR, jest decided that there is a better way to run the tests sometimes (or something, no idea how jest does this), having the routes tests run before the customScript test, causing them to fail.

~~To fix this, I created a custom test sequencer that always runs the customScript test first. This is not ideal, but it works for now. Please let me know if you have a better idea!~~

Now this feature flag will be created during global setup, that way we don't need a sequencer.

Memory leaks

TL;DR: Some things were leaking memory, causing tests to fail on my machine. The main culprits are packages that monkey-patch core Node functionality such as Request and fs, and not closing connections to the database. The other seems to be an issue with Jest, and is solved by using a different runtime.

This was by far the most time consuming part of this PR: I could not run the tests without Node running out of memory. What's even weirder is that this also happened/happens on the master branch! But the tests run fine elsewhere, so I thought it was a problem with my machine. For ref, I was running the tests on an Macbook Pro M1 Max with 64GB of RAM, so I don't think it was per se a physical memory issue as CI runs on way less memory (presumably, I don't know how much memory CI has).

I've tried a lot of things to try and fix this, but here is what works:

Switching to node 16.10 ([Bug]: Memory consumption issues on Node JS 16.11.0+ jestjs/jest#11956). After 16.11, Node apparently changed how they do VMs or something, which makes Jest very bad at cleaning up after itself. This is not ideal however, as we use v18.7. While the tests do not use any Node functionality that's in 18+ but not in 16.10 (like native fetch), this would cause a problem in the future.
Adding @side/jest-runtime as the runtime. This basically halved the leaked memory from ~100mb per test to ~40mb per test. Still not ideal, but otherwise still breaks on my machine.
Upgrading to passport@0.5.0 from 0.4.0. Known issue that it leaks in Jest (Jest detects memory leak when requiring passport jaredhanson/passport#711)
Stubbing express-slow-down in tests. It leaks, but there's no fix. Since we don't test it, and it only affects /pubDocument routes, I thought it was fine to just stub it out.
Closing the sequelize connection after all tests. In setup(beforeAll), sequelize.sync() is called, which (not sure) opens up a new connection to the db, one which isn't closed. This is also an issue on master. To combat this, I added a simple sequelize.close() to teardown. This does yield some errors, but the tests still pass so 🤷
Instead of providing the full Express app to supertest, create a connection to the app and pass that. This is a known supertest issue (see Jest --detectleaks shows memory leak while using SuperTest with Express ladjs/supertest#634), although this is also fine on Node 16.10.
By passing just a connection, we can then close that connection after the tests are done.
I also slightly changed how the supertest agent is created, upgraded to latest version of supertest, removed supertest-as-promised, as supertest now provides its own promises.

All this combined allows for somewhat stable memory allocation.
Some packages, such as firebase, still leak, but upgrading that is quite the undertaking, and only a few tests use it anyway.

Other changes to testing

Upgraded Jest and ts-jest to latest version
Disabled typechecking by default for jest, as that is already done by eslint and when compiling the app
Added npm run test-no-lint to test without linting, as that is quite slow.

Package upgrades & justifications

Dependencies

Updated

sequelize: v5.22 to v6.31. The main point of this PR.
express-slow-down: v1.4 to v1.6. Originally to debug the memory leak.
passport: v0.4.0 to v0.5.0. To solve a memory leak issue

Dev dependencies

Updated

jest: v26.6 to v29.5
supertest: v4.0.2 to v.6.3.3
ts-jest: v26.4.4 to v29.1.0
typescript: v4.4.4 to v4.5.5. Fixes an issue with jest-environment-jsdom

Added

jest-environment-jsdom: No longer included in jest by default, but needed for supertest to work properly.
@side/jest-runtime: to fix the memory leaking of jest in Node 18+

Types packages added

I also took the (completely self-indulgent) liberty to add some types packages for some untyped libraries that were used.
I took care to match the version of the types package to the version of the library, and to only add types packages for libraries that were used in the codebase.

I discovered these missing types by temporarily setting noImplicitAny to true in tsconfig.json, which caused the compiler to complain about missing types.

If undesired these can be removed!

@types/request-promise: ^4.1.48,
@types/rss: ^0.0.30,
@types/sinon: ^7.5.2,
@types/supertest: ^2.0.12,
@types/unidecode: ^0.1.1,
@types/uuid: ^3.4.10,
@types/lodash.mergewith: ^4.6.7,
@types/lodash.throttle: ^4.1.7,
@types/lodash.unescape: ^4.0.7,
@types/mudder: ^1.0.0,
@types/passport: ^0.4.7,
@types/pegjs: ^0.10.3,
@types/express-session: ^1.17.7,
@types/express-slow-down: ^1.3.2,
@types/express-sslify: ^1.2.2,
@types/file-saver: ^2.0.5,
@types/filesize: ^4.2.0,
@types/fs-extra: ^8.1.2,
@types/fuzzysearch: ^1.0.0,
@types/graphlib: ^2.1.8,
@types/hasbin: 1.2,
@types/jest: ^29.5.2,
@types/jsonpath: ^0.2.0,
@types/jsonwebtoken: ^9.0.2,
@types/amqplib: ^0.5.3,
@types/color: ^3.0.3,
@types/compression: ^1.7.2,
@types/cookie-parser: ^1.4.3,
@types/cors: ^2.8.13,
@types/crypto-js: ^4.1.1,
@types/dateformat: ^3.0.1,

…slint

…stomScript tests to run first

…leak

…encies as it is no longer used in the project

tefkah · 2023-06-21T12:39:47Z

jest.config.js

+	moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'json', 'node'],
+	globalSetup: '<rootDir>/stubstub/global/setup.ts',
+	globalTeardown: '<rootDir>/stubstub/global/teardown.ts',
+	runtime: '@side/jest-runtime',


This is to lower memory consumption in node 18+

tefkah · 2023-06-21T12:40:21Z

package.json

 		"test-dev": "jest --forceExit",
+		"test-no-lint": "NODE_OPTIONS=--max-old-space-size=8192 firebase emulators:exec --only database --import .firebase/default-contents \"jest --forceExit --runInBand --logHeapUsage --silent\"",


this is to improve the speed of running tests, so you don't need to wait for lints every time

tefkah · 2023-06-21T12:40:35Z

package.json

@@ -99,7 +100,7 @@
 		"classnames": "^2.2.6",
 		"color": "^3.1.1",
 		"compression": "^1.7.4",
-		"connect-session-sequelize": "^6.0.0",
+		"connect-session-sequelize": "^7.1.7",


for compat with sequelize v6

tefkah · 2023-06-21T12:40:42Z

package.json

@@ -151,7 +152,7 @@
 		"newrelic": "^5.7.0",
 		"no-slash": "^1.2.15",
 		"node-fetch": "^2.6.7",
-		"passport": "^0.4.0",
+		"passport": "^0.5.0",


to fix memory leak

tefkah · 2023-06-21T12:40:55Z

package.json

@@ -207,7 +208,6 @@
 		"styled-components": "^5.3.0",
 		"tmp-promise": "^2.0.1",
 		"ts-node": "^9.0.0",
-		"typescript": "^4.4.4",


should go in dev deps

tefkah · 2023-06-21T12:42:12Z

server/activityItem/model.ts

in general, all the sequelize redefs look like this.
Most of the changes are indentation, which is annoying

tefkah · 2023-06-21T12:43:02Z

server/collectionPub/model.ts

-	);
-};
+	},
+) as any;


i have added "as any" for now to ensure compatibility with the rest of the codebase, as before the models were also interpreted as any

tefkah · 2023-06-21T12:43:58Z

server/community/model.ts

+		organizationId: { type: dataTypes.UUID },
+	},
+	{
+		// @ts-expect-error ts(2345): Argument of type '{ classMethods: { associate: (models: any) => void; }; }' is not assignable to parameter of type 'ModelOptions<Model<any, any>>'. Object literal may only specify known properties, and 'classMethods' does not exist in type 'ModelOptions<Model<any, any>>'.


since the sequelize define's now have proper typechecking, this raises an error, so we have to ts-expect-error it

tefkah · 2023-06-21T12:44:42Z

server/discussionAnchor/__tests__/queries.test.ts

@@ -26,6 +26,8 @@ setup(beforeAll, async () => {
 	await models.resolve();
 });

+teardown(afterAll);


ive added teardown everywhere setup is used to close the connection with sequelize and prevent a memory leak.
I've confirmed that this seems to get rid of a part of the memory leaks by running jest --detectLeaks: without the teardown it fails, with it it passes

tefkah · 2023-06-21T12:46:44Z

server/models.ts

+    TD:DR if you get "Model is not defined" errors, make sure you import the
+    `sequelize` object from this file, not form `server/sequelize.ts`.
+*/
+export { sequelize };


this re-export is here to make sure that if you import sequelize all the models have been defined already

tefkah · 2023-06-21T12:47:59Z

server/activityItem/model.ts

 		},
-		{
-			indexes: [
-				{ fields: ['communityId'], method: 'BTREE' },


this is the one change to the defines. method is not a valid property, i don't think it ever was checking the sequelize v4 and v5 docs. But I think BTREE is the default index method anywan

tefkah · 2023-06-21T12:49:05Z

server/routes/pubDocument.tsx

+				windowMs: 60000, // 1 minute for requests to be kept in memory. value of 60000ms is default but expressed here for clarity
+				delayAfter: 60, // allow 60 requests per minute, then...
+				delayMs: 100, // 60th request has a 100ms delay, 7th has a 200ms delay, 8th gets 300ms, etc.
+				maxDelayMs: 20000, // max time of request delay will be 20secs


Important: I changed maxDelay to maxDelayMs: maxDelay is not a property you can set, so did not do anything. This might cause unexpected behavior now it is actually set, although i've not noticed anything

Awesome, thanks for pointing this out. We'll keep an eye on it!

tefkah · 2023-06-21T12:51:10Z

server/routes/pubDocument.tsx

+ * While testing, this is not very useful anyway.
+ */
+const speedLimiter: RequestHandler =
+	process.env.NODE_ENV === 'test'


Disable this on test, bc it (monkey-)patches node http and therefore causes a memory leak during tests. Since we don't test for this (maybe we should) i thought it could be okay to just ignore it

tefkah · 2023-06-21T12:52:18Z

stubstub/global/setup.ts

+// carefully limit what we require from here. Luckily, all we need is server/models.ts, and
+// the handful of utilities that it must import.
+//
+// This is absurd, but it works. If you know how to fix this, please do!


the hack is still needd, but i just set it manually below. could reinstate the function wrapper if desired, probably should just remove it

tefkah · 2023-06-21T12:52:38Z

stubstub/global/setup.ts

@@ -0,0 +1,59 @@
+/* eslint-disable no-console, global-require */
+// require('ts-node').register({


this is not required, ts-jest already loads ts-node

should probably remove it

tefkah · 2023-06-21T12:52:53Z

stubstub/global/teardown.ts

+
+import { ChildProcessWithoutNullStreams } from 'child_process';
+
+declare namespace global {


just add lil typesafety.

tefkah · 2023-06-21T12:54:06Z

stubstub/modelize/builders.ts

+ * ```
+ *
+ */
+type WithOptional<


small typehelper to make typescript happy

tefkah · 2023-06-21T12:55:15Z

stubstub/modelize/builders.ts

+		: WithOptionalBase<T, K>[PossiblyNullKey];
+};
+
+export const builders = {


main change here is that this is from

const builders = {} builders.User = ...

to

const builders = { User: // }

for better type inference

tefkah · 2023-06-21T12:56:04Z

stubstub/modelize/link.ts

-	const definitionsByBoundName = {};
-	const definitions = [];
-	const resolveIdentifiersCallbacks = [];
+export type ModelDefinition = {


these types are inferred from the parser, which i did not change.
I think they are accurate!

tefkah · 2023-06-21T12:57:08Z

tools/5to6/v5/models.js

this should maybe not be changed?

3mcd

Looks great. I left a few questions and small nits. I'm going to go ahead and do some smoke testing while those are resolved. Then we can hopefully get this merged by EOD.

3mcd · 2023-06-21T13:06:13Z

custom-sequencer.js

Could we create the feature flag in the setupFiles instead of using a custom sequencer?

3mcd · 2023-06-21T13:07:28Z

jest.config.js

+		'^.+\\.[tj]sx?$': [
+			'ts-jest',
+			{
+				diagnostics: false,


Oooh, does this speed up the tests at all?

server/facets/create.ts

server/login/api.ts

server/logout/api.ts

3mcd · 2023-06-21T13:30:26Z

server/routes/pubDocument.tsx

+				windowMs: 60000, // 1 minute for requests to be kept in memory. value of 60000ms is default but expressed here for clarity
+				delayAfter: 60, // allow 60 requests per minute, then...
+				delayMs: 100, // 60th request has a 100ms delay, 7th has a 200ms delay, 8th gets 300ms, etc.
+				maxDelayMs: 20000, // max time of request delay will be 20secs


Awesome, thanks for pointing this out. We'll keep an eye on it!

workers/queue.ts

tsconfig.json

3mcd · 2023-06-21T13:38:40Z

stubstub/modelize/builders.ts

+		: WithOptionalBase<T, K>[PossiblyNullKey];
+};
+
+export const builders = {


…nually

tefkah added 29 commits May 30, 2023 18:57

feat(sequelize): create separate sequelize file

c5bdbc7

feat(sequelize): do naive migration

9b0c782

chore(sequelize): add some types packages

47d227a

chore(sequelize): add some more types packages

ba62a7f

feat(sequelize): successfully upgrade sequelize?

88cda62

chore: improve typesafety for RSS

8c3da11

chore: add more @types packages

5fe7704

fix(sequelize): selectively upsert based on collectionId when available

8d287d8

fix(sequelize): rename exported model in associate method to please e…

fc857d6

…slint

fix(sequelize): make tests run

008e3c3

Merge branch 'mastter' into upgrade-sequelize

6cf2593

jest >:(

6e623c3

fix(sequelize): fix facets and searchdata tests

774724a

fix(sequelize): remove esbuild-runner config

87bcf5f

fix(jest): get around jest's override of setimmediate

a77ce86

fix(jest): fix memory leak issue with sequelize and supertest

7cbc968

fix(jest): fix memory leak issue with passport and express-slow-down

fe39fc5

feat(jest): add custom test sequencer to sort tests and prioritize cu…

08501a0

…stomScript tests to run first

fix(jest): remove unnecessary stuff from test commands

40dbc5d

fix(jest): add @side/jest-runtime to prevent mem leak in node 18

211ae12

fix(jest): add proper teardown to tests with setup to prevent memory …

70bd906

…leak

chore(package.json): remove supertest-as-promised package from depend…

0512de0

…encies as it is no longer used in the project

chore: revert upgrades to ts, prettier, and eslint

9f5f677

chore: misc test stuff

6534eb9

fix: upgrade to typescript 4.5 to fix tsc errors in @types/jsdom

17b80b1

fix: revert eslint error

3df3bcf

fix: fix esoteric eslint issues, to be dealt with later

f4f7ca4

fix: ignore weird eslint errors for now

59c2ca1

fix: make jest exit with 0 with duplicate snapshots removed

cc6137f

pubpubBot temporarily deployed to pubpub-pipel-tfk-upgrad-hj0iuy June 14, 2023 10:41 Inactive

tefkah commented Jun 21, 2023

View reviewed changes

tools/5to6/v5/models.js Outdated

Copy link

Collaborator Author

tefkah Jun 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should maybe not be changed?

3mcd requested changes Jun 21, 2023

View reviewed changes

tefkah added 5 commits June 21, 2023 18:12

fix: remove unused code

8eb815f

fix: remove custom sequencer, initialize customScripts featureflag ma…

7dbf0d0

…nually

chore: merge with master

8aac024

chore: remove test files

fcbf692

chore: fix eslint errors

51b9bdf

pubpubBot temporarily deployed to pubpub-pipel-tfk-upgrad-ybz6hz June 21, 2023 17:57 Inactive

3mcd approved these changes Jun 21, 2023

View reviewed changes

3mcd merged commit 1bb951c into master Jun 21, 2023
1 check passed

3mcd deleted the tfk/upgrade-sequelize branch June 21, 2023 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade sequelize and improve tests #2723

Upgrade sequelize and improve tests #2723

tefkah commented Jun 13, 2023 •

edited

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023 •

edited

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

3mcd Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

3mcd Jun 21, 2023

tefkah Jun 21, 2023

tefkah Jun 21, 2023

3mcd left a comment

3mcd Jun 21, 2023

3mcd Jun 21, 2023

3mcd Jun 21, 2023

3mcd Jun 21, 2023

		"test-dev": "jest --forceExit",
		"test-no-lint": "NODE_OPTIONS=--max-old-space-size=8192 firebase emulators:exec --only database --import .firebase/default-contents \"jest --forceExit --runInBand --logHeapUsage --silent\"",

		@@ -0,0 +1,59 @@
		/* eslint-disable no-console, global-require */
		// require('ts-node').register({


		import { ChildProcessWithoutNullStreams } from 'child_process';

		declare namespace global {

Upgrade sequelize and improve tests #2723

Upgrade sequelize and improve tests #2723

Conversation

tefkah commented Jun 13, 2023 • edited

Issue(s) Resolved

Test Plan

Notes

Future work

Typesafety

Incorporating #2707

Sequelize upgrade

Why move the sequelize instance to a different file?

Why the as any after every model definiton?

Why not include #2707 here?

Testing tools

The bugs

Database not being initialized properly

Missing Feature Flags

Memory leaks

Other changes to testing

Package upgrades & justifications

Dependencies

Updated

Dev dependencies

Updated

Added

Types packages added

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tefkah Jun 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3mcd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tefkah commented Jun 13, 2023 •

edited

Why the `as any` after every model definiton?

tefkah Jun 21, 2023 •

edited