From a7e34dac0bb8d29c0325b8a1e32520dda42c9b2d Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Tue, 29 Oct 2024 08:09:03 +0000 Subject: [PATCH 01/40] initial framework --- .mocharc.js | 1 + Makefile | 4 ++ config/db-migration-test.json | 10 +++ lib/bin/create-docker-databases.js | 1 - lib/bin/run-migrations.js | 2 + test/db-migrations/1900-test-first.spec.js | 13 ++++ .../20241008-01-add-user_preferences.spec.js | 37 ++++++++++ test/db-migrations/3000-test-last.spec.js | 13 ++++ .../mocha-setup.db-migrations.js | 42 ++++++++++++ test/db-migrations/utils.js | 68 +++++++++++++++++++ 10 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 config/db-migration-test.json create mode 100644 test/db-migrations/1900-test-first.spec.js create mode 100644 test/db-migrations/20241008-01-add-user_preferences.spec.js create mode 100644 test/db-migrations/3000-test-last.spec.js create mode 100644 test/db-migrations/mocha-setup.db-migrations.js create mode 100644 test/db-migrations/utils.js diff --git a/.mocharc.js b/.mocharc.js index 6fd536d09..0752a0322 100644 --- a/.mocharc.js +++ b/.mocharc.js @@ -9,6 +9,7 @@ module.exports = { ignore: [ + 'test/db-migrations/**', 'test/e2e/**', ], }; diff --git a/Makefile b/Makefile index 40791f049..705f69b8c 100644 --- a/Makefile +++ b/Makefile @@ -87,6 +87,10 @@ test: lint test-ci: lint BCRYPT=insecure npx mocha --recursive --reporter test/ci-mocha-reporter.js +.PHONY: test-db-migrations +test-db-migrations: + NODE_CONFIG_ENV=db-migration-test npx mocha --bail --sort --require test/db-migrations/mocha-setup.db-migrations.js ./test/db-migrations/**/*.spec.js + .PHONY: test-fast test-fast: node_version BCRYPT=insecure npx mocha --recursive --fgrep @slow --invert diff --git a/config/db-migration-test.json b/config/db-migration-test.json new file mode 100644 index 000000000..29e5cd52e --- /dev/null +++ b/config/db-migration-test.json @@ -0,0 +1,10 @@ +{ + "default": { + "database": { + "host": "localhost", + "user": "jubilant", + "password": "jubilant", + "database": "jubilant_test" + } + } +} diff --git a/lib/bin/create-docker-databases.js b/lib/bin/create-docker-databases.js index 3e778a931..56ff333f9 100644 --- a/lib/bin/create-docker-databases.js +++ b/lib/bin/create-docker-databases.js @@ -25,7 +25,6 @@ const { log } = program.opts(); (async () => { const dbmain = connect('postgres'); - await dbmain.raw("create user jubilant with password 'jubilant';"); await Promise.all(['jubilant', 'jubilant_test'].map(async (database) => { await dbmain.raw(`create database ${database} with owner=jubilant encoding=UTF8;`); const dbj = connect(database); diff --git a/lib/bin/run-migrations.js b/lib/bin/run-migrations.js index 0a681ce1b..575a4b467 100644 --- a/lib/bin/run-migrations.js +++ b/lib/bin/run-migrations.js @@ -9,6 +9,8 @@ const { withDatabase, migrate } = require('../model/migrate'); +// TODO add optional arg which allows running _until a certain point_ + (async () => { try { await withDatabase(require('config').get('default.database'))(migrate); diff --git a/test/db-migrations/1900-test-first.spec.js b/test/db-migrations/1900-test-first.spec.js new file mode 100644 index 000000000..b1e800ccf --- /dev/null +++ b/test/db-migrations/1900-test-first.spec.js @@ -0,0 +1,13 @@ +// Test order is very important, so if this test fails then the whole suite may +// be doing unexpected things. + +describe('1900-test-first', () => { + after(() => { + global.firstHasBeenRun = true; + }); + + it('should be run first', () => { + // expect + assert.equal(global.firstHasBeenRun, undefined); + }); +}); diff --git a/test/db-migrations/20241008-01-add-user_preferences.spec.js b/test/db-migrations/20241008-01-add-user_preferences.spec.js new file mode 100644 index 000000000..322ff2313 --- /dev/null +++ b/test/db-migrations/20241008-01-add-user_preferences.spec.js @@ -0,0 +1,37 @@ +const { + assertTableDoesNotExist, + assertTableSchema, + describeMigration, +} = require('./utils'); + +describeMigration('20241008-01-add-user_preferences', ({ runMigrationBeingTested }) => { + before(async () => { + await assertTableDoesNotExist('user_site_preferences'); + await assertTableDoesNotExist('user_project_preferences'); + + await runMigrationBeingTested(); + }); + + it('should create user_site_preferences table', async () => { + await assertTableSchema('user_site_preferences', + { name:'userId', is_nullable:false, data_type:'integer' }, + // TODO make sure missing cols are caught + // TODO make sure all specified props exist + ); + }); + + it('should create user_site_preferences userId index', async () => { + // TODO + }); + + it('should create user_project_preferences table', async () => { + // TODO + await assertIndexExists('user_site_preferences', { + + }); + }); + + it('should create user_project_preferences userId index', async () => { + // TODO + }); +}); diff --git a/test/db-migrations/3000-test-last.spec.js b/test/db-migrations/3000-test-last.spec.js new file mode 100644 index 000000000..c28f8f705 --- /dev/null +++ b/test/db-migrations/3000-test-last.spec.js @@ -0,0 +1,13 @@ +// Test order is very important, so if this test fails then the whole suite may +// be doing unexpected things. + +describe('3000-test-last', () => { + it('should NOT be run first', () => { + // expect + assert.equal(global.firstHasBeenRun, true); + }); + + it('should be LAST run', function() { + // FIXME work out some way to test this + }); +}); diff --git a/test/db-migrations/mocha-setup.db-migrations.js b/test/db-migrations/mocha-setup.db-migrations.js new file mode 100644 index 000000000..91ca2468f --- /dev/null +++ b/test/db-migrations/mocha-setup.db-migrations.js @@ -0,0 +1,42 @@ +global.assert = require('node:assert'); +const fs = require('node:fs'); +const slonik = require('slonik'); + +const _log = level => (...args) => console.log(level, ...args); +global.log = _log('[INFO]'); + +async function mochaGlobalSetup() { + console.log('server.mochaGlobalSetup()'); + + global.assert = assert; + + global.sql = slonik.sql; + + const { user, password, host, database } = jsonFile('./config/db-migration-test.json').default.database; + const dbUrl = `postgres://${user}:${password}@${host}/${database}`; + log('dbUrl:', dbUrl); + global.db = slonik.createPool(dbUrl); + + 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. + + If you are using odk-postgres14 docker, try: + + docker exec odk-postgres14 psql -U postgres ${database} -c "DROP SCHEMA public CASCADE; CREATE SCHEMA public" + `); + process.exit(1); + } +} + +function mochaGlobalTeardown() { + console.log('server.mochaGlobalTeardown()'); + db?.end(); +} + +module.exports = { mochaGlobalSetup, mochaGlobalTeardown }; + +function jsonFile(path) { + return JSON.parse(fs.readFileSync(path, { encoding:'utf8' })); +} diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js new file mode 100644 index 000000000..c855cb9a4 --- /dev/null +++ b/test/db-migrations/utils.js @@ -0,0 +1,68 @@ +module.exports = { + assertTableDoesNotExist, + assertTableSchema, + describeMigration, +}; + +const fs = require('node:fs'); + +function describeMigration(migrationName, fn) { + const migrationFile = `./lib/model/migrations/${migrationName}.js`; + assert.ok(fs.existsSync(migrationFile), `Could not find migration file at ${migrationFile}`); + + assert.strictEqual(typeof fn, 'function'); + assert.strictEqual(fn.length, 1); + + assert.strictEqual(arguments.length, 2); + + const runMigrationBeingTested = (() => { + let alreadyRun; + return async () => { + if(alreadyRun) throw new Error('Migration has already run! Check your test structure.'); + alreadyRun = true; + await migrator.runUntil(migrationName); + }; + })(); + + return describe(`database migration: ${migrationName}`, () => fn({ runMigrationBeingTested })); +} + +async function assertTableExists(tableName) { + const count = await db.oneFirst(sql`SELECT COUNT(*) FROM information_schema.tables WHERE table_name=${tableName}`); + assert.strictEqual(count, 1, `Table not found: ${tableName}`); +} + +async function assertTableDoesNotExist(tableName) { + const count = await db.oneFirst(sql`SELECT COUNT(*) FROM information_schema.tables WHERE table_name=${tableName}`); + assert.strictEqual(count, 0, `Table should not exist: ${tableName}`); +} + +async function assertTableSchema(tableName, ...expectedCols) { + await assertTableExists(tableName); + + expectedCols.forEach((def, idx) => { + if(!def.name) throw new Error(`Expected column definition is missing required prop: .name at index ${idx}`); + }); + + const actualCols = await db.maybeOne(sql`SELECT * FROM information_schema.columns WHERE table_name=${tableName}`); + assert.notStrictEqual(actualCols, null, `Expected table not found: ${tableName}`); + + assert.deepEqual( + expectedCols.map(col => col.name), + actualCols.map(col => col.name), + 'Expected columns did not match returned columns!', + ); + + assertRowsMatch(actualCols, expectedCols); +} + +function assertRowsMatch(actual, expected) { + assert.strictEqual(actual.length, expected.length, 'row count mismatch'); + + expectedCols.forEach((expectedRow, rowIdx) => { + for(const [colName, expectedV] of Object.entries(expectedRow)) { + const actualV = actualCols[colName]; + assert.strictEqual(actualV, expectedV, `Value mismatch in row ${rowIdx}, col ${colName}`); + } + }); +} From fd965425a98d6b328d81b6c3efaf1fddd7090d51 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Tue, 29 Oct 2024 08:33:54 +0000 Subject: [PATCH 02/40] working test framework --- Makefile | 4 +- test/db-migrations/migrator.js | 44 +++++++++++++++++++ .../mocha-setup.db-migrations.js | 14 ++++-- test/db-migrations/utils.js | 10 ++++- 4 files changed, 66 insertions(+), 6 deletions(-) create mode 100644 test/db-migrations/migrator.js diff --git a/Makefile b/Makefile index 705f69b8c..d85149f0e 100644 --- a/Makefile +++ b/Makefile @@ -89,7 +89,9 @@ test-ci: lint .PHONY: test-db-migrations test-db-migrations: - NODE_CONFIG_ENV=db-migration-test npx mocha --bail --sort --require test/db-migrations/mocha-setup.db-migrations.js ./test/db-migrations/**/*.spec.js + NODE_CONFIG_ENV=db-migration-test npx mocha --bail --sort --timeout=20000 \ + --require test/db-migrations/mocha-setup.db-migrations.js \ + ./test/db-migrations/**/*.spec.js .PHONY: test-fast test-fast: node_version diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js new file mode 100644 index 000000000..df9cbcc0f --- /dev/null +++ b/test/db-migrations/migrator.js @@ -0,0 +1,44 @@ +module.exports = { + runBefore, + runIncluding, +}; + +const fs = require('node:fs'); +const { execSync } = require('node:child_process'); +const _ = require('lodash'); + +const migrationsDir = './lib/model/migrations'; +const allMigrations = fs.readdirSync(migrationsDir) + .filter(f => f.endsWith('.js')) + .map(f => f.replace(/\.js$/, '')) + .sort(); // TODO check that this is how knex sorts migration files + +log(); +log('allMigrations:'); +log(); +allMigrations.forEach(m => log('*', m)); +log(); +log('Total:', allMigrations.length); +log(); + +function runBefore(migrationName) { + const idx = allMigrations.indexOf(migrationName); + + log('runBefore()', migrationName, 'found at', idx); + + if(idx === -1) throw new Error(`Unknown migration: ${migrationName}`); + + if(idx === 0) return; + + const previousMigration = allMigrations[idx - 1]; + + log('previousMigration:', previousMigration); + + return runIncluding(previousMigration); +} + +function runIncluding(lastMigrationToRun) { + const env = { ..._.pick(process.env, 'PATH', 'NODE_CONFIG_ENV') }; + const res = execSync(`node ./lib/bin/run-migrations.js ${lastMigrationToRun}`, { env, encoding:'utf8' }); + log(`Ran migrations up-to-and-including ${lastMigrationToRun}:\n`, res); +}; diff --git a/test/db-migrations/mocha-setup.db-migrations.js b/test/db-migrations/mocha-setup.db-migrations.js index 91ca2468f..b9c2c70cd 100644 --- a/test/db-migrations/mocha-setup.db-migrations.js +++ b/test/db-migrations/mocha-setup.db-migrations.js @@ -6,7 +6,7 @@ const _log = level => (...args) => console.log(level, ...args); global.log = _log('[INFO]'); async function mochaGlobalSetup() { - console.log('server.mochaGlobalSetup()'); + log('mochaGlobalSetup() :: ENTRY'); global.assert = assert; @@ -24,15 +24,23 @@ async function mochaGlobalSetup() { If you are using odk-postgres14 docker, try: - docker exec odk-postgres14 psql -U postgres ${database} -c "DROP SCHEMA public CASCADE; CREATE SCHEMA public" + docker exec odk-postgres14 psql -U postgres ${database} -c " + DROP SCHEMA public CASCADE; + CREATE SCHEMA public; + GRANT ALL ON SCHEMA public TO postgres; + GRANT ALL ON SCHEMA public TO public; + " `); process.exit(1); } + + log('mochaGlobalSetup() :: EXIT'); } function mochaGlobalTeardown() { - console.log('server.mochaGlobalTeardown()'); + log('mochaGlobalTeardown() :: ENTRY'); db?.end(); + log('mochaGlobalTeardown() :: EXIT'); } module.exports = { mochaGlobalSetup, mochaGlobalTeardown }; diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index c855cb9a4..66cc0c025 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -5,6 +5,7 @@ module.exports = { }; const fs = require('node:fs'); +const migrator = require('./migrator'); function describeMigration(migrationName, fn) { const migrationFile = `./lib/model/migrations/${migrationName}.js`; @@ -20,11 +21,16 @@ function describeMigration(migrationName, fn) { return async () => { if(alreadyRun) throw new Error('Migration has already run! Check your test structure.'); alreadyRun = true; - await migrator.runUntil(migrationName); + migrator.runIncluding(migrationName); }; })(); - return describe(`database migration: ${migrationName}`, () => fn({ runMigrationBeingTested })); + return describe(`database migration: ${migrationName}`, () => { + before(async () => { + migrator.runBefore(migrationName); + }); + return fn({ runMigrationBeingTested }); + }); } async function assertTableExists(tableName) { From 17e41df4ddba6a40f3ccb79ac096fedb88c2742b Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Tue, 29 Oct 2024 09:27:38 +0000 Subject: [PATCH 03/40] wip --- ...0241010-01-schedule-entity-form-upgrade.js | 26 ------ test/db-migrations/.gitignore | 1 + .../20241008-01-add-user_preferences.spec.js | 11 +-- test/db-migrations/migrator.js | 88 +++++++++++++++---- test/db-migrations/utils.js | 71 +++++++++++---- 5 files changed, 133 insertions(+), 64 deletions(-) delete mode 100644 lib/model/migrations/20241010-01-schedule-entity-form-upgrade.js create mode 100644 test/db-migrations/.gitignore diff --git a/lib/model/migrations/20241010-01-schedule-entity-form-upgrade.js b/lib/model/migrations/20241010-01-schedule-entity-form-upgrade.js deleted file mode 100644 index 1aa35ac5f..000000000 --- a/lib/model/migrations/20241010-01-schedule-entity-form-upgrade.js +++ /dev/null @@ -1,26 +0,0 @@ -// Copyright 2024 ODK Central Developers -// See the NOTICE file at the top-level directory of this distribution and at -// https://github.com/getodk/central-backend/blob/master/NOTICE. -// This file is part of ODK Central. It is subject to the license terms in -// the LICENSE file found in the top-level directory of this distribution and at -// https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, -// including this file, may be copied, modified, propagated, or distributed -// except according to the terms contained in the LICENSE file. - -const up = (db) => db.raw(` - INSERT INTO audits ("action", "acteeId", "loggedAt", "details") - SELECT 'upgrade.process.form.entities_version', forms."acteeId", clock_timestamp(), - '{"upgrade": "As part of upgrading Central to v2024.3, this form is being updated to the latest entities-version spec."}' - FROM forms - JOIN form_defs fd ON forms."id" = fd."formId" - JOIN dataset_form_defs dfd ON fd."id" = dfd."formDefId" - JOIN projects ON projects."id" = forms."projectId" - WHERE dfd."actions" @> '["update"]' - AND forms."deletedAt" IS NULL - AND projects."deletedAt" IS NULL - GROUP BY forms."acteeId"; -`); - -const down = () => {}; - -module.exports = { up, down }; diff --git a/test/db-migrations/.gitignore b/test/db-migrations/.gitignore new file mode 100644 index 000000000..307c98fa3 --- /dev/null +++ b/test/db-migrations/.gitignore @@ -0,0 +1 @@ +/.holding-pen/ diff --git a/test/db-migrations/20241008-01-add-user_preferences.spec.js b/test/db-migrations/20241008-01-add-user_preferences.spec.js index 322ff2313..db1198a6f 100644 --- a/test/db-migrations/20241008-01-add-user_preferences.spec.js +++ b/test/db-migrations/20241008-01-add-user_preferences.spec.js @@ -1,4 +1,5 @@ const { + assertIndexExists, assertTableDoesNotExist, assertTableSchema, describeMigration, @@ -14,9 +15,11 @@ describeMigration('20241008-01-add-user_preferences', ({ runMigrationBeingTested it('should create user_site_preferences table', async () => { await assertTableSchema('user_site_preferences', - { name:'userId', is_nullable:false, data_type:'integer' }, - // TODO make sure missing cols are caught + { column_name:'userId', is_nullable:'NO', data_type:'integer' }, + { column_name:'propertyName', is_nullable:'NO', data_type:'integer' }, + { column_name:'propertyValue', is_nullable:'NO', data_type:'integer' }, // TODO make sure all specified props exist + // TODO make sure over-speficied props fail ); }); @@ -26,9 +29,7 @@ describeMigration('20241008-01-add-user_preferences', ({ runMigrationBeingTested it('should create user_project_preferences table', async () => { // TODO - await assertIndexExists('user_site_preferences', { - - }); + await assertIndexExists('user_site_preferences', 'CREATE INDEX "user_site_preferences_userId_idx" ON public.user_site_preferences USING btree ("userId")'); }); it('should create user_project_preferences userId index', async () => { diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index df9cbcc0f..93af29468 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -1,4 +1,5 @@ module.exports = { + exists, runBefore, runIncluding, }; @@ -7,27 +8,24 @@ const fs = require('node:fs'); const { execSync } = require('node:child_process'); const _ = require('lodash'); +// Horrible hacks. Without this: +// +// 1. production migration code needs modifying, and +// 2. it takes 3 mins+ just to run the migrations + const migrationsDir = './lib/model/migrations'; -const allMigrations = fs.readdirSync(migrationsDir) - .filter(f => f.endsWith('.js')) - .map(f => f.replace(/\.js$/, '')) - .sort(); // TODO check that this is how knex sorts migration files - -log(); -log('allMigrations:'); -log(); -allMigrations.forEach(m => log('*', m)); -log(); -log('Total:', allMigrations.length); -log(); +const holdingPen = './test/db-migrations/.holding-pen'; -function runBefore(migrationName) { - const idx = allMigrations.indexOf(migrationName); +fs.mkdirSync(holdingPen, { recursive:true }); - log('runBefore()', migrationName, 'found at', idx); +restoreMigrations(); +const allMigrations = loadMigrationsList(); +moveMigrationsToHoldingPen(); - if(idx === -1) throw new Error(`Unknown migration: ${migrationName}`); +let lastRunIdx = -1; +function runBefore(migrationName) { + const idx = getIndex(migrationName); if(idx === 0) return; const previousMigration = allMigrations[idx - 1]; @@ -38,7 +36,61 @@ function runBefore(migrationName) { } function runIncluding(lastMigrationToRun) { - const env = { ..._.pick(process.env, 'PATH', 'NODE_CONFIG_ENV') }; - const res = execSync(`node ./lib/bin/run-migrations.js ${lastMigrationToRun}`, { env, encoding:'utf8' }); + const finalIdx = getIndex(lastMigrationToRun); + + for(let restoreIdx=lastRunIdx+1; restoreIdx<=finalIdx; ++restoreIdx) { + const f = allMigrations[restoreIdx] + '.js'; + fs.renameSync(`${holdingPen}/${f}`, `${migrationsDir}/${f}`) + } + + log('Running migrations until:', lastMigrationToRun, '...'); + const res = execSync(`node ./lib/bin/run-migrations.js`, { encoding:'utf8' }); + + lastRunIdx = finalIdx; + log(`Ran migrations up-to-and-including ${lastMigrationToRun}:\n`, res); }; + +function getIndex(migrationName) { + const idx = allMigrations.indexOf(migrationName); + log('getIndex()', migrationName, 'found at', idx); + if(idx === -1) throw new Error(`Unknown migration: ${migrationName}`); + return idx; +} + +function restoreMigrations() { + moveAll(holdingPen, migrationsDir); +} + +function moveMigrationsToHoldingPen() { + moveAll(migrationsDir, holdingPen); +} + +function moveAll(src, tgt) { + fs.readdirSync(src) + .forEach(f => fs.renameSync(`${src}/${f}`, `${tgt}/${f}`)); +} + +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 + log(); + log('All migrations:'); + log(); + migrations.forEach(m => log('*', m)); + log(); + log('Total:', migrations.length); + log(); + return migrations; +} + +function exists(migrationName) { + try { + getIndex(migrationName); + return true; + } catch(err) { + return false; + } +} diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index 66cc0c025..2dc26f5a8 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -1,4 +1,5 @@ module.exports = { + assertIndexExists, assertTableDoesNotExist, assertTableSchema, describeMigration, @@ -8,8 +9,7 @@ const fs = require('node:fs'); const migrator = require('./migrator'); function describeMigration(migrationName, fn) { - const migrationFile = `./lib/model/migrations/${migrationName}.js`; - assert.ok(fs.existsSync(migrationFile), `Could not find migration file at ${migrationFile}`); + assert.ok(migrator.exists(migrationName)); assert.strictEqual(typeof fn, 'function'); assert.strictEqual(fn.length, 1); @@ -33,6 +33,17 @@ function describeMigration(migrationName, fn) { }); } +async function assertIndexExists(tableName, expected) { + if(arguments.length !== 2) throw new Error('Incorrect arg count.'); + const actualIndexes = await db.anyFirst(sql`SELECT indexdef FROM pg_indexes WHERE tablename=${tableName}`); + + if(actualIndexes.includes(expected)) return true; + assert.ok(false, 'Could not find expected index: \n' + JSON.stringify({ + expected, + actualIndexes, + }, null, 2)); +} + async function assertTableExists(tableName) { const count = await db.oneFirst(sql`SELECT COUNT(*) FROM information_schema.tables WHERE table_name=${tableName}`); assert.strictEqual(count, 1, `Table not found: ${tableName}`); @@ -47,28 +58,58 @@ async function assertTableSchema(tableName, ...expectedCols) { await assertTableExists(tableName); expectedCols.forEach((def, idx) => { - if(!def.name) throw new Error(`Expected column definition is missing required prop: .name at index ${idx}`); + if(!def.column_name) throw new Error(`Expected column definition is missing required prop: .column_name at index ${idx}`); }); - const actualCols = await db.maybeOne(sql`SELECT * FROM information_schema.columns WHERE table_name=${tableName}`); - assert.notStrictEqual(actualCols, null, `Expected table not found: ${tableName}`); + const actualCols = await db.any(sql`SELECT * FROM information_schema.columns WHERE table_name=${tableName}`); + console.log('actualCols:', actualCols); - assert.deepEqual( - expectedCols.map(col => col.name), - actualCols.map(col => col.name), + assertEqualInAnyOrder( + expectedCols.map(col => col.column_name), + actualCols.map(col => col.column_name), 'Expected columns did not match returned columns!', ); assertRowsMatch(actualCols, expectedCols); } -function assertRowsMatch(actual, expected) { - assert.strictEqual(actual.length, expected.length, 'row count mismatch'); +function assertRowsMatch(actualRows, expectedRows) { + assert.strictEqual(actualRows.length, expectedRows.length, 'row count mismatch'); + + const remainingRows = [...actualRows]; + for(let i=0; i _.pick(r, Object.keys(x))); + assert.fail(`Expected row ${i} not found in table '${tableName}':\n json=${JSON.stringify({ remainingRows, filteredRemainingRows, expectedRow:x })}`); + } + } +} + +function assertEqualInAnyOrder(a, b, message) { + if(!Array.isArray(a)) throw new Error('IllegalArgument: first arg is not an array'); + if(!Array.isArray(b)) throw new Error('IllegalArgument: second arg is not an array'); + assert.deepEqual([...a].sort(), [...b].sort(), message); +} - expectedCols.forEach((expectedRow, rowIdx) => { - for(const [colName, expectedV] of Object.entries(expectedRow)) { - const actualV = actualCols[colName]; - assert.strictEqual(actualV, expectedV, `Value mismatch in row ${rowIdx}, col ${colName}`); +function assertIncludes(actual, expected) { + for(const [k, expectedVal] of Object.entries(expected)) { + const actualVal = actual[k]; + try { + assert.deepEqual(actualVal, expectedVal); + return true; + } catch(err) { + return false; } - }); + } } From f39d515fa302de0e55218f87a7e7b6d5cd965ab3 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Tue, 29 Oct 2024 09:38:26 +0000 Subject: [PATCH 04/40] wip --- .../20241008-01-add-user_preferences.spec.js | 26 ++++++++++++------- test/db-migrations/utils.js | 17 +++++++----- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/test/db-migrations/20241008-01-add-user_preferences.spec.js b/test/db-migrations/20241008-01-add-user_preferences.spec.js index db1198a6f..c3ce33024 100644 --- a/test/db-migrations/20241008-01-add-user_preferences.spec.js +++ b/test/db-migrations/20241008-01-add-user_preferences.spec.js @@ -1,5 +1,5 @@ const { - assertIndexExists, + assertIndexExists, assertTableDoesNotExist, assertTableSchema, describeMigration, @@ -16,23 +16,31 @@ describeMigration('20241008-01-add-user_preferences', ({ runMigrationBeingTested it('should create user_site_preferences table', async () => { await assertTableSchema('user_site_preferences', { column_name:'userId', is_nullable:'NO', data_type:'integer' }, - { column_name:'propertyName', is_nullable:'NO', data_type:'integer' }, - { column_name:'propertyValue', is_nullable:'NO', data_type:'integer' }, - // TODO make sure all specified props exist - // TODO make sure over-speficied props fail + { column_name:'propertyName', is_nullable:'NO', data_type:'text' }, + { column_name:'propertyValue', is_nullable:'NO', data_type:'jsonb' }, ); }); it('should create user_site_preferences userId index', async () => { - // TODO + await assertIndexExists( + 'user_site_preferences', + 'CREATE INDEX "user_site_preferences_userId_idx" ON public.user_site_preferences USING btree ("userId")', + ); }); it('should create user_project_preferences table', async () => { - // TODO - await assertIndexExists('user_site_preferences', 'CREATE INDEX "user_site_preferences_userId_idx" ON public.user_site_preferences USING btree ("userId")'); + await assertTableSchema('user_project_preferences', + { column_name:'userId', is_nullable:'NO', data_type:'integer' }, + { column_name:'projectId', is_nullable:'NO', data_type:'integer' }, + { column_name:'propertyName', is_nullable:'NO', data_type:'text' }, + { column_name:'propertyValue', is_nullable:'NO', data_type:'jsonb' }, + ); }); it('should create user_project_preferences userId index', async () => { - // TODO + await assertIndexExists( + 'user_project_preferences', + 'CREATE INDEX "user_project_preferences_userId_idx" ON public.user_project_preferences USING btree ("userId")', + ); }); }); diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index 2dc26f5a8..c2e56d8fd 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -6,6 +6,7 @@ module.exports = { }; const fs = require('node:fs'); +const _ = require('lodash'); const migrator = require('./migrator'); function describeMigration(migrationName, fn) { @@ -38,10 +39,10 @@ async function assertIndexExists(tableName, expected) { const actualIndexes = await db.anyFirst(sql`SELECT indexdef FROM pg_indexes WHERE tablename=${tableName}`); if(actualIndexes.includes(expected)) return true; - assert.ok(false, 'Could not find expected index: \n' + JSON.stringify({ - expected, - actualIndexes, - }, null, 2)); + assert.fail( + 'Could not find expected index:\njson=' + + JSON.stringify({ expected, actualIndexes, }), + ); } async function assertTableExists(tableName) { @@ -91,7 +92,10 @@ function assertRowsMatch(actualRows, expectedRows) { } if(!found) { const filteredRemainingRows = remainingRows.map(r => _.pick(r, Object.keys(x))); - assert.fail(`Expected row ${i} not found in table '${tableName}':\n json=${JSON.stringify({ remainingRows, filteredRemainingRows, expectedRow:x })}`); + assert.fail( + `Expected row ${i} not found:\njson=` + + JSON.stringify({ remainingRows, filteredRemainingRows, expectedRow:x }), + ); } } } @@ -107,9 +111,8 @@ function assertIncludes(actual, expected) { const actualVal = actual[k]; try { assert.deepEqual(actualVal, expectedVal); - return true; } catch(err) { - return false; + assert.fail(`Could not find all properties of ${expected} in ${actual}`); } } } From 9494638f91751ca57e0eae20e28ba05b647f14b2 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Tue, 29 Oct 2024 09:40:30 +0000 Subject: [PATCH 05/40] revert changes to lib --- lib/bin/create-docker-databases.js | 1 + lib/bin/run-migrations.js | 2 -- ...0241010-01-schedule-entity-form-upgrade.js | 26 +++++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 lib/model/migrations/20241010-01-schedule-entity-form-upgrade.js diff --git a/lib/bin/create-docker-databases.js b/lib/bin/create-docker-databases.js index 56ff333f9..3e778a931 100644 --- a/lib/bin/create-docker-databases.js +++ b/lib/bin/create-docker-databases.js @@ -25,6 +25,7 @@ const { log } = program.opts(); (async () => { const dbmain = connect('postgres'); + await dbmain.raw("create user jubilant with password 'jubilant';"); await Promise.all(['jubilant', 'jubilant_test'].map(async (database) => { await dbmain.raw(`create database ${database} with owner=jubilant encoding=UTF8;`); const dbj = connect(database); diff --git a/lib/bin/run-migrations.js b/lib/bin/run-migrations.js index 575a4b467..0a681ce1b 100644 --- a/lib/bin/run-migrations.js +++ b/lib/bin/run-migrations.js @@ -9,8 +9,6 @@ const { withDatabase, migrate } = require('../model/migrate'); -// TODO add optional arg which allows running _until a certain point_ - (async () => { try { await withDatabase(require('config').get('default.database'))(migrate); diff --git a/lib/model/migrations/20241010-01-schedule-entity-form-upgrade.js b/lib/model/migrations/20241010-01-schedule-entity-form-upgrade.js new file mode 100644 index 000000000..1aa35ac5f --- /dev/null +++ b/lib/model/migrations/20241010-01-schedule-entity-form-upgrade.js @@ -0,0 +1,26 @@ +// Copyright 2024 ODK Central Developers +// See the NOTICE file at the top-level directory of this distribution and at +// https://github.com/getodk/central-backend/blob/master/NOTICE. +// This file is part of ODK Central. It is subject to the license terms in +// the LICENSE file found in the top-level directory of this distribution and at +// https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, +// including this file, may be copied, modified, propagated, or distributed +// except according to the terms contained in the LICENSE file. + +const up = (db) => db.raw(` + INSERT INTO audits ("action", "acteeId", "loggedAt", "details") + SELECT 'upgrade.process.form.entities_version', forms."acteeId", clock_timestamp(), + '{"upgrade": "As part of upgrading Central to v2024.3, this form is being updated to the latest entities-version spec."}' + FROM forms + JOIN form_defs fd ON forms."id" = fd."formId" + JOIN dataset_form_defs dfd ON fd."id" = dfd."formDefId" + JOIN projects ON projects."id" = forms."projectId" + WHERE dfd."actions" @> '["update"]' + AND forms."deletedAt" IS NULL + AND projects."deletedAt" IS NULL + GROUP BY forms."acteeId"; +`); + +const down = () => {}; + +module.exports = { up, down }; From 3d5e337d5c278dd5175e88b221053a2f2e439d36 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Tue, 29 Oct 2024 09:41:55 +0000 Subject: [PATCH 06/40] restore migrations in cleanup --- test/db-migrations/migrator.js | 1 + test/db-migrations/mocha-setup.db-migrations.js | 1 + 2 files changed, 2 insertions(+) diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index 93af29468..7fe5b6453 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -2,6 +2,7 @@ module.exports = { exists, runBefore, runIncluding, + restoreMigrations, }; const fs = require('node:fs'); diff --git a/test/db-migrations/mocha-setup.db-migrations.js b/test/db-migrations/mocha-setup.db-migrations.js index b9c2c70cd..ab4880ffa 100644 --- a/test/db-migrations/mocha-setup.db-migrations.js +++ b/test/db-migrations/mocha-setup.db-migrations.js @@ -40,6 +40,7 @@ async function mochaGlobalSetup() { function mochaGlobalTeardown() { log('mochaGlobalTeardown() :: ENTRY'); db?.end(); + migrator.restoreMigrations(); log('mochaGlobalTeardown() :: EXIT'); } From c8df2a7f0e9abb5c458fc75f1eecb1202b9595b5 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Tue, 29 Oct 2024 09:42:27 +0000 Subject: [PATCH 07/40] add missing import --- test/db-migrations/mocha-setup.db-migrations.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/db-migrations/mocha-setup.db-migrations.js b/test/db-migrations/mocha-setup.db-migrations.js index ab4880ffa..ddf128ffa 100644 --- a/test/db-migrations/mocha-setup.db-migrations.js +++ b/test/db-migrations/mocha-setup.db-migrations.js @@ -1,6 +1,7 @@ global.assert = require('node:assert'); const fs = require('node:fs'); const slonik = require('slonik'); +const migrator = require('./migrator'); const _log = level => (...args) => console.log(level, ...args); global.log = _log('[INFO]'); From f59961ddb7f110f9ecc217737dd6e4336e4a3803 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Tue, 29 Oct 2024 09:47:41 +0000 Subject: [PATCH 08/40] lint --- test/db-migrations/.eslintrc.js | 19 +++++++++++++++++++ .../20241008-01-add-user_preferences.spec.js | 2 +- test/db-migrations/migrator.js | 11 +++++------ test/db-migrations/utils.js | 11 +++++------ 4 files changed, 30 insertions(+), 13 deletions(-) create mode 100644 test/db-migrations/.eslintrc.js diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js new file mode 100644 index 000000000..1c8be7708 --- /dev/null +++ b/test/db-migrations/.eslintrc.js @@ -0,0 +1,19 @@ +module.exports = { + extends: '../.eslintrc.js', + rules: { + 'key-spacing': 'off', + 'keyword-spacing': 'off', + 'no-console': 'off', + 'no-multi-spaces': 'off', + 'no-plusplus': 'off', + 'no-use-before-define': 'off', + 'object-curly-newline': 'off', + 'prefer-arrow-callback': 'off', + }, + globals: { + assert: false, + db: false, + log: false, + sql: false, + }, +}; diff --git a/test/db-migrations/20241008-01-add-user_preferences.spec.js b/test/db-migrations/20241008-01-add-user_preferences.spec.js index c3ce33024..976fc8eaa 100644 --- a/test/db-migrations/20241008-01-add-user_preferences.spec.js +++ b/test/db-migrations/20241008-01-add-user_preferences.spec.js @@ -1,4 +1,4 @@ -const { +const { assertIndexExists, assertTableDoesNotExist, assertTableSchema, diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index 7fe5b6453..91d3ed70c 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -7,7 +7,6 @@ module.exports = { const fs = require('node:fs'); const { execSync } = require('node:child_process'); -const _ = require('lodash'); // Horrible hacks. Without this: // @@ -41,7 +40,7 @@ function runIncluding(lastMigrationToRun) { for(let restoreIdx=lastRunIdx+1; restoreIdx<=finalIdx; ++restoreIdx) { const f = allMigrations[restoreIdx] + '.js'; - fs.renameSync(`${holdingPen}/${f}`, `${migrationsDir}/${f}`) + fs.renameSync(`${holdingPen}/${f}`, `${migrationsDir}/${f}`); } log('Running migrations until:', lastMigrationToRun, '...'); @@ -50,7 +49,7 @@ function runIncluding(lastMigrationToRun) { lastRunIdx = finalIdx; log(`Ran migrations up-to-and-including ${lastMigrationToRun}:\n`, res); -}; +} function getIndex(migrationName) { const idx = allMigrations.indexOf(migrationName); @@ -74,9 +73,9 @@ function moveAll(src, tgt) { 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 + .filter(f => f.endsWith('.js')) + .map(f => f.replace(/\.js$/, '')) + .sort(); // TODO check that this is how knex sorts migration files log(); log('All migrations:'); log(); diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index c2e56d8fd..a9bf30fa0 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -5,7 +5,6 @@ module.exports = { describeMigration, }; -const fs = require('node:fs'); const _ = require('lodash'); const migrator = require('./migrator'); @@ -84,20 +83,20 @@ function assertRowsMatch(actualRows, expectedRows) { for(let j=0; j _.pick(r, Object.keys(x))); assert.fail( `Expected row ${i} not found:\njson=` + JSON.stringify({ remainingRows, filteredRemainingRows, expectedRow:x }), ); - } - } + } + } } function assertEqualInAnyOrder(a, b, message) { From 082998fc87939dcbb074efa4ea547860cd7a8a9b Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 05:27:37 +0000 Subject: [PATCH 09/40] rename file --- Makefile | 2 +- .../{mocha-setup.db-migrations.js => mocha-setup.js} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename test/db-migrations/{mocha-setup.db-migrations.js => mocha-setup.js} (100%) diff --git a/Makefile b/Makefile index fcf15a5cc..3abde822b 100644 --- a/Makefile +++ b/Makefile @@ -110,7 +110,7 @@ test-ci: lint .PHONY: test-db-migrations test-db-migrations: NODE_CONFIG_ENV=db-migration-test npx mocha --bail --sort --timeout=20000 \ - --require test/db-migrations/mocha-setup.db-migrations.js \ + --require test/db-migrations/mocha-setup.js \ ./test/db-migrations/**/*.spec.js .PHONY: test-fast diff --git a/test/db-migrations/mocha-setup.db-migrations.js b/test/db-migrations/mocha-setup.js similarity index 100% rename from test/db-migrations/mocha-setup.db-migrations.js rename to test/db-migrations/mocha-setup.js From 0934fbfeb54ef30416eceb0c24a59d89e8bda954 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 05:34:53 +0000 Subject: [PATCH 10/40] move up log declaration --- test/db-migrations/mocha-setup.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/db-migrations/mocha-setup.js b/test/db-migrations/mocha-setup.js index ddf128ffa..643fd05c5 100644 --- a/test/db-migrations/mocha-setup.js +++ b/test/db-migrations/mocha-setup.js @@ -1,11 +1,11 @@ +const _log = level => (...args) => console.log(level, ...args); +global.log = _log('[INFO]'); + global.assert = require('node:assert'); const fs = require('node:fs'); const slonik = require('slonik'); const migrator = require('./migrator'); -const _log = level => (...args) => console.log(level, ...args); -global.log = _log('[INFO]'); - async function mochaGlobalSetup() { log('mochaGlobalSetup() :: ENTRY'); From fae9a22837cc31676ffb0d62d6a23139ea5dd748 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 06:13:54 +0000 Subject: [PATCH 11/40] check has run in different way --- test/db-migrations/1900-test-first.spec.js | 13 ------------- test/db-migrations/3000-test-last.spec.js | 13 ------------- test/db-migrations/migrator.js | 5 +++++ test/db-migrations/utils.js | 3 ++- 4 files changed, 7 insertions(+), 27 deletions(-) delete mode 100644 test/db-migrations/1900-test-first.spec.js delete mode 100644 test/db-migrations/3000-test-last.spec.js diff --git a/test/db-migrations/1900-test-first.spec.js b/test/db-migrations/1900-test-first.spec.js deleted file mode 100644 index b1e800ccf..000000000 --- a/test/db-migrations/1900-test-first.spec.js +++ /dev/null @@ -1,13 +0,0 @@ -// Test order is very important, so if this test fails then the whole suite may -// be doing unexpected things. - -describe('1900-test-first', () => { - after(() => { - global.firstHasBeenRun = true; - }); - - it('should be run first', () => { - // expect - assert.equal(global.firstHasBeenRun, undefined); - }); -}); diff --git a/test/db-migrations/3000-test-last.spec.js b/test/db-migrations/3000-test-last.spec.js deleted file mode 100644 index c28f8f705..000000000 --- a/test/db-migrations/3000-test-last.spec.js +++ /dev/null @@ -1,13 +0,0 @@ -// Test order is very important, so if this test fails then the whole suite may -// be doing unexpected things. - -describe('3000-test-last', () => { - it('should NOT be run first', () => { - // expect - assert.equal(global.firstHasBeenRun, true); - }); - - it('should be LAST run', function() { - // FIXME work out some way to test this - }); -}); diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index 91d3ed70c..91856787c 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -1,5 +1,6 @@ module.exports = { exists, + hasRun, runBefore, runIncluding, restoreMigrations, @@ -94,3 +95,7 @@ function exists(migrationName) { return false; } } + +function hasRun(migratioName) { + return lastRunIdx >= getIndex(migratioName); +} diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index a9bf30fa0..ecd606615 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -9,7 +9,8 @@ const _ = require('lodash'); const migrator = require('./migrator'); function describeMigration(migrationName, fn) { - assert.ok(migrator.exists(migrationName)); + assert.ok(migrator.exists(migrationName), `Migration '${migrationName}' already exists.`); + assert.ok(!migrator.hasRun(migrationName), `Migration '${migrationName}' has already been run.`); assert.strictEqual(typeof fn, 'function'); assert.strictEqual(fn.length, 1); From 23fdaa67b1f857fb081483cbd5dfdb72d84ee4cf Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 06:16:53 +0000 Subject: [PATCH 12/40] ci: add workflow --- .github/workflows/db-migrations.yml | 36 +++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 .github/workflows/db-migrations.yml diff --git a/.github/workflows/db-migrations.yml b/.github/workflows/db-migrations.yml new file mode 100644 index 000000000..be6f67104 --- /dev/null +++ b/.github/workflows/db-migrations.yml @@ -0,0 +1,36 @@ +name: Database Migrations + +on: + push: + paths: + - lib/model/migrations/** + - test/db-migrations/** + +jobs: + standard-tests: + timeout-minutes: 20 + # TODO should we use the same container as circle & central? + runs-on: ubuntu-latest + services: + # see: https://docs.github.com/en/enterprise-server@3.5/actions/using-containerized-services/creating-postgresql-service-containers + postgres: + image: postgres:14.10 + env: + POSTGRES_PASSWORD: odktest + ports: + - 5432:5432 + # Set health checks to wait until postgres has started + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + steps: + - uses: actions/checkout@v4 + - name: Use Node.js 20 + uses: actions/setup-node@v4 + with: + node-version: 20.17.0 + cache: 'npm' + - run: npm ci --legacy-peer-deps + - run: make test-db-migrations From d70880f5813770a157f9b325a738574833ff8154 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 06:23:21 +0000 Subject: [PATCH 13/40] wip --- .github/workflows/db-migrations.yml | 5 ++- .github/workflows/oidc-e2e.yml | 41 -------------------- .github/workflows/oidc-integration.yml | 37 ------------------ .github/workflows/s3-e2e.yml | 53 -------------------------- .github/workflows/soak-test.yml | 38 ------------------ 5 files changed, 3 insertions(+), 171 deletions(-) delete mode 100644 .github/workflows/oidc-e2e.yml delete mode 100644 .github/workflows/oidc-integration.yml delete mode 100644 .github/workflows/s3-e2e.yml delete mode 100644 .github/workflows/soak-test.yml diff --git a/.github/workflows/db-migrations.yml b/.github/workflows/db-migrations.yml index be6f67104..bf79f2987 100644 --- a/.github/workflows/db-migrations.yml +++ b/.github/workflows/db-migrations.yml @@ -3,8 +3,9 @@ name: Database Migrations on: push: paths: - - lib/model/migrations/** - - test/db-migrations/** + - .github/workflows/db-migrations.yml + - lib/model/migrations/** + - test/db-migrations/** jobs: standard-tests: diff --git a/.github/workflows/oidc-e2e.yml b/.github/workflows/oidc-e2e.yml deleted file mode 100644 index e4093862b..000000000 --- a/.github/workflows/oidc-e2e.yml +++ /dev/null @@ -1,41 +0,0 @@ -name: OIDC e2e tests - -on: push - -env: - DEBUG: pw:api - ODK_PLAYWRIGHT_BROWSERS: chromium,firefox,webkit - -jobs: - oidc-e2e-test: - timeout-minutes: 6 - # TODO should we use the same container as circle & central? - runs-on: ubuntu-latest - services: - # see: https://docs.github.com/en/enterprise-server@3.5/actions/using-containerized-services/creating-postgresql-service-containers - postgres: - image: postgres:14.10 - env: - POSTGRES_PASSWORD: odktest - ports: - - 5432:5432 - # Set health checks to wait until postgres has started - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - steps: - - uses: actions/checkout@v4 - - name: Use Node.js 20 - uses: actions/setup-node@v4 - with: - node-version: 20.17.0 - cache: 'npm' - - run: make test-oidc-e2e - - name: Archive playwright screenshots - if: failure() - uses: actions/upload-artifact@v4 - with: - name: Playwright Screenshots - path: test/e2e/oidc/playwright-results/**/*.png diff --git a/.github/workflows/oidc-integration.yml b/.github/workflows/oidc-integration.yml deleted file mode 100644 index f04d2d487..000000000 --- a/.github/workflows/oidc-integration.yml +++ /dev/null @@ -1,37 +0,0 @@ -name: OIDC integration tests - -on: push - -jobs: - oidc-integration-test: - timeout-minutes: 6 - # TODO should we use the same container as circle & central? - runs-on: ubuntu-latest - services: - # see: https://docs.github.com/en/enterprise-server@3.5/actions/using-containerized-services/creating-postgresql-service-containers - postgres: - image: postgres:14.10 - env: - POSTGRES_PASSWORD: odktest - ports: - - 5432:5432 - # Set health checks to wait until postgres has started - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - steps: - - uses: actions/checkout@v4 - - name: Use Node.js 20 - uses: actions/setup-node@v4 - with: - node-version: 20.17.0 - cache: 'npm' - - run: npm ci --legacy-peer-deps - - run: FAKE_OIDC_ROOT_URL=http://localhost:9898 make fake-oidc-server-ci > fake-oidc-server.log & - - run: node lib/bin/create-docker-databases.js - - run: make test-oidc-integration - - name: Fake OIDC Server Logs - if: always() - run: "! [[ -f ./fake-oidc-server.log ]] || cat ./fake-oidc-server.log" diff --git a/.github/workflows/s3-e2e.yml b/.github/workflows/s3-e2e.yml deleted file mode 100644 index 7db6e4cd8..000000000 --- a/.github/workflows/s3-e2e.yml +++ /dev/null @@ -1,53 +0,0 @@ -name: S3 E2E Tests - -on: push - -jobs: - s3-e2e: - timeout-minutes: 15 - # TODO should we use the same container as circle & central? - runs-on: ubuntu-latest - services: - # see: https://docs.github.com/en/enterprise-server@3.5/actions/using-containerized-services/creating-postgresql-service-containers - postgres: - image: postgres:14.10 - env: - POSTGRES_PASSWORD: odktest - ports: - - 5432:5432 - # Set health checks to wait until postgres has started - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - minio: - # see: https://github.com/minio/minio/discussions/16099 - image: minio/minio:edge-cicd - env: - MINIO_ROOT_USER: odk-central-dev - MINIO_ROOT_PASSWORD: topSecret123 - # Enable encryption - this changes how s3 ETags work - # See: https://docs.aws.amazon.com/AmazonS3/latest/API/API_Object.html - # See: https://github.com/minio/minio/discussions/19012 - MINIO_KMS_AUTO_ENCRYPTION: on - MINIO_KMS_SECRET_KEY: odk-minio-test-key:QfdUCrn3UQ58W5pqCS5SX4SOlec9sT8yb4rZ4zK24w0= - ports: - - 9000:9000 - options: >- - --health-cmd "curl -s http://localhost:9000/minio/health/live" - --health-interval 10s - --health-timeout 5s - --health-retries 5 - steps: - - uses: actions/checkout@v4 - - name: Use Node.js 20 - uses: actions/setup-node@v4 - with: - node-version: 20.17.0 - cache: 'npm' - - run: npm ci --legacy-peer-deps - - run: node lib/bin/create-docker-databases.js - - name: E2E Test - timeout-minutes: 10 - run: ./test/e2e/s3/run-tests.sh diff --git a/.github/workflows/soak-test.yml b/.github/workflows/soak-test.yml deleted file mode 100644 index 8fe92c02b..000000000 --- a/.github/workflows/soak-test.yml +++ /dev/null @@ -1,38 +0,0 @@ -name: Soak Test - -on: push - -jobs: - soak-test: - timeout-minutes: 15 - # TODO should we use the same container as circle & central? - runs-on: ubuntu-latest - services: - # see: https://docs.github.com/en/enterprise-server@3.5/actions/using-containerized-services/creating-postgresql-service-containers - postgres: - image: postgres:14.10 - env: - POSTGRES_PASSWORD: odktest - ports: - - 5432:5432 - # Set health checks to wait until postgres has started - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - steps: - - uses: actions/checkout@v4 - - name: Use Node.js 20 - uses: actions/setup-node@v4 - with: - node-version: 20.17.0 - cache: 'npm' - - run: npm ci --legacy-peer-deps - - run: node lib/bin/create-docker-databases.js - - name: Soak Test - timeout-minutes: 10 - run: ./test/e2e/soak/ci - - name: Backend Logs - if: always() - run: "! [[ -f ./backend.log ]] || cat ./backend.log" From 055fc63428fc5c9b3a9ca926016c4146ae303311 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 06:26:52 +0000 Subject: [PATCH 14/40] ci: create db --- .github/workflows/db-migrations.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/db-migrations.yml b/.github/workflows/db-migrations.yml index bf79f2987..de0dc914f 100644 --- a/.github/workflows/db-migrations.yml +++ b/.github/workflows/db-migrations.yml @@ -34,4 +34,5 @@ jobs: node-version: 20.17.0 cache: 'npm' - run: npm ci --legacy-peer-deps + - run: node lib/bin/create-docker-databases.js - run: make test-db-migrations From 83b3dadb0a3065c389f17e3809ed97e6c6e8b1f7 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 06:29:45 +0000 Subject: [PATCH 15/40] remove big log --- test/db-migrations/utils.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index ecd606615..d273677bf 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -63,7 +63,6 @@ async function assertTableSchema(tableName, ...expectedCols) { }); const actualCols = await db.any(sql`SELECT * FROM information_schema.columns WHERE table_name=${tableName}`); - console.log('actualCols:', actualCols); assertEqualInAnyOrder( expectedCols.map(col => col.column_name), From ba2470ac91cc95824f483f8aa17486098bf95043 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 06:31:00 +0000 Subject: [PATCH 16/40] decrease test timeout --- .github/workflows/db-migrations.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/db-migrations.yml b/.github/workflows/db-migrations.yml index de0dc914f..dc00cb618 100644 --- a/.github/workflows/db-migrations.yml +++ b/.github/workflows/db-migrations.yml @@ -9,7 +9,7 @@ on: jobs: standard-tests: - timeout-minutes: 20 + timeout-minutes: 2 # TODO should we use the same container as circle & central? runs-on: ubuntu-latest services: From b5fcd121b8617449381dd7e252735748230754cf Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 06:31:31 +0000 Subject: [PATCH 17/40] re-add ci --- .github/workflows/oidc-e2e.yml | 41 ++++++++++++++++++++ .github/workflows/oidc-integration.yml | 37 ++++++++++++++++++ .github/workflows/s3-e2e.yml | 53 ++++++++++++++++++++++++++ 3 files changed, 131 insertions(+) create mode 100644 .github/workflows/oidc-e2e.yml create mode 100644 .github/workflows/oidc-integration.yml create mode 100644 .github/workflows/s3-e2e.yml diff --git a/.github/workflows/oidc-e2e.yml b/.github/workflows/oidc-e2e.yml new file mode 100644 index 000000000..e4093862b --- /dev/null +++ b/.github/workflows/oidc-e2e.yml @@ -0,0 +1,41 @@ +name: OIDC e2e tests + +on: push + +env: + DEBUG: pw:api + ODK_PLAYWRIGHT_BROWSERS: chromium,firefox,webkit + +jobs: + oidc-e2e-test: + timeout-minutes: 6 + # TODO should we use the same container as circle & central? + runs-on: ubuntu-latest + services: + # see: https://docs.github.com/en/enterprise-server@3.5/actions/using-containerized-services/creating-postgresql-service-containers + postgres: + image: postgres:14.10 + env: + POSTGRES_PASSWORD: odktest + ports: + - 5432:5432 + # Set health checks to wait until postgres has started + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + steps: + - uses: actions/checkout@v4 + - name: Use Node.js 20 + uses: actions/setup-node@v4 + with: + node-version: 20.17.0 + cache: 'npm' + - run: make test-oidc-e2e + - name: Archive playwright screenshots + if: failure() + uses: actions/upload-artifact@v4 + with: + name: Playwright Screenshots + path: test/e2e/oidc/playwright-results/**/*.png diff --git a/.github/workflows/oidc-integration.yml b/.github/workflows/oidc-integration.yml new file mode 100644 index 000000000..f04d2d487 --- /dev/null +++ b/.github/workflows/oidc-integration.yml @@ -0,0 +1,37 @@ +name: OIDC integration tests + +on: push + +jobs: + oidc-integration-test: + timeout-minutes: 6 + # TODO should we use the same container as circle & central? + runs-on: ubuntu-latest + services: + # see: https://docs.github.com/en/enterprise-server@3.5/actions/using-containerized-services/creating-postgresql-service-containers + postgres: + image: postgres:14.10 + env: + POSTGRES_PASSWORD: odktest + ports: + - 5432:5432 + # Set health checks to wait until postgres has started + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + steps: + - uses: actions/checkout@v4 + - name: Use Node.js 20 + uses: actions/setup-node@v4 + with: + node-version: 20.17.0 + cache: 'npm' + - run: npm ci --legacy-peer-deps + - run: FAKE_OIDC_ROOT_URL=http://localhost:9898 make fake-oidc-server-ci > fake-oidc-server.log & + - run: node lib/bin/create-docker-databases.js + - run: make test-oidc-integration + - name: Fake OIDC Server Logs + if: always() + run: "! [[ -f ./fake-oidc-server.log ]] || cat ./fake-oidc-server.log" diff --git a/.github/workflows/s3-e2e.yml b/.github/workflows/s3-e2e.yml new file mode 100644 index 000000000..7db6e4cd8 --- /dev/null +++ b/.github/workflows/s3-e2e.yml @@ -0,0 +1,53 @@ +name: S3 E2E Tests + +on: push + +jobs: + s3-e2e: + timeout-minutes: 15 + # TODO should we use the same container as circle & central? + runs-on: ubuntu-latest + services: + # see: https://docs.github.com/en/enterprise-server@3.5/actions/using-containerized-services/creating-postgresql-service-containers + postgres: + image: postgres:14.10 + env: + POSTGRES_PASSWORD: odktest + ports: + - 5432:5432 + # Set health checks to wait until postgres has started + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + minio: + # see: https://github.com/minio/minio/discussions/16099 + image: minio/minio:edge-cicd + env: + MINIO_ROOT_USER: odk-central-dev + MINIO_ROOT_PASSWORD: topSecret123 + # Enable encryption - this changes how s3 ETags work + # See: https://docs.aws.amazon.com/AmazonS3/latest/API/API_Object.html + # See: https://github.com/minio/minio/discussions/19012 + MINIO_KMS_AUTO_ENCRYPTION: on + MINIO_KMS_SECRET_KEY: odk-minio-test-key:QfdUCrn3UQ58W5pqCS5SX4SOlec9sT8yb4rZ4zK24w0= + ports: + - 9000:9000 + options: >- + --health-cmd "curl -s http://localhost:9000/minio/health/live" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + steps: + - uses: actions/checkout@v4 + - name: Use Node.js 20 + uses: actions/setup-node@v4 + with: + node-version: 20.17.0 + cache: 'npm' + - run: npm ci --legacy-peer-deps + - run: node lib/bin/create-docker-databases.js + - name: E2E Test + timeout-minutes: 10 + run: ./test/e2e/s3/run-tests.sh From 52add3bb571cc6d6b4ba467300d89a44e37d7348 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 06:51:11 +0000 Subject: [PATCH 18/40] Add .only() and .skip() --- test/db-migrations/utils.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index d273677bf..3f2c8ceae 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -8,15 +8,17 @@ module.exports = { const _ = require('lodash'); const migrator = require('./migrator'); -function describeMigration(migrationName, fn) { +function _describeMigration(describeFn, migrationName, fn) { + assert.strictEqual(arguments.length, 3, 'Incorrect argument count.'); + + assert.strictEqual(typeof describeFn, 'function'); + assert.ok(migrator.exists(migrationName), `Migration '${migrationName}' already exists.`); assert.ok(!migrator.hasRun(migrationName), `Migration '${migrationName}' has already been run.`); assert.strictEqual(typeof fn, 'function'); assert.strictEqual(fn.length, 1); - assert.strictEqual(arguments.length, 2); - const runMigrationBeingTested = (() => { let alreadyRun; return async () => { @@ -26,13 +28,16 @@ function describeMigration(migrationName, fn) { }; })(); - return describe(`database migration: ${migrationName}`, () => { + return describeFn(`database migration: ${migrationName}`, () => { before(async () => { migrator.runBefore(migrationName); }); return fn({ runMigrationBeingTested }); }); } +function describeMigration(...args) { return _describeMigration(describe, ...args); } +describeMigration.only = (...args) => _describeMigration(describe.only, ...args); +describeMigration.skip = (...args) => _describeMigration(describe.skip, ...args); async function assertIndexExists(tableName, expected) { if(arguments.length !== 2) throw new Error('Incorrect arg count.'); From 331e29f0e76ab665b11bd5825ad5bb5a8fb2d8b9 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 13 Nov 2024 06:58:50 +0000 Subject: [PATCH 19/40] lint --- test/db-migrations/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index 3f2c8ceae..1e4ce510e 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -36,7 +36,7 @@ function _describeMigration(describeFn, migrationName, fn) { }); } function describeMigration(...args) { return _describeMigration(describe, ...args); } -describeMigration.only = (...args) => _describeMigration(describe.only, ...args); +describeMigration.only = (...args) => _describeMigration(describe.only, ...args); // eslint-disable-line no-only-tests/no-only-tests describeMigration.skip = (...args) => _describeMigration(describe.skip, ...args); async function assertIndexExists(tableName, expected) { From 36b75810a2a595ef8ca94ee8126f34b4b1fb51ad Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Thu, 14 Nov 2024 05:35:27 +0000 Subject: [PATCH 20/40] re-instate soak test yml --- .github/workflows/soak-test.yml | 38 +++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 .github/workflows/soak-test.yml diff --git a/.github/workflows/soak-test.yml b/.github/workflows/soak-test.yml new file mode 100644 index 000000000..f07cb96ad --- /dev/null +++ b/.github/workflows/soak-test.yml @@ -0,0 +1,38 @@ +name: Soak Test + +on: push + +jobs: + soak-test: + timeout-minutes: 15 + # TODO should we use the same container as circle & central? + runs-on: ubuntu-latest + services: + # see: https://docs.github.com/en/enterprise-server@3.5/actions/using-containerized-services/creating-postgresql-service-containers + postgres: + image: postgres:14.10 + env: + POSTGRES_PASSWORD: odktest + ports: + - 5432:5432 + # Set health checks to wait until postgres has started + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + steps: + - uses: actions/checkout@v4 + - name: Use Node.js 20 + uses: actions/setup-node@v4 + with: + node-version: 20.17.0 + cache: 'npm' + - run: npm ci + - run: node lib/bin/create-docker-databases.js + - name: Soak Test + timeout-minutes: 10 + run: ./test/e2e/soak/ci + - name: Backend Logs + if: always() + run: "! [[ -f ./backend.log ]] || cat ./backend.log" From 9d37c6e117cf04444d84ce7d781ad488dbd23d96 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 05:53:23 +0000 Subject: [PATCH 21/40] ci: fix test name --- .github/workflows/db-migrations.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/db-migrations.yml b/.github/workflows/db-migrations.yml index d0f03f718..7924f04a3 100644 --- a/.github/workflows/db-migrations.yml +++ b/.github/workflows/db-migrations.yml @@ -8,7 +8,7 @@ on: - test/db-migrations/** jobs: - standard-tests: + db-migration-tests: timeout-minutes: 2 # TODO should we use the same container as circle & central? runs-on: ubuntu-latest From 841d8da9cef22c57d6296051c34247494443874b Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 05:57:14 +0000 Subject: [PATCH 22/40] comment, var name --- test/db-migrations/migrator.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index 91856787c..5c69629a2 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -12,7 +12,7 @@ const { execSync } = require('node:child_process'); // Horrible hacks. Without this: // // 1. production migration code needs modifying, and -// 2. it takes 3 mins+ just to run the migrations +// 2. it takes 3+ mins just to run the migrations const migrationsDir = './lib/model/migrations'; const holdingPen = './test/db-migrations/.holding-pen'; @@ -96,6 +96,6 @@ function exists(migrationName) { } } -function hasRun(migratioName) { - return lastRunIdx >= getIndex(migratioName); +function hasRun(migrationName) { + return lastRunIdx >= getIndex(migrationName); } From 2bf5e108933e450de26bdc0017c59832313d9013 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 05:59:53 +0000 Subject: [PATCH 23/40] fix error message --- test/db-migrations/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index 1e4ce510e..811d915b1 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -13,7 +13,7 @@ function _describeMigration(describeFn, migrationName, fn) { assert.strictEqual(typeof describeFn, 'function'); - assert.ok(migrator.exists(migrationName), `Migration '${migrationName}' already exists.`); + assert.ok(migrator.exists(migrationName), `Migration '${migrationName}' does not exist.`); assert.ok(!migrator.hasRun(migrationName), `Migration '${migrationName}' has already been run.`); assert.strictEqual(typeof fn, 'function'); From 9da3542cd3640d538e6d573af0e6b26f0e217721 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 06:06:07 +0000 Subject: [PATCH 24/40] remove asyncs --- test/db-migrations/utils.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index 811d915b1..47a71de3b 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -21,7 +21,7 @@ function _describeMigration(describeFn, migrationName, fn) { const runMigrationBeingTested = (() => { let alreadyRun; - return async () => { + return () => { if(alreadyRun) throw new Error('Migration has already run! Check your test structure.'); alreadyRun = true; migrator.runIncluding(migrationName); @@ -29,7 +29,7 @@ function _describeMigration(describeFn, migrationName, fn) { })(); return describeFn(`database migration: ${migrationName}`, () => { - before(async () => { + before(() => { migrator.runBefore(migrationName); }); return fn({ runMigrationBeingTested }); From 7a60283d69b1f3934e2f9846cf8e6303fdf6af52 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 06:12:22 +0000 Subject: [PATCH 25/40] remove global assert --- test/db-migrations/.eslintrc.js | 1 - test/db-migrations/mocha-setup.js | 3 --- test/db-migrations/utils.js | 1 + 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index 1c8be7708..6b53ad991 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -11,7 +11,6 @@ module.exports = { 'prefer-arrow-callback': 'off', }, globals: { - assert: false, db: false, log: false, sql: false, diff --git a/test/db-migrations/mocha-setup.js b/test/db-migrations/mocha-setup.js index 643fd05c5..08d592164 100644 --- a/test/db-migrations/mocha-setup.js +++ b/test/db-migrations/mocha-setup.js @@ -1,7 +1,6 @@ const _log = level => (...args) => console.log(level, ...args); global.log = _log('[INFO]'); -global.assert = require('node:assert'); const fs = require('node:fs'); const slonik = require('slonik'); const migrator = require('./migrator'); @@ -9,8 +8,6 @@ const migrator = require('./migrator'); async function mochaGlobalSetup() { log('mochaGlobalSetup() :: ENTRY'); - global.assert = assert; - global.sql = slonik.sql; const { user, password, host, database } = jsonFile('./config/db-migration-test.json').default.database; diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index 47a71de3b..a7075259a 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -5,6 +5,7 @@ module.exports = { describeMigration, }; +const assert = require('node:assert'); const _ = require('lodash'); const migrator = require('./migrator'); From 8482ed4c317aa2685f437f8560097c5a0bd78c79 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 06:21:41 +0000 Subject: [PATCH 26/40] Migrator: update comments --- test/db-migrations/migrator.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index 5c69629a2..69ddc4429 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -1,3 +1,17 @@ +// This functions by moving migration files in and out of the migrations target +// directory. +// +// Aims: +// +// * ensure that the real production migration command is run in tests +// * avoid isolation differences between running the tests & migrations in the +// same node instance. These differences might include shared globals, +// database connection pool state, database transaction states etc. +// * ensure that these tests do not depend directly on knex. Removing knex +// dependency is a long-term project goal. +// * allow replacement of the production migration implementation without +// changing tests + module.exports = { exists, hasRun, @@ -9,11 +23,6 @@ module.exports = { const fs = require('node:fs'); const { execSync } = require('node:child_process'); -// Horrible hacks. Without this: -// -// 1. production migration code needs modifying, and -// 2. it takes 3+ mins just to run the migrations - const migrationsDir = './lib/model/migrations'; const holdingPen = './test/db-migrations/.holding-pen'; From b1b738f3f681224185e6c25c1a1bb8a33a9d55fa Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 07:42:12 +0000 Subject: [PATCH 27/40] ci: add more dependencies --- .github/workflows/db-migrations.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/db-migrations.yml b/.github/workflows/db-migrations.yml index 7924f04a3..3bdb07487 100644 --- a/.github/workflows/db-migrations.yml +++ b/.github/workflows/db-migrations.yml @@ -4,8 +4,12 @@ on: push: paths: - .github/workflows/db-migrations.yml + - lib/bin/create-docker-databases.js - lib/model/migrations/** - test/db-migrations/** + - package.json + - package-lock.json + - Makefile jobs: db-migration-tests: From 1e6bb2ba8e6a943703e01020bf5927d34534cfa7 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 07:44:09 +0000 Subject: [PATCH 28/40] Clean database before use --- test/db-migrations/mocha-setup.js | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/test/db-migrations/mocha-setup.js b/test/db-migrations/mocha-setup.js index 08d592164..9d3b3a718 100644 --- a/test/db-migrations/mocha-setup.js +++ b/test/db-migrations/mocha-setup.js @@ -15,22 +15,9 @@ async function mochaGlobalSetup() { log('dbUrl:', dbUrl); global.db = slonik.createPool(dbUrl); - 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. - - If you are using odk-postgres14 docker, try: - - docker exec odk-postgres14 psql -U postgres ${database} -c " - DROP SCHEMA public CASCADE; - CREATE SCHEMA public; - GRANT ALL ON SCHEMA public TO postgres; - GRANT ALL ON SCHEMA public TO public; - " - `); - process.exit(1); - } + // Try to clean up the test database. This should work unless you've used + // different users to create/configure the DB. + await db.query(sql`DROP OWNED BY CURRENT_USER`); log('mochaGlobalSetup() :: EXIT'); } From e746eae40075c44fba3f2552fe38cf093193bebc Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 07:46:59 +0000 Subject: [PATCH 29/40] delete all logging --- test/db-migrations/.eslintrc.js | 1 - test/db-migrations/migrator.js | 18 ++---------------- test/db-migrations/mocha-setup.js | 10 ---------- 3 files changed, 2 insertions(+), 27 deletions(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index 6b53ad991..949bd2159 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -12,7 +12,6 @@ module.exports = { }, globals: { db: false, - log: false, sql: false, }, }; diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index 69ddc4429..c2de6f96a 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -40,8 +40,6 @@ function runBefore(migrationName) { const previousMigration = allMigrations[idx - 1]; - log('previousMigration:', previousMigration); - return runIncluding(previousMigration); } @@ -53,17 +51,13 @@ function runIncluding(lastMigrationToRun) { fs.renameSync(`${holdingPen}/${f}`, `${migrationsDir}/${f}`); } - log('Running migrations until:', lastMigrationToRun, '...'); - const res = execSync(`node ./lib/bin/run-migrations.js`, { encoding:'utf8' }); + execSync(`node ./lib/bin/run-migrations.js`, { encoding:'utf8' }); lastRunIdx = finalIdx; - - log(`Ran migrations up-to-and-including ${lastMigrationToRun}:\n`, res); } function getIndex(migrationName) { const idx = allMigrations.indexOf(migrationName); - log('getIndex()', migrationName, 'found at', idx); if(idx === -1) throw new Error(`Unknown migration: ${migrationName}`); return idx; } @@ -82,18 +76,10 @@ function moveAll(src, tgt) { } function loadMigrationsList() { - const migrations = fs.readdirSync(migrationsDir) + return fs.readdirSync(migrationsDir) .filter(f => f.endsWith('.js')) .map(f => f.replace(/\.js$/, '')) .sort(); // TODO check that this is how knex sorts migration files - log(); - log('All migrations:'); - log(); - migrations.forEach(m => log('*', m)); - log(); - log('Total:', migrations.length); - log(); - return migrations; } function exists(migrationName) { diff --git a/test/db-migrations/mocha-setup.js b/test/db-migrations/mocha-setup.js index 9d3b3a718..d976497de 100644 --- a/test/db-migrations/mocha-setup.js +++ b/test/db-migrations/mocha-setup.js @@ -1,32 +1,22 @@ -const _log = level => (...args) => console.log(level, ...args); -global.log = _log('[INFO]'); - const fs = require('node:fs'); const slonik = require('slonik'); const migrator = require('./migrator'); async function mochaGlobalSetup() { - log('mochaGlobalSetup() :: ENTRY'); - global.sql = slonik.sql; const { user, password, host, database } = jsonFile('./config/db-migration-test.json').default.database; const dbUrl = `postgres://${user}:${password}@${host}/${database}`; - log('dbUrl:', dbUrl); global.db = slonik.createPool(dbUrl); // Try to clean up the test database. This should work unless you've used // different users to create/configure the DB. await db.query(sql`DROP OWNED BY CURRENT_USER`); - - log('mochaGlobalSetup() :: EXIT'); } function mochaGlobalTeardown() { - log('mochaGlobalTeardown() :: ENTRY'); db?.end(); migrator.restoreMigrations(); - log('mochaGlobalTeardown() :: EXIT'); } module.exports = { mochaGlobalSetup, mochaGlobalTeardown }; From 193e0c5c9dc43ce70815632e5b584e5d4107fb73 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 07:50:33 +0000 Subject: [PATCH 30/40] Revert "delete all logging" This reverts commit e746eae40075c44fba3f2552fe38cf093193bebc. --- test/db-migrations/.eslintrc.js | 1 + test/db-migrations/migrator.js | 18 ++++++++++++++++-- test/db-migrations/mocha-setup.js | 10 ++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index 949bd2159..6b53ad991 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -12,6 +12,7 @@ module.exports = { }, globals: { db: false, + log: false, sql: false, }, }; diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index c2de6f96a..69ddc4429 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -40,6 +40,8 @@ function runBefore(migrationName) { const previousMigration = allMigrations[idx - 1]; + log('previousMigration:', previousMigration); + return runIncluding(previousMigration); } @@ -51,13 +53,17 @@ function runIncluding(lastMigrationToRun) { fs.renameSync(`${holdingPen}/${f}`, `${migrationsDir}/${f}`); } - execSync(`node ./lib/bin/run-migrations.js`, { encoding:'utf8' }); + log('Running migrations until:', lastMigrationToRun, '...'); + const res = execSync(`node ./lib/bin/run-migrations.js`, { encoding:'utf8' }); lastRunIdx = finalIdx; + + log(`Ran migrations up-to-and-including ${lastMigrationToRun}:\n`, res); } function getIndex(migrationName) { const idx = allMigrations.indexOf(migrationName); + log('getIndex()', migrationName, 'found at', idx); if(idx === -1) throw new Error(`Unknown migration: ${migrationName}`); return idx; } @@ -76,10 +82,18 @@ function moveAll(src, tgt) { } function loadMigrationsList() { - return fs.readdirSync(migrationsDir) + 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 + log(); + log('All migrations:'); + log(); + migrations.forEach(m => log('*', m)); + log(); + log('Total:', migrations.length); + log(); + return migrations; } function exists(migrationName) { diff --git a/test/db-migrations/mocha-setup.js b/test/db-migrations/mocha-setup.js index d976497de..9d3b3a718 100644 --- a/test/db-migrations/mocha-setup.js +++ b/test/db-migrations/mocha-setup.js @@ -1,22 +1,32 @@ +const _log = level => (...args) => console.log(level, ...args); +global.log = _log('[INFO]'); + const fs = require('node:fs'); const slonik = require('slonik'); const migrator = require('./migrator'); async function mochaGlobalSetup() { + log('mochaGlobalSetup() :: ENTRY'); + global.sql = slonik.sql; const { user, password, host, database } = jsonFile('./config/db-migration-test.json').default.database; const dbUrl = `postgres://${user}:${password}@${host}/${database}`; + log('dbUrl:', dbUrl); global.db = slonik.createPool(dbUrl); // Try to clean up the test database. This should work unless you've used // different users to create/configure the DB. await db.query(sql`DROP OWNED BY CURRENT_USER`); + + log('mochaGlobalSetup() :: EXIT'); } function mochaGlobalTeardown() { + log('mochaGlobalTeardown() :: ENTRY'); db?.end(); migrator.restoreMigrations(); + log('mochaGlobalTeardown() :: EXIT'); } module.exports = { mochaGlobalSetup, mochaGlobalTeardown }; From d25107263129a2df44a7d6f5c9fd7c9e83b4246b Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 07:51:42 +0000 Subject: [PATCH 31/40] clean up logging --- test/db-migrations/.eslintrc.js | 1 - test/db-migrations/migrator.js | 2 -- test/db-migrations/mocha-setup.js | 2 +- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index 6b53ad991..0561f5b47 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -3,7 +3,6 @@ module.exports = { rules: { 'key-spacing': 'off', 'keyword-spacing': 'off', - 'no-console': 'off', 'no-multi-spaces': 'off', 'no-plusplus': 'off', 'no-use-before-define': 'off', diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index 69ddc4429..f5f93bea3 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -40,8 +40,6 @@ function runBefore(migrationName) { const previousMigration = allMigrations[idx - 1]; - log('previousMigration:', previousMigration); - return runIncluding(previousMigration); } diff --git a/test/db-migrations/mocha-setup.js b/test/db-migrations/mocha-setup.js index 9d3b3a718..ddef91541 100644 --- a/test/db-migrations/mocha-setup.js +++ b/test/db-migrations/mocha-setup.js @@ -1,4 +1,4 @@ -const _log = level => (...args) => console.log(level, ...args); +const _log = level => (...args) => console.log(level, ...args); // eslint-disable-line no-console global.log = _log('[INFO]'); const fs = require('node:fs'); From b4b5a10239b2acd01b58499550ccd518e3bddbf8 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 07:53:54 +0000 Subject: [PATCH 32/40] remove lint rule: key-spacing --- test/db-migrations/.eslintrc.js | 1 - .../20241008-01-add-user_preferences.spec.js | 14 +++++++------- test/db-migrations/migrator.js | 4 ++-- test/db-migrations/mocha-setup.js | 2 +- test/db-migrations/utils.js | 2 +- 5 files changed, 11 insertions(+), 12 deletions(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index 0561f5b47..ff3506b35 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -1,7 +1,6 @@ module.exports = { extends: '../.eslintrc.js', rules: { - 'key-spacing': 'off', 'keyword-spacing': 'off', 'no-multi-spaces': 'off', 'no-plusplus': 'off', diff --git a/test/db-migrations/20241008-01-add-user_preferences.spec.js b/test/db-migrations/20241008-01-add-user_preferences.spec.js index 976fc8eaa..59afa13f4 100644 --- a/test/db-migrations/20241008-01-add-user_preferences.spec.js +++ b/test/db-migrations/20241008-01-add-user_preferences.spec.js @@ -15,9 +15,9 @@ describeMigration('20241008-01-add-user_preferences', ({ runMigrationBeingTested it('should create user_site_preferences table', async () => { await assertTableSchema('user_site_preferences', - { column_name:'userId', is_nullable:'NO', data_type:'integer' }, - { column_name:'propertyName', is_nullable:'NO', data_type:'text' }, - { column_name:'propertyValue', is_nullable:'NO', data_type:'jsonb' }, + { column_name: 'userId', is_nullable: 'NO', data_type: 'integer' }, + { column_name: 'propertyName', is_nullable: 'NO', data_type: 'text' }, + { column_name: 'propertyValue', is_nullable: 'NO', data_type: 'jsonb' }, ); }); @@ -30,10 +30,10 @@ describeMigration('20241008-01-add-user_preferences', ({ runMigrationBeingTested it('should create user_project_preferences table', async () => { await assertTableSchema('user_project_preferences', - { column_name:'userId', is_nullable:'NO', data_type:'integer' }, - { column_name:'projectId', is_nullable:'NO', data_type:'integer' }, - { column_name:'propertyName', is_nullable:'NO', data_type:'text' }, - { column_name:'propertyValue', is_nullable:'NO', data_type:'jsonb' }, + { column_name: 'userId', is_nullable: 'NO', data_type: 'integer' }, + { column_name: 'projectId', is_nullable: 'NO', data_type: 'integer' }, + { column_name: 'propertyName', is_nullable: 'NO', data_type: 'text' }, + { column_name: 'propertyValue', is_nullable: 'NO', data_type: 'jsonb' }, ); }); diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index f5f93bea3..eebfe41b2 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -26,7 +26,7 @@ const { execSync } = require('node:child_process'); const migrationsDir = './lib/model/migrations'; const holdingPen = './test/db-migrations/.holding-pen'; -fs.mkdirSync(holdingPen, { recursive:true }); +fs.mkdirSync(holdingPen, { recursive: true }); restoreMigrations(); const allMigrations = loadMigrationsList(); @@ -52,7 +52,7 @@ function runIncluding(lastMigrationToRun) { } log('Running migrations until:', lastMigrationToRun, '...'); - const res = execSync(`node ./lib/bin/run-migrations.js`, { encoding:'utf8' }); + const res = execSync(`node ./lib/bin/run-migrations.js`, { encoding: 'utf8' }); lastRunIdx = finalIdx; diff --git a/test/db-migrations/mocha-setup.js b/test/db-migrations/mocha-setup.js index ddef91541..07e0ccf15 100644 --- a/test/db-migrations/mocha-setup.js +++ b/test/db-migrations/mocha-setup.js @@ -32,5 +32,5 @@ function mochaGlobalTeardown() { module.exports = { mochaGlobalSetup, mochaGlobalTeardown }; function jsonFile(path) { - return JSON.parse(fs.readFileSync(path, { encoding:'utf8' })); + return JSON.parse(fs.readFileSync(path, { encoding: 'utf8' })); } diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index a7075259a..f0e0ca5ce 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -99,7 +99,7 @@ function assertRowsMatch(actualRows, expectedRows) { const filteredRemainingRows = remainingRows.map(r => _.pick(r, Object.keys(x))); assert.fail( `Expected row ${i} not found:\njson=` + - JSON.stringify({ remainingRows, filteredRemainingRows, expectedRow:x }), + JSON.stringify({ remainingRows, filteredRemainingRows, expectedRow: x }), ); } } From 3e5e3bb17fbb8dcdb99e204ccbca6cd59a5f99a5 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 07:54:58 +0000 Subject: [PATCH 33/40] remove eslint rule no-plusplus --- test/db-migrations/.eslintrc.js | 1 - test/db-migrations/migrator.js | 2 +- test/db-migrations/utils.js | 4 ++-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index ff3506b35..ed084b558 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -3,7 +3,6 @@ module.exports = { rules: { 'keyword-spacing': 'off', 'no-multi-spaces': 'off', - 'no-plusplus': 'off', 'no-use-before-define': 'off', 'object-curly-newline': 'off', 'prefer-arrow-callback': 'off', diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index eebfe41b2..ddd00a321 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -46,7 +46,7 @@ function runBefore(migrationName) { function runIncluding(lastMigrationToRun) { const finalIdx = getIndex(lastMigrationToRun); - for(let restoreIdx=lastRunIdx+1; restoreIdx<=finalIdx; ++restoreIdx) { + for(let restoreIdx=lastRunIdx+1; restoreIdx<=finalIdx; ++restoreIdx) { // eslint-disable-line no-plusplus const f = allMigrations[restoreIdx] + '.js'; fs.renameSync(`${holdingPen}/${f}`, `${migrationsDir}/${f}`); } diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index f0e0ca5ce..7480c61e0 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -83,10 +83,10 @@ function assertRowsMatch(actualRows, expectedRows) { assert.strictEqual(actualRows.length, expectedRows.length, 'row count mismatch'); const remainingRows = [...actualRows]; - for(let i=0; i Date: Wed, 20 Nov 2024 07:55:49 +0000 Subject: [PATCH 34/40] remove eslint rule exception : object-curly-newline --- test/db-migrations/.eslintrc.js | 1 - test/db-migrations/20241008-01-add-user_preferences.spec.js | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index ed084b558..8f6b05f7d 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -4,7 +4,6 @@ module.exports = { 'keyword-spacing': 'off', 'no-multi-spaces': 'off', 'no-use-before-define': 'off', - 'object-curly-newline': 'off', 'prefer-arrow-callback': 'off', }, globals: { diff --git a/test/db-migrations/20241008-01-add-user_preferences.spec.js b/test/db-migrations/20241008-01-add-user_preferences.spec.js index 59afa13f4..404d49a59 100644 --- a/test/db-migrations/20241008-01-add-user_preferences.spec.js +++ b/test/db-migrations/20241008-01-add-user_preferences.spec.js @@ -1,9 +1,9 @@ -const { +const { // eslint-disable-line object-curly-newline assertIndexExists, assertTableDoesNotExist, assertTableSchema, describeMigration, -} = require('./utils'); +} = require('./utils'); // eslint-disable-line object-curly-newline describeMigration('20241008-01-add-user_preferences', ({ runMigrationBeingTested }) => { before(async () => { From e6edbdd377c880495b17b0d7d1aa974fbddde829 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 07:57:38 +0000 Subject: [PATCH 35/40] eslint: no-multi-spaces --- test/db-migrations/.eslintrc.js | 1 - .../20241008-01-add-user_preferences.spec.js | 10 +++++----- test/db-migrations/utils.js | 4 ++-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index 8f6b05f7d..bb25ec590 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -2,7 +2,6 @@ module.exports = { extends: '../.eslintrc.js', rules: { 'keyword-spacing': 'off', - 'no-multi-spaces': 'off', 'no-use-before-define': 'off', 'prefer-arrow-callback': 'off', }, diff --git a/test/db-migrations/20241008-01-add-user_preferences.spec.js b/test/db-migrations/20241008-01-add-user_preferences.spec.js index 404d49a59..8a89e1eea 100644 --- a/test/db-migrations/20241008-01-add-user_preferences.spec.js +++ b/test/db-migrations/20241008-01-add-user_preferences.spec.js @@ -15,8 +15,8 @@ describeMigration('20241008-01-add-user_preferences', ({ runMigrationBeingTested it('should create user_site_preferences table', async () => { await assertTableSchema('user_site_preferences', - { column_name: 'userId', is_nullable: 'NO', data_type: 'integer' }, - { column_name: 'propertyName', is_nullable: 'NO', data_type: 'text' }, + { column_name: 'userId', is_nullable: 'NO', data_type: 'integer' }, // eslint-disable-line no-multi-spaces + { column_name: 'propertyName', is_nullable: 'NO', data_type: 'text' }, // eslint-disable-line no-multi-spaces { column_name: 'propertyValue', is_nullable: 'NO', data_type: 'jsonb' }, ); }); @@ -30,9 +30,9 @@ describeMigration('20241008-01-add-user_preferences', ({ runMigrationBeingTested it('should create user_project_preferences table', async () => { await assertTableSchema('user_project_preferences', - { column_name: 'userId', is_nullable: 'NO', data_type: 'integer' }, - { column_name: 'projectId', is_nullable: 'NO', data_type: 'integer' }, - { column_name: 'propertyName', is_nullable: 'NO', data_type: 'text' }, + { column_name: 'userId', is_nullable: 'NO', data_type: 'integer' }, // eslint-disable-line no-multi-spaces + { column_name: 'projectId', is_nullable: 'NO', data_type: 'integer' }, // eslint-disable-line no-multi-spaces + { column_name: 'propertyName', is_nullable: 'NO', data_type: 'text' }, // eslint-disable-line no-multi-spaces { column_name: 'propertyValue', is_nullable: 'NO', data_type: 'jsonb' }, ); }); diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index 7480c61e0..aee153b36 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -37,8 +37,8 @@ function _describeMigration(describeFn, migrationName, fn) { }); } function describeMigration(...args) { return _describeMigration(describe, ...args); } -describeMigration.only = (...args) => _describeMigration(describe.only, ...args); // eslint-disable-line no-only-tests/no-only-tests -describeMigration.skip = (...args) => _describeMigration(describe.skip, ...args); +describeMigration.only = (...args) => _describeMigration(describe.only, ...args); // eslint-disable-line no-only-tests/no-only-tests, no-multi-spaces +describeMigration.skip = (...args) => _describeMigration(describe.skip, ...args); // eslint-disable-line no-multi-spaces async function assertIndexExists(tableName, expected) { if(arguments.length !== 2) throw new Error('Incorrect arg count.'); From fc02afc117d268ab614e944cb3943a8160b8427b Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 07:58:06 +0000 Subject: [PATCH 36/40] eslint: prefer-arrow-callback --- test/db-migrations/.eslintrc.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index bb25ec590..b163d625c 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -3,7 +3,6 @@ module.exports = { rules: { 'keyword-spacing': 'off', 'no-use-before-define': 'off', - 'prefer-arrow-callback': 'off', }, globals: { db: false, From 8a3c7accf6988b250656d589906e8ce0beafef69 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 07:59:33 +0000 Subject: [PATCH 37/40] eslint: keyword-spacing --- test/db-migrations/.eslintrc.js | 1 - test/db-migrations/migrator.js | 8 ++++---- test/db-migrations/utils.js | 24 ++++++++++++------------ 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index b163d625c..db4ab165e 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -1,7 +1,6 @@ module.exports = { extends: '../.eslintrc.js', rules: { - 'keyword-spacing': 'off', 'no-use-before-define': 'off', }, globals: { diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index ddd00a321..31c5b495d 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -36,7 +36,7 @@ let lastRunIdx = -1; function runBefore(migrationName) { const idx = getIndex(migrationName); - if(idx === 0) return; + if (idx === 0) return; const previousMigration = allMigrations[idx - 1]; @@ -46,7 +46,7 @@ function runBefore(migrationName) { function runIncluding(lastMigrationToRun) { const finalIdx = getIndex(lastMigrationToRun); - for(let restoreIdx=lastRunIdx+1; restoreIdx<=finalIdx; ++restoreIdx) { // eslint-disable-line no-plusplus + for (let restoreIdx=lastRunIdx+1; restoreIdx<=finalIdx; ++restoreIdx) { // eslint-disable-line no-plusplus const f = allMigrations[restoreIdx] + '.js'; fs.renameSync(`${holdingPen}/${f}`, `${migrationsDir}/${f}`); } @@ -62,7 +62,7 @@ function runIncluding(lastMigrationToRun) { function getIndex(migrationName) { const idx = allMigrations.indexOf(migrationName); log('getIndex()', migrationName, 'found at', idx); - if(idx === -1) throw new Error(`Unknown migration: ${migrationName}`); + if (idx === -1) throw new Error(`Unknown migration: ${migrationName}`); return idx; } @@ -98,7 +98,7 @@ function exists(migrationName) { try { getIndex(migrationName); return true; - } catch(err) { + } catch (err) { return false; } } diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index aee153b36..5cc042078 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -23,7 +23,7 @@ function _describeMigration(describeFn, migrationName, fn) { const runMigrationBeingTested = (() => { let alreadyRun; return () => { - if(alreadyRun) throw new Error('Migration has already run! Check your test structure.'); + if (alreadyRun) throw new Error('Migration has already run! Check your test structure.'); alreadyRun = true; migrator.runIncluding(migrationName); }; @@ -41,10 +41,10 @@ describeMigration.only = (...args) => _describeMigration(describe.only, . describeMigration.skip = (...args) => _describeMigration(describe.skip, ...args); // eslint-disable-line no-multi-spaces async function assertIndexExists(tableName, expected) { - if(arguments.length !== 2) throw new Error('Incorrect arg count.'); + if (arguments.length !== 2) throw new Error('Incorrect arg count.'); const actualIndexes = await db.anyFirst(sql`SELECT indexdef FROM pg_indexes WHERE tablename=${tableName}`); - if(actualIndexes.includes(expected)) return true; + if (actualIndexes.includes(expected)) return true; assert.fail( 'Could not find expected index:\njson=' + JSON.stringify({ expected, actualIndexes, }), @@ -65,7 +65,7 @@ async function assertTableSchema(tableName, ...expectedCols) { await assertTableExists(tableName); expectedCols.forEach((def, idx) => { - if(!def.column_name) throw new Error(`Expected column definition is missing required prop: .column_name at index ${idx}`); + if (!def.column_name) throw new Error(`Expected column definition is missing required prop: .column_name at index ${idx}`); }); const actualCols = await db.any(sql`SELECT * FROM information_schema.columns WHERE table_name=${tableName}`); @@ -83,19 +83,19 @@ function assertRowsMatch(actualRows, expectedRows) { assert.strictEqual(actualRows.length, expectedRows.length, 'row count mismatch'); const remainingRows = [...actualRows]; - for(let i=0; i _.pick(r, Object.keys(x))); assert.fail( `Expected row ${i} not found:\njson=` + @@ -106,17 +106,17 @@ function assertRowsMatch(actualRows, expectedRows) { } function assertEqualInAnyOrder(a, b, message) { - if(!Array.isArray(a)) throw new Error('IllegalArgument: first arg is not an array'); - if(!Array.isArray(b)) throw new Error('IllegalArgument: second arg is not an array'); + if (!Array.isArray(a)) throw new Error('IllegalArgument: first arg is not an array'); + if (!Array.isArray(b)) throw new Error('IllegalArgument: second arg is not an array'); assert.deepEqual([...a].sort(), [...b].sort(), message); } function assertIncludes(actual, expected) { - for(const [k, expectedVal] of Object.entries(expected)) { + for (const [k, expectedVal] of Object.entries(expected)) { const actualVal = actual[k]; try { assert.deepEqual(actualVal, expectedVal); - } catch(err) { + } catch (err) { assert.fail(`Could not find all properties of ${expected} in ${actual}`); } } From d8280a1e8fbf1960b0b1f6b0d2580ae11a3e0a76 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Wed, 20 Nov 2024 08:02:25 +0000 Subject: [PATCH 38/40] eslint: remove no-use-before-define --- test/db-migrations/.eslintrc.js | 3 --- test/db-migrations/migrator.js | 32 +++++++++++++++---------------- test/db-migrations/mocha-setup.js | 2 +- test/db-migrations/utils.js | 20 +++++++++---------- 4 files changed, 27 insertions(+), 30 deletions(-) diff --git a/test/db-migrations/.eslintrc.js b/test/db-migrations/.eslintrc.js index db4ab165e..c271fa8e7 100644 --- a/test/db-migrations/.eslintrc.js +++ b/test/db-migrations/.eslintrc.js @@ -1,8 +1,5 @@ module.exports = { extends: '../.eslintrc.js', - rules: { - 'no-use-before-define': 'off', - }, globals: { db: false, log: false, diff --git a/test/db-migrations/migrator.js b/test/db-migrations/migrator.js index 31c5b495d..739b8d971 100644 --- a/test/db-migrations/migrator.js +++ b/test/db-migrations/migrator.js @@ -12,14 +12,6 @@ // * allow replacement of the production migration implementation without // changing tests -module.exports = { - exists, - hasRun, - runBefore, - runIncluding, - restoreMigrations, -}; - const fs = require('node:fs'); const { execSync } = require('node:child_process'); @@ -28,23 +20,23 @@ const holdingPen = './test/db-migrations/.holding-pen'; fs.mkdirSync(holdingPen, { recursive: true }); -restoreMigrations(); -const allMigrations = loadMigrationsList(); -moveMigrationsToHoldingPen(); +restoreMigrations(); // eslint-disable-line no-use-before-define +const allMigrations = loadMigrationsList(); // eslint-disable-line no-use-before-define +moveMigrationsToHoldingPen(); // eslint-disable-line no-use-before-define let lastRunIdx = -1; function runBefore(migrationName) { - const idx = getIndex(migrationName); + const idx = getIndex(migrationName); // eslint-disable-line no-use-before-define if (idx === 0) return; const previousMigration = allMigrations[idx - 1]; - return runIncluding(previousMigration); + return runIncluding(previousMigration); // eslint-disable-line no-use-before-define } function runIncluding(lastMigrationToRun) { - const finalIdx = getIndex(lastMigrationToRun); + const finalIdx = getIndex(lastMigrationToRun); // eslint-disable-line no-use-before-define for (let restoreIdx=lastRunIdx+1; restoreIdx<=finalIdx; ++restoreIdx) { // eslint-disable-line no-plusplus const f = allMigrations[restoreIdx] + '.js'; @@ -67,11 +59,11 @@ function getIndex(migrationName) { } function restoreMigrations() { - moveAll(holdingPen, migrationsDir); + moveAll(holdingPen, migrationsDir); // eslint-disable-line no-use-before-define } function moveMigrationsToHoldingPen() { - moveAll(migrationsDir, holdingPen); + moveAll(migrationsDir, holdingPen); // eslint-disable-line no-use-before-define } function moveAll(src, tgt) { @@ -106,3 +98,11 @@ function exists(migrationName) { function hasRun(migrationName) { return lastRunIdx >= getIndex(migrationName); } + +module.exports = { + exists, + hasRun, + runBefore, + runIncluding, + restoreMigrations, +}; diff --git a/test/db-migrations/mocha-setup.js b/test/db-migrations/mocha-setup.js index 07e0ccf15..235620d69 100644 --- a/test/db-migrations/mocha-setup.js +++ b/test/db-migrations/mocha-setup.js @@ -10,7 +10,7 @@ async function mochaGlobalSetup() { global.sql = slonik.sql; - const { user, password, host, database } = jsonFile('./config/db-migration-test.json').default.database; + const { user, password, host, database } = jsonFile('./config/db-migration-test.json').default.database; // eslint-disable-line no-use-before-define const dbUrl = `postgres://${user}:${password}@${host}/${database}`; log('dbUrl:', dbUrl); global.db = slonik.createPool(dbUrl); diff --git a/test/db-migrations/utils.js b/test/db-migrations/utils.js index 5cc042078..85788ea01 100644 --- a/test/db-migrations/utils.js +++ b/test/db-migrations/utils.js @@ -1,10 +1,3 @@ -module.exports = { - assertIndexExists, - assertTableDoesNotExist, - assertTableSchema, - describeMigration, -}; - const assert = require('node:assert'); const _ = require('lodash'); const migrator = require('./migrator'); @@ -70,13 +63,13 @@ async function assertTableSchema(tableName, ...expectedCols) { const actualCols = await db.any(sql`SELECT * FROM information_schema.columns WHERE table_name=${tableName}`); - assertEqualInAnyOrder( + assertEqualInAnyOrder( // eslint-disable-line no-use-before-define expectedCols.map(col => col.column_name), actualCols.map(col => col.column_name), 'Expected columns did not match returned columns!', ); - assertRowsMatch(actualCols, expectedCols); + assertRowsMatch(actualCols, expectedCols); // eslint-disable-line no-use-before-define } function assertRowsMatch(actualRows, expectedRows) { @@ -89,7 +82,7 @@ function assertRowsMatch(actualRows, expectedRows) { for (let j=0; j Date: Tue, 10 Dec 2024 06:06:54 +0000 Subject: [PATCH 39/40] run on PRs --- .github/workflows/db-migrations.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/db-migrations.yml b/.github/workflows/db-migrations.yml index 3bdb07487..17feecef7 100644 --- a/.github/workflows/db-migrations.yml +++ b/.github/workflows/db-migrations.yml @@ -1,6 +1,15 @@ name: Database Migrations on: + pull_request: + paths: + - .github/workflows/db-migrations.yml + - lib/bin/create-docker-databases.js + - lib/model/migrations/** + - test/db-migrations/** + - package.json + - package-lock.json + - Makefile push: paths: - .github/workflows/db-migrations.yml From 6317a80364533439f40d9fecd7d1a0094fd3c430 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Tue, 10 Dec 2024 06:07:46 +0000 Subject: [PATCH 40/40] update node version --- .github/workflows/db-migrations.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/db-migrations.yml b/.github/workflows/db-migrations.yml index 17feecef7..8b6594c9c 100644 --- a/.github/workflows/db-migrations.yml +++ b/.github/workflows/db-migrations.yml @@ -41,10 +41,10 @@ jobs: --health-retries 5 steps: - uses: actions/checkout@v4 - - name: Use Node.js 20 + - name: Set node version uses: actions/setup-node@v4 with: - node-version: 20.17.0 + node-version: 22.12.0 cache: 'npm' - run: npm ci - run: node lib/bin/create-docker-databases.js