[NEEDS #12] Refactor dependency version checking logic

.babelrc

@@ -1,3 +1,4 @@
 {
+  "plugins": ["transform-object-rest-spread"],
   "presets": ["es2015"]
-}
+}

.eslintrc

@@ -10,17 +10,13 @@
   ],
   "rules": {
     "chai-friendly/no-unused-expressions": 2,
-    "comma-dangle": 0,
+    "comma-dangle": ["error", "never"],
     "consistent-return": 0,
+    "func-names": 0,
     "import/prefer-default-export": 0,
     "linebreak-style": 0,
     "no-bitwise": 0,
     "no-param-reassign": 0,
-    "no-unnamed-functions": 0,
-    "no-unused-expressions": 0,
-
-    // this is for the tests, don't worrry about it for now
-    "import/no-unresolved": 1,
-    "import/extensions": 1
+    "no-unused-expressions": 0
   }
 }

.gitignore

@@ -39,5 +39,7 @@ node_modules
 # Generated test files
 test/repos
 test/server/lfs-test-server
+test/server/local-repo
+test/server/local-repo-cached
 
 \.DS_Store

README.md

@@ -46,4 +46,147 @@ Once cloned:
 ```
 cd nodegit-lfs
 yarn
-```
+```
+
+## Tests
+
+**Tests are still a Work In Progress.**
+The tests rely on `mocha`, `chai`, `sinon`, and `sinon-chai`, and are currently a mix of system and unit tests.
+
+### Naming
+
+**TODO:** Split out system tests from unit tests and then revise the below guidlines.
+
+These guidelines are inspired by [Better Specs { rspec guidelines with ruby }](https://www.betterspecs.org). Given that these guidelines are, by necessity, quite subjective, consider them as beneficial for the consistency rather than the "correctness" that they provide.
+
+The linter should catch basic stylistic errors (e.g. prefer arrow functions unless `this` is needed).
+
+---
+
+When naming individual `it` blocks, the resulting test name should read like a sentence. For example: `it('returns false if the repo has no filters')`.
+
+Directly describe what the test should do. Use verbs like `returns`, `errors`, `responds`, `reads`, etc. rather than prefixing tests with `should`, suffixing them with `correctly`, etc.. For example:
+
+``` javascript
+/* Bad */
+it('should read globs from `.gitattributes` correctly', function () {
+  // ...
+});
+
+/* Good */
+it('reads globs from `.gitattributes`', function () {
+  // ...
+});
+```
+
+---
+
+If you have the temptation to start a test with `can`, try to restructure the test to use a nested `describe` instead (whose message starts with `when`). For example:
+
+``` javascript
+/* Bad */
+it('can initialize a non-LFS repo', function () {
+  // ...
+});
+
+/* Good */
+describe('when provided a non-LFS repo', () => {
+  it('initializes the repo', function () {
+    // ...
+  });
+});
+```
+
+---
+
+For tests whose behavior varies cross-platform, surround the tests with a `process.platform` test. Be sure to add tests for all platforms where behavior could vary, even if you cannot run them locally. Because CI will run on Windows, macOS, and Linux, this should not be an obstacle to thorough test coverage. For example:
+
+``` javascript
+/* Bad */
+describe('when on Windows', () => {
+  beforeEach(() => {
+    sinon.stub(process, 'platform').returns('win32');
+  });
+
+  it('runs the provided command', function () {
+    // ...
+  });
+});
+
+describe('when on macOS', () => {
+  beforeEach(() => {
+    sinon.stub(process, 'platform').returns('darwin');
+  });
+
+  it('runs the provided command', function () {
+    // ...
+  });
+});
+
+describe('when on Linux', () => {
+  beforeEach(() => {
+    sinon.stub(process, 'platform').returns('linux');
+  });
+
+  it('runs the provided command', function () {
+    // ...
+  });
+});
+
+/* Good */
+switch (process.platform) {
+  case 'win32':
+    it('runs the provided command', function () {
+      // ...
+    });
+    break;
+  case 'macOS':
+    it('runs the provided command', function () {
+      // ...
+    });
+    break;
+  case 'linux':
+    it('runs the provided command', function () {
+      // ...
+    });
+    break;
+}
+```
+
+---
+
+When referring to parameters, use `provided` rather than `passed-in`, `given`, etc. For example:
+
+``` javascript
+/* Bad */
+it('runs the passed-in command', function () {
+  // ...
+});
+
+/* Good */
+it('runs the provided command', function () {
+  // ...
+});
+```
+
+---
+
+Each implementation file should correspond to a similarly-named `<file name>.spec.js` test file in the same relative location. For example, `src/callbacks/check.js` should have a test file located in `test/tests/callbacks/check.spec.js`.
+
+The top-level `describe` should be named after the file. `describe`s for each function should be named after the function itself. On a related note, each function should have its own `describe` block, even if there is only `it` block inside it. In the case that a function does not have a pre-defined name (e.g. it's an anonymous function exported as the default), refer to it as `the default export`. For example:
+
+``` javascript
+/* For named exports: */
+describe('helpers', () => { // Named after the file name, `helpers.spec.js`
+  describe('hasLfsFilters', () => { // Named after the exported function name, `hasLfsFilters`
+    // ...
+  });
+});
+
+/* For default exports: */
+describe('check', () => {
+  describe('the default export', () => {
+    // ...
+  });
+});
+```

package.json

@@ -1,7 +1,7 @@
 {
   "name": "nodegit-lfs",
   "version": "0.0.11",
-  "description": "A wrapper for NodeGit to facilitate LFS Support",
+  "description": "A wrapper for NodeGit to facilitate LFS support",
   "main": "build/src/index.js",
   "scripts": {
     "build": "yarn eslint && yarn compile",
@@ -36,9 +36,6 @@
     "nodegit": ">=0.20.1"
   },
   "devDependencies": {
-    "babel-cli": "^6.24.1",
-    "babel-core": "^6.25.0",
-    "babel-preset-es2015": "^6.24.1",
     "chai": "^4.0.2",
     "eslint": "^3.19.0",
     "eslint-config-airbnb-base": "^11.2.0",
@@ -48,15 +45,22 @@
     "jsdoc-to-markdown": "^3.0.0",
     "mocha": "^3.4.2",
     "nyc": "^11.0.3",
-    "prompt": "^1.0.0"
+    "prompt": "^1.0.0",
+    "sinon": "^3.2.1",
+    "sinon-chai": "^2.13.0"
   },
   "dependencies": {
+    "babel-cli": "^6.24.1",
+    "babel-core": "^6.25.0",
+    "babel-plugin-transform-object-rest-spread": "^6.26.0",
+    "babel-preset-es2015": "^6.24.1",
     "default-shell": "^1.0.1",
     "fs-extra": "^3.0.1",
     "ignore": "^3.3.3",
     "node-pty": "github:implausible/node-pty#57f5f4ef6b751ffcd056fb6f2b626a0a0d804301",
     "promisify-node": "^0.4.0",
     "ramda": "^0.24.1",
+    "semver": "^5.4.1",
     "tmp": "^0.0.33"
   }
 }

src/callbacks/check.js

@@ -8,11 +8,9 @@ export default src => loadGitattributeFiltersFromRepo(src.repo())
     const file = src.path();
     const filterIgnore = ignore().add(filters);
 
-    // these ignore rules are the closest to the .gitignore rules I have found
-    if (filterIgnore.ignores(file)) {
-      return Error.CODE.OK;
-    }
-
-    return Error.CODE.PASSTHROUGH;
+    // The rules for `.gitignore` are close enough to those for `.gitattributes`
+    return filterIgnore.ignores(file)
+      ? Error.CODE.OK
+      : Error.CODE.PASSTHROUGH;
   })
   .catch(() => Error.CODE.PASSTHROUGH);

src/commands/checkout.js

@@ -3,12 +3,11 @@ import { core } from './lfsCommands';
 import {
   regex,
   BAD_CORE_RESPONSE,
-  BAD_REGEX_PARSE_RESULT,
+  BAD_REGEX_PARSE_RESULT
 } from '../constants';
 
 import generateResponse from '../utils/generateResponse';
 import {
-  regexResult,
   verifyOutput,
   errorCatchHandler
 } from '../helpers';
@@ -22,7 +21,7 @@ const generateCheckoutStats = (raw) => {
     const filteredLines = R.filter(isValidLine, outputLines);
     const statLine = filteredLines.pop();
 
-    const byteResults = regexResult(statLine, regex.TOTAL_BYTES);
+    const byteResults = statLine.match(regex.TOTAL_BYTES);
 
     stats.total_bytes_checked_out =
       byteResults !== null ?
@@ -32,19 +31,19 @@ const generateCheckoutStats = (raw) => {
       byteResults !== null ?
         byteResults[1].trim() : BAD_REGEX_PARSE_RESULT;
 
-    const fileResults = regexResult(statLine, regex.TOTAL_FILES);
+    const fileResults = statLine.match(regex.TOTAL_FILES);
 
     stats.total_files_checked_out =
       fileResults !== null ?
         fileResults[0].trim() : BAD_REGEX_PARSE_RESULT;
 
-    const skippedByteResults = regexResult(statLine, regex.SKIPPED_BYTES);
+    const skippedByteResults = statLine.match(regex.SKIPPED_BYTES);
 
     stats.total_bytes_skipped =
       skippedByteResults !== null ?
         skippedByteResults[0].trim() : BAD_REGEX_PARSE_RESULT;
 
-    const skippedFileResults = regexResult(statLine, regex.SKIPPED_FILES);
+    const skippedFileResults = statLine.match(regex.SKIPPED_FILES);
 
     stats.total_files_skipped =
       skippedFileResults !== null ?
@@ -62,11 +61,9 @@ const generateCheckoutStats = (raw) => {
 };
 
 function checkout(repo, callback) {
-  const response = generateResponse();
-  const repoPath = repo.workdir();
-
-  return core.checkout('', { cwd: repoPath }, callback)
+  return core.checkout('', { cwd: repo.workdir() }, callback)
     .then(({ stdout }) => {
+      const response = generateResponse();
       response.raw = stdout;
       response.checkout = generateCheckoutStats(stdout);
 
@@ -77,7 +74,7 @@ function checkout(repo, callback) {
       }
 
       return response;
-    }, errorCatchHandler(response));
+    }, errorCatchHandler);
 }
 
 export default checkout;

src/commands/clone.js

@@ -2,13 +2,13 @@ import R from 'ramda';
 import { core } from './lfsCommands';
 import {
   regex,
-  BAD_REGEX_PARSE_RESULT,
+  BAD_REGEX_PARSE_RESULT
 } from '../constants';
 import generateResponse from '../utils/generateResponse';
 import {
-  regexResult,
   errorCatchHandler,
-  verifyOutput } from '../helpers';
+  verifyOutput
+} from '../helpers';
 
 const isValidLine = str => str !== '';
 
@@ -19,7 +19,7 @@ const generateCloneStats = (raw) => {
     const filteredLines = R.filter(isValidLine, outputLines);
     const statLine = filteredLines.pop();
 
-    const byteResults = regexResult(statLine, regex.TOTAL_BYTES);
+    const byteResults = statLine.match(regex.TOTAL_BYTES);
 
     stats.total_bytes_cloned =
       byteResults !== null ?
@@ -29,19 +29,19 @@ const generateCloneStats = (raw) => {
       byteResults !== null ?
         byteResults[1].trim() : BAD_REGEX_PARSE_RESULT;
 
-    const fileResults = regexResult(statLine, regex.TOTAL_FILES);
+    const fileResults = statLine.match(regex.TOTAL_FILES);
 
     stats.total_files_cloned =
       fileResults !== null ?
         fileResults[0].trim() : BAD_REGEX_PARSE_RESULT;
 
-    const skippedByteResults = regexResult(statLine, regex.SKIPPED_BYTES);
+    const skippedByteResults = statLine.match(regex.SKIPPED_BYTES);
 
     stats.total_bytes_skipped =
       skippedByteResults !== null ?
         skippedByteResults[0].trim() : BAD_REGEX_PARSE_RESULT;
 
-    const skippedFileResults = regexResult(statLine, regex.SKIPPED_FILES);
+    const skippedFileResults = statLine.match(regex.SKIPPED_FILES);
 
     stats.total_files_skipped =
       skippedFileResults !== null ?
@@ -66,16 +66,16 @@ function clone(url, cwd, options) {
   const {
     branch,
     callback,
+    env = {}
   } = (options || {});
   const args = branch ? `-b ${branch}` : '';
 
-  const response = generateResponse();
-  return core.clone(`${url} ${args}`, { cwd }, callback)
-    .then(({ stdout }) => {
-      response.raw = stdout;
-      response.clone = generateCloneStats(stdout);
-      return response;
-    }, errorCatchHandler(response));
+  return core.clone(`${url} ${args}`, { cwd, env }, callback)
+    .then(({ stdout }) => ({
+      ...generateResponse(),
+      raw: stdout,
+      clone: generateCloneStats(stdout)
+    }), errorCatchHandler);
 }
 
 export default clone;