fix: pr feedback

ipfs · Nov 6, 2019 · 40dc006 · 40dc006
1 parent c73f9d2
commit 40dc006
Show file tree

Hide file tree

Showing 7 changed files with 62 additions and 85 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -66,7 +66,6 @@ function noop () {}
 
 function expectedRepoOptions () {
   const options = {
-    autoMigrate: true,
     lock: process.browser ? 'memory' : 'fs',
     storageBackends: {
       // packages are exchanged to browser-compatible