wip: add higher-level db migration tests #1252

alxndrsn · 2024-10-29T09:39:54Z

TODO

run tests in CI
stop using node:assert
- use chai in tests
- replace assertIncludes() with e.g. https://www.chaijs.com/api/assert/#method_deepinclude
- replace assertEqualInAnyOrder() with e.g. https://www.chaijs.com/plugins/deep-equal-in-any-order/
remove new .eslintrc

Follow-up

describeMigration.only()
assertColumnExists()

matthew-white · 2024-10-29T15:16:57Z

test/db-migrations/mocha-setup.db-migrations.js

@@ -0,0 +1,52 @@
+global.assert = require('node:assert');


Instead of making assert global, could we use Should.js instead? That's what we use in other Backend tests. Or Chai would be good too, since that's the direction we're planning to go (#1167). I think it's totally OK for things like describeMigration() in utils.js to use assert, but I think the tests themselves should use something else.

👍 I'll migrate the assertions to chai once other issues have been resolved.

@matthew-white revisiting this, a few queries:

is this PR the appropriate place to add chai as a project dependency?

it helpful to introduce more uses of should.js when it's planned to be replaced?

is it really an issue using assert when it's a node built-in, and there is existing precedent for using it elsewhere in this codebase? Specifically:

lib/model/migrations/20190520-01-add-form-versioning.js

test/db-migrations/mocha-setup.js

test/e2e/oidc/playwright-tests/src/utils.js

test/e2e/standard/test.js

test/integration/other/form-purging.js

test/integration/other/submission-purging.js

test/unit/data/entity.js

test/unit/data/odata-filter.js

test/unit/model/frames/entity.js

is this PR the appropriate place to add chai as a project dependency?

Yes, feel free! Or you could it add to package.json in a separate PR if that feels better. We do plan to make that switch at some point.

it helpful to introduce more uses of should.js when it's planned to be replaced?

You're definitely welcome to use Should.js, since that's what we currently use. Should.js and Chai are very similar, so it wouldn't take long to replace any Should.js assertions you add with Chai ones. In many/most cases, no change will be needed, e.g., .should.equal() in Should.js is .should.equal() in Chai.

is it really an issue using assert when it's a node built-in, and there is existing precedent for using it elsewhere in this codebase? Specifically:

lib/model/migrations/20190520-01-add-form-versioning.js

test/db-migrations/mocha-setup.js

test/e2e/oidc/playwright-tests/src/utils.js

test/e2e/standard/test.js

test/integration/other/form-purging.js

test/integration/other/submission-purging.js

test/unit/data/entity.js

test/unit/data/odata-filter.js

test/unit/model/frames/entity.js

Our uses of assert have been limited, and with the exception of test/db-migrations/utils.js, I think we should try not to increase them. Almost all our Backend tests use Should.js, and our Frontend tests use Chai, which is very similar.

test/integration/ and test/unit/ just use assert for assert.throws(). I don't think there's a particular reason for that. We could use should.throws() instead.

I don't mind assert being used in files that aren't modified often, like utility functions. I just want to avoid it in actual tests that developers are more likely to read or copy from. I'm also happy with custom assertion functions being named assert*(), e.g., assertTableExists(). I didn't realize that test/e2e/oidc/playwright-tests/src/utils.js used assert, but if those are utility functions, I don't think that's an issue.

On the other hand, maybe it'd be a good idea to replace uses of assert in test/e2e/standard/test.js, since those are actual tests. I see that that file was added recently. I don't feel like there's a pressing need to make that replacement though. I'm glad we have those tests.

For many migration tests, I suspect that there won't be a need for assertions beyond those made by the assert*() functions in utils.js. For example, test/db-migrations/20241008-01-add-user_preferences.spec.js doesn't use Should.js, but it also doesn't use assert directly.

matthew-white

I have a few different questions, but overall this definitely looks like it's heading in the right direction to me. I like the assertion functions in utils.js, and I like the looks of the actual tests (20241008-01-add-user_preferences.spec.js). If we could get our migration tests looking like that going forward, that'd be a big improvement. 👍

matthew-white · 2024-11-11T05:12:45Z

test/db-migrations/migrator.js

+function loadMigrationsList() {
+  const migrations = fs.readdirSync(migrationsDir)
+    .filter(f => f.endsWith('.js'))
+    .map(f => f.replace(/\.js$/, ''))
+    .sort(); // TODO check that this is how knex sorts migration files


How about asking Knex for the list? Something like:

central-backend/test/integration/other/migrations.js

Line 21 in 3dda93f

const migrations = await migrator.migrate.list({ directory: migrationsDir });

I'd like this code to be independent of knex.

Could you say more about the benefit of that approach? knex provides convenient methods for list() and up(). I think that whatever we replace knex with will need to have similar functions. For example, we sometimes run up and down during development. By using the built-in list() method, the code here won't need to know how Knex sorts files.

Could you say more about the benefit of that approach?

Work on this PR was initiated by a document titled "Removing knex from ODK Central". Not using knex is a primary goal.

As long as we have Knex migrations in the codebase and are testing them, I think it's fair to use the knex library. Once we're ready to remove Knex entirely (perhaps not for a while), we would remove all references to knex.

That said, I also don't have strong feelings about this. Your current approach looks reasonable to me. 👍

test/db-migrations/migrator.js

test/db-migrations/utils.js

test/db-migrations/1900-test-first.spec.js

config/db-migration-test.json

test/db-migrations/.eslintrc.js

test/db-migrations/mocha-setup.db-migrations.js

test/db-migrations/utils.js

matthew-white

Feels like it's getting close!

matthew-white · 2024-11-14T19:19:33Z

.github/workflows/db-migrations.yml

+jobs:
+  standard-tests:
+    timeout-minutes: 2
+    # TODO should we use the same container as circle & central?


Any downside to using the same container?

matthew-white · 2024-11-14T19:21:09Z

.github/workflows/db-migrations.yml

+    - test/db-migrations/**
+
+jobs:
+  standard-tests:


Should this have a different name?

matthew-white · 2024-11-14T19:26:40Z

test/db-migrations/migrator.js

+// Horrible hacks.  Without this:
+//
+// 1. production migration code needs modifying, and
+// 2. it takes 3 mins+ just to run the migrations


I don't think migrations take 3 minutes to run, do they?

matthew-white · 2024-11-14T19:30:17Z

test/db-migrations/migrator.js

+
+  const previousMigration = allMigrations[idx - 1];
+
+  log('previousMigration:', previousMigration);


This logging doesn't feel super useful to me given that runIncluding() also logs.

matthew-white · 2024-11-14T19:33:46Z

test/db-migrations/migrator.js

+  }
+}
+
+function hasRun(migratioName) {


Suggested change

function hasRun(migratioName) {

function hasRun(migrationName) {

matthew-white · 2024-11-14T19:39:20Z

test/db-migrations/mocha-setup.js

+  const existingTables = await db.oneFirst(sql`SELECT COUNT(*) FROM information_schema.tables WHERE table_schema='public'`);
+  if(existingTables) {
+    console.log(`
+    Existing tables were found in the public database schema.  Reset the database before running migration tests.


In what situations will this be needed? Is the database automatically reset when tests succeed? It'd be nice to have to manually reset only rarely.

matthew-white · 2024-11-14T19:42:12Z

test/db-migrations/utils.js

+
+  assert.strictEqual(typeof describeFn, 'function');
+
+  assert.ok(migrator.exists(migrationName), `Migration '${migrationName}' already exists.`);


Migration '${migrationName}' already exists.

Won't this throw when the migration doesn't exist? Maybe I'm misunderstanding something.

matthew-white · 2024-11-14T19:43:50Z

test/db-migrations/utils.js

+
+  const runMigrationBeingTested = (() => {
+    let alreadyRun;
+    return async () => {


I don't think this function needs to be async. (I don't see await.)

alxndrsn added 8 commits October 29, 2024 08:09

initial framework

a7e34da

working test framework

fd96542

wip

17e41df

wip

f39d515

revert changes to lib

9494638

restore migrations in cleanup

3d5e337

add missing import

c8df2a7

lint

f59961d

matthew-white reviewed Oct 29, 2024

View reviewed changes

matthew-white reviewed Nov 11, 2024

View reviewed changes

Merge branch 'master' into db-migration-tests

288b03c

matthew-white mentioned this pull request Nov 12, 2024

Revisit testing database migrations #466

Open

alxndrsn added 14 commits November 13, 2024 05:26

Merge branch 'master' into db-migration-tests

cf2e099

rename file

082998f

move up log declaration

0934fbf

check has run in different way

fae9a22

ci: add workflow

23fdaa6

wip

d70880f

ci: create db

055fc63

remove big log

83b3dad

decrease test timeout

ba2470a

re-add ci

b5fcd12

Add .only() and .skip()

52add3b

lint

331e29f

Merge branch 'master' into db-migration-tests

bcf7574

re-instate soak test yml

36b7581

matthew-white reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip: add higher-level db migration tests #1252

wip: add higher-level db migration tests #1252

alxndrsn commented Oct 29, 2024 •

edited

Loading

matthew-white Oct 29, 2024

alxndrsn Nov 13, 2024

alxndrsn Nov 13, 2024

matthew-white Nov 14, 2024 •

edited

Loading

matthew-white left a comment

matthew-white Nov 11, 2024

alxndrsn Nov 11, 2024

matthew-white Nov 11, 2024

alxndrsn Nov 13, 2024

matthew-white Nov 14, 2024

matthew-white left a comment

matthew-white Nov 14, 2024

matthew-white Nov 14, 2024

matthew-white Nov 14, 2024

matthew-white Nov 14, 2024

matthew-white Nov 14, 2024

matthew-white Nov 14, 2024

matthew-white Nov 14, 2024 •

edited

Loading

matthew-white Nov 14, 2024


		const previousMigration = allMigrations[idx - 1];

		log('previousMigration:', previousMigration);

	function hasRun(migratioName) {
	function hasRun(migrationName) {


		assert.strictEqual(typeof describeFn, 'function');

		assert.ok(migrator.exists(migrationName), `Migration '${migrationName}' already exists.`);

wip: add higher-level db migration tests #1252

Are you sure you want to change the base?

wip: add higher-level db migration tests #1252

Conversation

alxndrsn commented Oct 29, 2024 • edited Loading

TODO

Follow-up

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-white Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

matthew-white 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

Choose a reason for hiding this comment

matthew-white 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-white Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxndrsn commented Oct 29, 2024 •

edited

Loading

matthew-white Nov 14, 2024 •

edited

Loading

matthew-white Nov 14, 2024 •

edited

Loading