From 40dc0063e59f9da4c6b972059ab1de6d21128d95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Uhl=C3=AD=C5=99?= Date: Wed, 6 Nov 2019 10:50:49 +0100 Subject: [PATCH] fix: pr feedback --- src/default-options.js | 1 - src/errors/index.js | 9 ++--- src/index.js | 7 +++- test/browser.js | 14 ++----- test/migrations-test.js | 84 ++++++++++++++++++++--------------------- test/node.js | 31 ++++----------- test/options-test.js | 1 - 7 files changed, 62 insertions(+), 85 deletions(-) diff --git a/src/default-options.js b/src/default-options.js index 913a264e..e9322d67 100644 --- a/src/default-options.js +++ b/src/default-options.js @@ -2,7 +2,6 @@ // Default configuration for a repo in node.js module.exports = { - autoMigrate: true, lock: 'fs', storageBackends: { root: require('datastore-fs'), diff --git a/src/errors/index.js b/src/errors/index.js index fbe05711..67628414 100644 --- a/src/errors/index.js +++ b/src/errors/index.js @@ -7,8 +7,7 @@ class LockExistsError extends Error { constructor (message) { super(message) this.name = 'LockExistsError' - this.code = 'ERR_LOCK_EXISTS' - this.message = message + this.code = LockExistsError.code } } @@ -22,8 +21,7 @@ class NotFoundError extends Error { constructor (message) { super(message) this.name = 'NotFoundError' - this.code = 'ERR_NOT_FOUND' - this.message = message + this.code = NotFoundError.code } } @@ -37,8 +35,7 @@ class InvalidRepoVersionError extends Error { constructor (message) { super(message) this.name = 'InvalidRepoVersionError' - this.code = 'ERR_INVALID_REPO_VERSION' - this.message = message + this.code = InvalidRepoVersionError.code } } diff --git a/src/index.js b/src/index.js index cf2b6b32..2696ee03 100644 --- a/src/index.js +++ b/src/index.js @@ -20,6 +20,7 @@ const defaultDatastore = require('./default-datastore') const ERRORS = require('./errors') const log = debug('repo') +const migrations = debug('repo:migrations') const noLimit = Number.MAX_SAFE_INTEGER const AUTO_MIGRATE_CONFIG_KEY = 'repoAutoMigrate' @@ -276,6 +277,10 @@ class IpfsRepo { } async _isAutoMigrationEnabled () { + if(this.options.autoMigrate !== undefined){ + return this.options.autoMigrate + } + let autoMigrateConfig try { autoMigrateConfig = await this.config.get(AUTO_MIGRATE_CONFIG_KEY) @@ -287,7 +292,7 @@ class IpfsRepo { } } - return autoMigrateConfig && this.options.autoMigrate + return autoMigrateConfig } async _migrate (toVersion) { diff --git a/test/browser.js b/test/browser.js index 3cf1fbf2..32a5e23b 100644 --- a/test/browser.js +++ b/test/browser.js @@ -4,21 +4,15 @@ const IPFSRepo = require('../src') -async function createTempRepo ({ dontOpen, opts }) { +async function createTempRepo (options = {}) { const date = Date.now().toString() const repoPath = 'test-repo-for-' + date - const repo = new IPFSRepo(repoPath, opts) + const repo = new IPFSRepo(repoPath, options) await repo.init({}) + await repo.open() - if (!dontOpen) { - await repo.open() - } - - return { - path: repoPath, - instance: repo - } + return repo } describe('IPFS Repo Tests on the Browser', () => { diff --git a/test/migrations-test.js b/test/migrations-test.js index 198a04f3..a2aee807 100644 --- a/test/migrations-test.js +++ b/test/migrations-test.js @@ -36,65 +36,65 @@ module.exports = (createTempRepo) => { }) beforeEach(async () => { - ({ instance: repo } = await createTempRepo({})) + repo = await createTempRepo() sinon.reset() }) - it('should migrate by default', async () => { - migrateStub.resolves() - repoVersionStub.value(8) - getLatestMigrationVersionStub.returns(9) - - await repo.version.set(7) - await repo.close() + // Testing migration logic + const migrationLogic = [ + { config: true, option: true, result: true }, + { config: true, option: false, result: false }, + { config: true, option: undefined, result: true }, + { config: false, option: true, result: true }, + { config: false, option: false, result: false }, + { config: false, option: undefined, result: false }, + { config: undefined, option: true, result: true }, + { config: undefined, option: false, result: false }, + { config: undefined, option: undefined, result: true }, + ] + + migrationLogic.forEach(({ config, option, result }) => { + it(`should ${result ? '' : 'not '}migrate when config=${config} and option=${option}`, async () => { + migrateStub.resolves() + repoVersionStub.value(8) + getLatestMigrationVersionStub.returns(9) + + if (config !== undefined) { + await repo.config.set('repoAutoMigrate', config) + } + await repo.version.set(7) + await repo.close() + + const newOpts = Object.assign({}, repo.options) + newOpts.autoMigrate = option + const newRepo = new IPFSRepo(repo.path, newOpts) - expect(migrateStub.called).to.be.false() + expect(migrateStub.called).to.be.false() - await repo.open() + try { + await newRepo.open() + if (!result) expect.fail('should have thrown error') + } catch (err) { + expect(err.code).to.equal(errors.InvalidRepoVersionError.code) + } - expect(migrateStub.called).to.be.true() + expect(migrateStub.called).to.eq(result) + }) }) - it('should not migrate when option autoMigrate is false', async () => { + it('should migrate by default', async () => { migrateStub.resolves() - repoVersionStub.resolves(8) + repoVersionStub.value(8) getLatestMigrationVersionStub.returns(9) await repo.version.set(7) await repo.close() - const newOpts = Object.assign({}, repo.options) - newOpts.autoMigrate = false - const newRepo = new IPFSRepo(repo.path, newOpts) - - expect(migrateStub.called).to.be.false() - try { - await newRepo.open() - expect.fail('should have thrown error') - } catch (err) { - expect(err.code).to.equal(errors.InvalidRepoVersionError.code) - } - expect(migrateStub.called).to.be.false() - }) - - it('should not migrate when config option repoAutoMigrate is false', async () => { - migrateStub.resolves() - repoVersionStub.resolves(8) - getLatestMigrationVersionStub.returns(9) - await repo.config.set('repoAutoMigrate', false) - await repo.version.set(7) - await repo.close() + await repo.open() - expect(migrateStub.called).to.be.false() - try { - await repo.open() - expect.fail('should have thrown error') - } catch (err) { - expect(migrateStub.called).to.be.false() - expect(err.code).to.equal(errors.InvalidRepoVersionError.code) - } + expect(migrateStub.called).to.be.true() }) it('should not migrate when versions matches', async () => { diff --git a/test/node.js b/test/node.js index cab5666d..970b2724 100644 --- a/test/node.js +++ b/test/node.js @@ -17,27 +17,13 @@ const fsstat = promisify(fs.stat) const IPFSRepo = require('../src') -async function createTempRepo ({ init, dontOpen, opts }) { - const testRepoPath = path.join(__dirname, 'test-repo') +async function createTempRepo (options = {}) { const date = Date.now().toString() const repoPath = path.join(os.tmpdir(), 'test-repo-for-' + date) - - const repo = new IPFSRepo(repoPath, opts) - - if (init) { - await repo.init({}) - } else { - await asyncNcp(testRepoPath, repoPath) - } - - if (!dontOpen) { - await repo.open() - } - - return { - path: repoPath, - instance: repo - } + await asyncNcp(path.join(__dirname, 'test-repo'), repoPath) + const repo = new IPFSRepo(repoPath, options) + await repo.open() + return repo } describe('IPFS Repo Tests onNode.js', () => { @@ -98,7 +84,7 @@ describe('IPFS Repo Tests onNode.js', () => { repos.forEach((r) => describe(r.name, () => { const testRepoPath = path.join(__dirname, 'test-repo') const date = Date.now().toString() - const repoPath = testRepoPath + '-for-' + date + const repoPath = path.join(os.tmpdir(), 'test-repo-for-' + date) const repo = new IPFSRepo(repoPath, r.opts) @@ -112,10 +98,7 @@ describe('IPFS Repo Tests onNode.js', () => { }) after(async () => { - try { - await repo.close() - } catch (e) { - } + await repo.close() await asyncRimraf(repoPath) }) diff --git a/test/options-test.js b/test/options-test.js index 17b688c7..8b2be95e 100644 --- a/test/options-test.js +++ b/test/options-test.js @@ -66,7 +66,6 @@ function noop () {} function expectedRepoOptions () { const options = { - autoMigrate: true, lock: process.browser ? 'memory' : 'fs', storageBackends: { // packages are exchanged to browser-compatible